Re: bug in chdir-safer

[Paul Eggert]

Paul Eggert <address@hidden> writes:

> I don't think it's worth worrying about non-POSIX systems, so I'll
> prepare a patch that undoes that stuff (both here and elsewhere).
> There are some related things that need normalizing, too, e.g.,
> whether to open directories with O_NONBLOCK, O_DIRECTORY, and the like.

I installed the following patch along those lines.

2005-12-26  Paul Eggert  <address@hidden>

        * lib/chdir-long.c (cdb_free): Don't bother trying to open directory
        for write access: POSIX says that must fail.
        * lib/chdir-safer.c (chdir_no_follow): Likewise.
        * lib/fts.c (diropen): Likewise.
        * lib/save-cwd.c (save_cwd): Likewise.
        * lib/chdir-long.c (cdb_free): Open with O_NOCTTY | O_NONBLOCK as
        well, for minor improvements on hosts that lack O_DIRECTORY.
        * lib/chmod-safer.c (defined_S_IFMT): New macro.
        Include stat-macros.h.
        Include stdlib.h, for abort().
        Don't include stdio.h or assert.h; no longer needed.
        (same_file_type): Don't assume S_IFMT is defined, as POSIX
        does not require this.  Don't assume S_IFCHR and S_IFBLK have
        their usual sort of bit pattern.
        (fchmod_new): Open with O_NOCTTY for as well, for minor
        improvement on hosts where that matters.  Don't bother to assert,
        since the caller (in this source file) checks the same thing.
        Discard any errno from a close failure, for consistency with other
        code.
        * lib/chown.c (rpl_chown) [CHOWN_MODIFIES_SYMLINK]:
        Don't try O_WRONLY unless O_RDONLY failed wth EACCES.
        Fall back on chown if open failed with EACCES.

        * src/chown-core.c (restricted_chown):
        Don't try O_WRONLY unless O_RDONLY failed wth EACCES.
        * src/remove.c (fd_to_subdirp): Open with O_DIRECTORY | O_NOCTTY
        | O_NOFOLLOW too, for consistency with other dir-openers.
        Use POSIX-preferred O_NONBLOCK rather than O_NDELAY.
        (is_empty_dir): Likewise.
        * src/shred.c (wipename): Likewise.  Don't bother trying to open
        dir for writing, since POSIX prohibits it.

Index: lib/chdir-long.c
===================================================================
RCS file: /fetish/cu/lib/chdir-long.c,v
retrieving revision 1.8
diff -p -u -r1.8 chdir-long.c
--- lib/chdir-long.c    22 Sep 2005 06:05:39 -0000      1.8
+++ lib/chdir-long.c    27 Dec 2005 07:52:31 -0000
@@ -77,13 +77,10 @@ cdb_free (struct cd_buf const *cdb)
 static int
 cdb_advance_fd (struct cd_buf *cdb, char const *dir)
 {
-  int new_fd = openat (cdb->fd, dir, O_RDONLY | O_DIRECTORY);
+  int new_fd = openat (cdb->fd, dir,
+                      O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
   if (new_fd < 0)
-    {
-      new_fd = openat (cdb->fd, dir, O_WRONLY | O_DIRECTORY);
-      if (new_fd < 0)
-       return -1;
-    }
+    return -1;
 
   cdb_free (cdb);
   cdb->fd = new_fd;
Index: lib/chdir-safer.c
===================================================================
RCS file: /fetish/cu/lib/chdir-safer.c,v
retrieving revision 1.7
diff -p -u -r1.7 chdir-safer.c
--- lib/chdir-safer.c   26 Dec 2005 18:53:58 -0000      1.7
+++ lib/chdir-safer.c   27 Dec 2005 07:52:31 -0000
@@ -44,27 +44,18 @@
    && (Stat_buf_1).st_dev == (Stat_buf_2).st_dev)
 
 /* Like chdir, but fail if DIR is a symbolic link to a directory (or
-   similar funny business), or if DIR is neither readable nor
-   writable.  This avoids a minor race condition between when a
-   directory is created or statted and when the process chdirs into it.
-
-   On some systems, a writable yet unreadable directory cannot be
-   opened via open with O_WRONLY.  For example, on Linux 2.6, the
-   open syscall fails with EISDIR.  */
+   similar funny business), or if DIR not readable.  This avoids a
+   minor race condition between when a directory is created or statted
+   and when the process chdirs into it.  */
 int
 chdir_no_follow (char const *dir)
 {
   int result = 0;
   int saved_errno;
-  int open_flags = O_DIRECTORY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK;
-  int fd = open (dir, O_RDONLY | open_flags);
+  int fd = open (dir,
+                O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK);
   if (fd < 0)
-    {
-      if (errno == EACCES)
-       fd = open (dir, O_WRONLY | open_flags);
-      if (fd < 0)
-       return fd;
-    }
+    return -1;
 
   /* If open follows symlinks, lstat DIR and fstat FD to ensure that
      they are the same file; if they are different files, set errno to
Index: lib/chmod-safer.c
===================================================================
RCS file: /fetish/cu/lib/chmod-safer.c,v
retrieving revision 1.3
diff -p -u -r1.3 chmod-safer.c
--- lib/chmod-safer.c   23 Dec 2005 08:27:44 -0000      1.3
+++ lib/chmod-safer.c   27 Dec 2005 07:52:31 -0000
@@ -23,16 +23,21 @@
 
 #include "chmod-safer.h"
 
+#ifdef S_IFMT
+# define defined_S_IFMT true
+#else
+# define defined_S_IFMT false
+#endif
+
+#include "stat-macros.h"
+
 #include <stdbool.h>
-#include <stdio.h>
-#include <assert.h>
+#include <stdlib.h>
 #include <fcntl.h>
 #include <errno.h>
 #include <unistd.h>
 
-#include "fcntl--.h" /* for the open->open_safer mapping */
-
-#if !defined O_NOFOLLOW
+#ifndef O_NOFOLLOW
 # define O_NOFOLLOW 0
 #endif
 
@@ -48,12 +53,17 @@ static inline bool
 same_file_type (struct stat const *st, dev_t device, mode_t type)
 {
   /* The types must always match.  */
-  if ( ! (st->st_mode & type))
+  if (! (defined_S_IFMT ? (st->st_mode & S_IFMT) == type
+        : type == S_IFDIR ? S_ISDIR (st->st_mode)
+        : type == S_IFIFO ? S_ISFIFO (st->st_mode)
+        : type == S_IFBLK ? S_ISBLK (st->st_mode)
+        : type == S_IFCHR ? S_ISCHR (st->st_mode)
+        : (abort (), false)))
     return false;
 
   /* For character and block devices, the major and minor device
      numbers must match, too.  */
-  if (type & (S_IFCHR | S_IFBLK))
+  if (S_ISBLK (type) || S_ISCHR (type))
     return st->st_rdev == device;
 
   return true;
@@ -66,40 +76,39 @@ same_file_type (struct stat const *st, d
 static int
 fchmod_new (char const *file, mode_t mode, dev_t device, mode_t file_type)
 {
-  int fail = 1;
+  int result = 0;
   struct stat sb;
-  int saved_errno = 0;
-  int fd = open (file, O_NOFOLLOW | O_RDONLY | O_NDELAY);
+  int fd = open (file, O_RDONLY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK);
+  int saved_errno;
 
-  assert (O_NOFOLLOW);
+  if (fd < 0)
+    return -1;
 
-  if (0 <= fd
-      && fstat (fd, &sb) == 0
-      /* Given the entry we've just created, if its link count is
-        not 1 or its type/device has changed, then someone may be
-        trying to do something nasty.  However, the risk of such an
-        attack is so low that it isn't worth a special diagnostic.
-        Simply skip the fchmod and set errno, so that the
-        code below reports the failure to set permissions.
-        Note that we don't check the link count if the expected
-        type is `directory'.  */
-      && (((sb.st_nlink == 1 || file_type == S_IFDIR)
-          && same_file_type (&sb, device, file_type))
-         || ((errno = EACCES), 0))
-      && fchmod (fd, mode) == 0)
+  /* Given the entry we've just created, if its link count is
+     not 1 or its type/device has changed, then someone may be
+     trying to do something nasty.  However, the risk of such an
+     attack is so low that it isn't worth a special diagnostic.
+     Simply skip the fchmod and set errno, so that the
+     code below reports the failure to set permissions.
+     Note that we don't check the link count if the expected
+     type is `directory'.  */
+  result = fstat (fd, &sb);
+  if (result == 0)
     {
-      fail = 0;
+      if ((sb.st_nlink == 1 || S_ISDIR (file_type))
+         && same_file_type (&sb, device, file_type))
+       result = fchmod (fd, mode);
+      else
+       {
+         errno = EACCES;
+         result = -1;
+       }
     }
-  else
-    {
-      saved_errno = errno;
-    }
-
-  if (0 <= fd && close (fd) != 0 && saved_errno == 0)
-    saved_errno = errno;
 
+  saved_errno = errno;
+  close (fd);
   errno = saved_errno;
-  return fail;
+  return result;
 }
 
 /* Use a safer variant of chmod, if the underlying system facilities permit.
Index: lib/chown.c
===================================================================
RCS file: /fetish/cu/lib/chown.c,v
retrieving revision 1.16
diff -p -u -r1.16 chown.c
--- lib/chown.c 22 Sep 2005 06:05:39 -0000      1.16
+++ lib/chown.c 27 Dec 2005 07:52:31 -0000
@@ -27,12 +27,15 @@
    most systems.  */
 #undef chown
 
+#include <stdbool.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <errno.h>
 
+#include "stat-macros.h"
+
 /* Provide a more-closely POSIX-conforming version of chown on
    systems with one or both of the following problems:
    - chown doesn't treat an ID of -1 as meaning
@@ -66,20 +69,34 @@ rpl_chown (const char *file, uid_t uid, 
        on the symlink itself.  To work around that, we open the
        file (but this can fail due to lack of read or write permission) and
        use fchown on the resulting descriptor.  */
-    int fd = open (file, O_RDONLY | O_NONBLOCK | O_NOCTTY);
-    if (fd < 0
-       && (fd = open (file, O_WRONLY | O_NONBLOCK | O_NOCTTY)) < 0)
-      return -1;
-    if (fchown (fd, uid, gid))
+    int open_flags = O_NONBLOCK | O_NOCTTY;
+    int fd = open (file, O_RDONLY | open_flags);
+    if (0 <= fd
+       || (errno == EACCES
+           && 0 <= (fd = open (file, O_WRONLY | open_flags))))
       {
+       int result = fchown (fd, uid, gid);
        int saved_errno = errno;
+
+       /* POSIX says fchown can fail with errno == EINVAL on sockets,
+          so fall back on chown in that case.  */
+       struct stat sb;
+       bool fchown_socket_failure =
+         (result != 0 && saved_errno == EINVAL
+          && fstat (fd, &sb) == 0 && S_ISFIFO (sb.st_mode));
+
        close (fd);
-       errno = saved_errno;
-       return -1;
+
+       if (! fchown_socket_failure)
+         {
+           errno = saved_errno;
+           return result;
+         }
       }
-    return close (fd);
+    else if (errno != EACCES)
+      return -1;
   }
-#else
-  return chown (file, uid, gid);
 #endif
+
+  return chown (file, uid, gid);
 }
Index: lib/fts.c
===================================================================
RCS file: /fetish/cu/lib/fts.c,v
retrieving revision 1.37
diff -p -u -r1.37 fts.c
--- lib/fts.c   12 Aug 2005 13:00:57 -0000      1.37
+++ lib/fts.c   27 Dec 2005 07:52:31 -0000
@@ -203,10 +203,7 @@ static int
 internal_function
 diropen (char const *dir)
 {
-  int fd = open (dir, O_RDONLY | O_DIRECTORY);
-  if (fd < 0)
-    fd = open (dir, O_WRONLY | O_DIRECTORY);
-  return fd;
+  return open (dir, O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
 }
 
 FTS *
Index: lib/save-cwd.c
===================================================================
RCS file: /fetish/cu/lib/save-cwd.c,v
retrieving revision 1.31
diff -p -u -r1.31 save-cwd.c
--- lib/save-cwd.c      22 Sep 2005 06:05:39 -0000      1.31
+++ lib/save-cwd.c      27 Dec 2005 07:52:31 -0000
@@ -75,12 +75,8 @@ save_cwd (struct saved_cwd *cwd)
   cwd->desc = open (".", O_RDONLY);
   if (cwd->desc < 0)
     {
-      cwd->desc = open (".", O_WRONLY);
-      if (cwd->desc < 0)
-       {
-         cwd->name = xgetcwd ();
-         return cwd->name ? 0 : -1;
-       }
+      cwd->name = xgetcwd ();
+      return cwd->name ? 0 : -1;
     }
 
   return 0;
Index: src/chown-core.c
===================================================================
RCS file: /fetish/cu/src/chown-core.c,v
retrieving revision 1.37
diff -p -u -r1.37 chown-core.c
--- src/chown-core.c    30 May 2005 07:31:51 -0000      1.37
+++ src/chown-core.c    27 Dec 2005 07:52:31 -0000
@@ -191,15 +191,13 @@ restricted_chown (char const *file,
 {
   enum RCH_status status = RC_ok;
   struct stat st;
-  int o_flags = (O_NONBLOCK | O_NOCTTY);
+  int open_flags = O_NONBLOCK | O_NOCTTY;
 
-  int fd = open (file, O_RDONLY | o_flags);
-  if (fd < 0)
-    {
-      fd = open (file, O_WRONLY | o_flags);
-      if (fd < 0)
-       return RC_error;
-    }
+  int fd = open (file, O_RDONLY | open_flags);
+  if (! (0 <= fd
+        || (errno == EACCES
+            && 0 <= (fd = open (file, O_WRONLY | open_flags)))))
+    return RC_error;
 
   if (fstat (fd, &st) != 0)
     {
Index: src/remove.c
===================================================================
RCS file: /fetish/cu/src/remove.c,v
retrieving revision 1.141
diff -p -u -r1.141 remove.c
--- src/remove.c        17 Dec 2005 13:46:27 -0000      1.141
+++ src/remove.c        27 Dec 2005 07:52:32 -0000
@@ -578,15 +578,16 @@ AD_is_removable (Dirstack_state const *d
   return ! (top->unremovable && hash_lookup (top->unremovable, file));
 }
 
-/* Return true if DIR is determined to be an empty directory
-   or if fdopendir or readdir fails.  */
+/* Return true if DIR is determined to be an empty directory.  */
 static bool
 is_empty_dir (int fd_cwd, char const *dir)
 {
   DIR *dirp;
   struct dirent const *dp;
   int saved_errno;
-  int fd = openat (fd_cwd, dir, O_RDONLY | O_NDELAY);
+  int fd = openat (fd_cwd, dir,
+                  (O_RDONLY | O_DIRECTORY
+                   | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK));
 
   if (fd < 0)
     return false;
@@ -987,8 +988,8 @@ fd_to_subdirp (int fd_cwd, char const *f
               struct stat *subdir_sb, Dirstack_state *ds,
               int *cwd_errno ATTRIBUTE_UNUSED)
 {
-  int fd_sub = openat_permissive (fd_cwd, f, O_RDONLY | O_NOFOLLOW,
-                                 0, cwd_errno);
+  int open_flags = O_RDONLY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK;
+  int fd_sub = openat_permissive (fd_cwd, f, open_flags, 0, cwd_errno);
 
   /* Record dev/ino of F.  We may compare them against saved values
      to thwart any attempt to subvert the traversal.  They are also used
Index: src/shred.c
===================================================================
RCS file: /fetish/cu/src/shred.c,v
retrieving revision 1.121
diff -p -u -r1.121 shred.c
--- src/shred.c 12 Dec 2005 22:43:16 -0000      1.121
+++ src/shred.c 27 Dec 2005 07:52:32 -0000
@@ -1031,9 +1031,7 @@ wipename (char *oldname, char const *qol
   bool first = true;
   bool ok = true;
 
-  int dir_fd = open (dir, O_WRONLY | O_NOCTTY);
-  if (dir_fd < 0)
-    dir_fd = open (dir, O_RDONLY | O_NOCTTY);
+  int dir_fd = open (dir, O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
 
   if (flags->verbose)
     error (0, 0, _("%s: removing"), qoldname);