Re: Bug#304556: file permissions race in mkdir, mknod, mkfifo (CAN-2005-1039)

[Jim Meyering]

Joey Hess <address@hidden> wrote:
> Package: coreutils
> Version: 5.2.1-2
> Severity: important
> Tags: security
>
> Our coreutils seems to be vulnerable to the problem described in
> CAN-2005-1039.
>
> http://www.securityfocus.com/archive/1/395489
>
> A quick strace of "mkdir -m 400 foo" shows the problem:
>
> mkdir("foo", 0400)                      = 0
> chmod("foo", 0400)                      = 0
>
> So if this is run in a directory where the attacker has access, such as
> a group writable directory (as commonly used for teams in eg, the Debian
> project; on alioth, etc), then the attacker can race between the mkdir
> and chmod calls, (re)moving the new directory and replacing it with a
> symlink to a file or directory owned by the user who ran mkdir. The
> chmod will then follow the symlink and act on that directory. And
> similar for the other commands.
>
> A fix would be to create the directory/device with the right perms
> and not chmod it again afterwards.

Thanks for the report.
FYI, one approach to `fixing' this would cause problems.
For example, the following patch looks like an improvement,
but it would introduce a bug:

Index: mkdir.c
===================================================================
RCS file: /fetish/cu/src/mkdir.c,v
retrieving revision 1.91
diff -u -p -r1.91 mkdir.c
--- mkdir.c     21 Sep 2004 22:26:42 -0000      1.91
+++ mkdir.c     14 Apr 2005 11:43:31 -0000
@@ -183,6 +183,7 @@ main (int argc, char **argv)
             been created.  */
 
          if (ok && specified_mode && dir_created
+             && (newmode & ~S_IRWXUGO)
              && chmod (dir, newmode) != 0)
            {
              error (0, errno, _("cannot set permissions of directory %s"),

If the affected directory's parent has a default ACL or has the set group
ID bit set, then the chmod may be required e.g., to reset the inherited
set group ID bit or to adjust any S_IRWXUGO bits that were changed via
the default ACL.

An alternative approach is to lstat the new directory, compare its mode to
the desired mode and then, only if necessary, call chmod.  But this still
leaves the race-window open.

The only raceless approach I can see is to lstat the name, open it,
fstat its file descriptor, compare the dev/inode with those from lstat,
check the mode, and if needed, call fchmod.  But that too can fail:
if the new directory is not readable by the user who just created it,
then the open will fail.  But when setting such restrictive permissions,
resorting to the use of the race-vulnerable lstat+chown isn't a big deal.
But now we're talking about pretty much more code and a little added cost
(always call lstat,open,fstat,and-maybe-fchmod rather than just chmod).
All for a theoretically exploitable race condition.

Is it worth it in this case?
Opinions welcome.

For reference, here's a possible patch, relative to the upstream repository:
(I'm not sure I like the idea of calling chown upon failed open.
Also, I wouldn't really put this code in mkdir.c -- it'd be used
by mknod.c and mkfifo.c, too. )

Index: mkdir.c
===================================================================
RCS file: /fetish/cu/src/mkdir.c,v
retrieving revision 1.91
diff -u -p -r1.91 mkdir.c
--- mkdir.c     21 Sep 2004 22:26:42 -0000      1.91
+++ mkdir.c     14 Apr 2005 14:21:43 -0000
@@ -1,5 +1,5 @@
 /* mkdir -- make directories
-   Copyright (C) 90, 1995-2002, 2004 Free Software Foundation, Inc.
+   Copyright (C) 90, 1995-2002, 2004, 2005 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -28,6 +28,7 @@
 #include "makepath.h"
 #include "modechange.h"
 #include "quote.h"
+#include "unistd-safer.h"
 
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "mkdir"
@@ -78,6 +79,79 @@ Mandatory arguments to long options are 
   exit (status);
 }
 
+/* Call fchmod(2) on FILE only after ensuring that the file system
+   object to be modified is not a symbolic link.
+   If anything goes wrong,
+     - FILE is a symlink (set errno to EINVAL)
+     - failed system call
+     - FILE changed dev/inode between lstat and open calls (set errno to 
EINVAL)
+   skip the fchmod and return -1.  Otherwise, return 0.
+   In most cases, there is no possibility for an attacker to replace FILE
+   with say, a symlink, to cause the permissions of some other file to be
+   affected, but if FILE cannot be opened, we go ahead and call chown
+   directly.
+
+   This function is intended to be called on a FILE that has just been
+   created, and that may already have its mode set to MODE.  In the
+   relatively unusual case that MODE does not reflect the permissions
+   of FILE, then we set them.  */
+
+int
+safe_chmod (char const *file, mode_t mode)
+{
+  struct stat lstat_stats;
+  struct stat fstat_stats;
+  int fd;
+
+  if (lstat (file, &lstat_stats) != 0)
+    return -1;
+
+  if (S_ISLNK (lstat_stats.st_mode))
+    {
+      errno = EINVAL;
+      return -1;
+    }
+
+  fd = fd_safer (open (file, O_RDONLY));
+  if (fd < 0)
+    {
+      fd = fd_safer (open (file, O_WRONLY));
+      if (fd < 0)
+       return chmod (file, mode) == 0 ? 0 : -1;
+    }
+
+  if (fstat (fd, &fstat_stats) != 0)
+    {
+      int saved_errno = errno;
+      close (fd);
+      errno = saved_errno;
+      return -1;
+    }
+
+  if (! SAME_INODE (lstat_stats, fstat_stats))
+    {
+      close (fd);
+      errno = EINVAL;
+      return -1;
+    }
+
+  if (fstat_stats.st_mode != mode)
+    {
+      if (fchmod (fd, mode) != 0)
+       {
+         int saved_errno = errno;
+         close (fd);
+         errno = saved_errno;
+         return -1;
+       }
+    }
+
+  if (close (fd) != 0)
+    return -1;
+
+  return 0;
+}
+
 int
 main (int argc, char **argv)
 {
@@ -183,7 +257,7 @@ main (int argc, char **argv)
             been created.  */
 
          if (ok && specified_mode && dir_created
-             && chmod (dir, newmode) != 0)
+             && safe_chmod (dir, newmode) != 0)
            {
              error (0, errno, _("cannot set permissions of directory %s"),
                     quote (dir));