[PATCH] O_DIRECTORY|O_CREAT handling

[PATCH] O_DIRECTORY|O_CREAT handling

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

The kernel currently handles open() calls with flags having at least
O_DIRECTORY|O_CREAT set strangely (to say the least).  It creates a
regular file, completely ignoring the O_DIRECTORY bit.  One can argue
that open() does not perform any real checking on the parameters and
that it is the programmers responsibility, but there is a twist.

Some programs create temporary directory which they afterward use as the
working directory or changed root.  I.e., we have code like this:

  mkdir ("/some/dirRANDOM");
  chdir ("/some/dirRANDOM");

or

  mkdir ("/some/dirRANDOM");
  fd = open ("/some/dirRANDOM", O_DIRECTORY);
  fchdir (fd);

or

  mkdir ("/some/dirRANDOM");
  chroot ("/some/dirRANDOM");

All these pieces of code have an obvious flaw, a race.  There is no
atomic way to do what we want.

Now combine these two problems.  How about making this work?

  fd = open ("/some/dirRANDOM", O_RDONLY|O_CREAT|O_DIRECTORY|O_EXCL, 0700);
  fchdir (fd);

(and similarly for a new interface fchroot which I can add to glibc).

I've attached a little patch which does just that.  Some comments on the
code:

 ~ if an existing directory is opened with O_RDWR open() returns
- -EISDIR.  I've mimicked this, allthough the error code might not be the
most obvious.

 ~ O_TRUNC is not allowed.


The small attached patch implements this proposal.  At least it does
what I want it to do, no idea what bugs I introduce.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (GNU/Linux)

iD8DBQE/5WqW2ijCOnn/RHQRAjZEAJ9SxzM1O6B/hQZN5jabqSSzXXtZwQCdH2sd
hRr+ejRNAU4Cl9V8MXpBIYo=
=jXqY
-----END PGP SIGNATURE-----

[-- Attachment #2: d-opendir --]
[-- Type: text/plain, Size: 1092 bytes --]

--- linux-2.6/fs/namei.c-opendir	2003-12-20 21:32:50.000000000 -0800
+++ linux-2.6/fs/namei.c	2003-12-21 01:37:09.000000000 -0800
@@ -1295,7 +1295,12 @@
 	if (!dentry->d_inode) {
 		if (!IS_POSIXACL(dir->d_inode))
 			mode &= ~current->fs->umask;
-		error = vfs_create(dir->d_inode, dentry, mode, nd);
+		if (unlikely(flag & O_DIRECTORY)) {
+			error = -EISDIR;
+			if (!(flag & FMODE_WRITE))
+				error = vfs_mkdir(dir->d_inode, dentry, mode);
+		} else
+			error = vfs_create(dir->d_inode, dentry, mode, nd);
 		up(&dir->d_inode->i_sem);
 		dput(nd->dentry);
 		nd->dentry = dentry;
@@ -1331,7 +1336,11 @@
 	dput(nd->dentry);
 	nd->dentry = dentry;
 	error = -EISDIR;
-	if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode))
+	/* Since we allow creating directories with open(2) we forbid
+	   opening an existing directory with O_CREAT only if the
+	   O_DIRECTORY flag is not set or if truncation is requested.  */
+	if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode) &&
+	    (!(flag & O_DIRECTORY) || (flag & O_TRUNC)))
 		goto exit;
 ok:
 	error = may_open(nd, acc_mode, flag);

Re: [PATCH] O_DIRECTORY|O_CREAT handling

[Willy Tarreau]

Hi Ulrich,

On Sun, Dec 21, 2003 at 01:40:39AM -0800, Ulrich Drepper wrote:
> 
> Some programs create temporary directory which they afterward use as the
> working directory or changed root.  I.e., we have code like this:
> 
>   mkdir ("/some/dirRANDOM");
>   chdir ("/some/dirRANDOM");
> 
> or
> 
>   mkdir ("/some/dirRANDOM");
>   fd = open ("/some/dirRANDOM", O_DIRECTORY);
>   fchdir (fd);
> 
> or
> 
>   mkdir ("/some/dirRANDOM");
>   chroot ("/some/dirRANDOM");
> 
> All these pieces of code have an obvious flaw, a race.  There is no
> atomic way to do what we want.

Although I agree on the race, I fail to see in what case it matters.
In my programs, I often use mkdir() to get temporary directories instead
of temporary files, just because of the atomicity of the test-and-set which
mkdir() provides. Basically, I do :

  base_dir="/tmp/tmpdir"; 
  do {
     rnd=random();
     sprintf(dir, "%s%d", base_dir, rnd);
  } while (!mkdir(dir, 0700);
  /* now I'm guaranteed that I'm the first to get this dir, */
  /* and only my UID can work in it */
  chdir(dir);
  
So the only race would be someone working with the same UID (or root) removing
the directory and replacing it with another one (or a symlink or anything)
between mkdir() and chdir(). But don't see any use or consequence to this.

> Now combine these two problems.  How about making this work?
> 
>   fd = open ("/some/dirRANDOM", O_RDONLY|O_CREAT|O_DIRECTORY|O_EXCL, 0700);
>   fchdir (fd);

It would be interesting, of course, but is it portable to other systems ? If
it is not, very few people will use it, unfortunately. But if others already
do it right, then why not include it ?

Cheers,
Willy

Re: [PATCH] O_DIRECTORY|O_CREAT handling

[Andrew Pimlott]

On Sun, Dec 21, 2003 at 11:51:10AM +0100, Willy Tarreau wrote:
>   base_dir="/tmp/tmpdir"; 
>   do {
>      rnd=random();
>      sprintf(dir, "%s%d", base_dir, rnd);
>   } while (!mkdir(dir, 0700);
>   /* now I'm guaranteed that I'm the first to get this dir, */
>   /* and only my UID can work in it */
>   chdir(dir);
>   
> So the only race would be someone working with the same UID (or root) removing
> the directory and replacing it with another one (or a symlink or anything)
> between mkdir() and chdir().

If /tmp isn't sticky, anyone can do this.  I think this is the
scenario Ulrich was referring to.

Andrew

Re: [PATCH] O_DIRECTORY|O_CREAT handling

[viro]

On Sun, Dec 28, 2003 at 01:03:41PM -0500, Andrew Pimlott wrote:
> On Sun, Dec 21, 2003 at 11:51:10AM +0100, Willy Tarreau wrote:
> >   base_dir="/tmp/tmpdir"; 
> >   do {
> >      rnd=random();
> >      sprintf(dir, "%s%d", base_dir, rnd);
> >   } while (!mkdir(dir, 0700);
> >   /* now I'm guaranteed that I'm the first to get this dir, */
> >   /* and only my UID can work in it */
> >   chdir(dir);
> >   
> > So the only race would be someone working with the same UID (or root) removing
> > the directory and replacing it with another one (or a symlink or anything)
> > between mkdir() and chdir().
> 
> If /tmp isn't sticky, anyone can do this.  I think this is the
> scenario Ulrich was referring to.

And if /etc/shadow is world-writable, anyone can do other interesting things.
Doctor, it hurts when I do it...