Re: bug in chdir-safer

[Paul Eggert]

Jim Meyering <address@hidden> writes:

>       * chdir-safer.c (chdir_no_follow): Remove unnecessary
>       test of S_ISDIR (sb_init.st_mode).

I had noticed that problem along with some others with chdir-safer.c.
I installed the following patch to clean up the other issues I saw.

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

        * chdir-safer.h (FCHMOD_SAFER_H): Remove: it was misnamed, and
        wasn't needed anyay.
        * chdir-safer.c (chdir_no_follow): Don't include stdio.h, assert.h,
        fcntl--.h; not needed.
        (O_DIRECTORY): Define if not already defined.
        (chdir_no_follow): Revamp describing comment to match code more
        closely.  Redo use of internal vars to avoid lint complaints.
        Work even if directory is writeable but not readable.
        Open with O_DIRECTORY | O_NOCTTY, for benefit of hosts that
        don't have O_NOFOLLOW.  Use O_NONBLOCK (POSIX spelling) rather
        than O_NDELAY.  Don't bother invoking fstat if open does not
        dereference symlink, since the result isn't used then.
        Don't assume file descriptor is positive; it might be zero
        now that we no longer include fcntl--.h (we don't need fcntl--.h
        since we immediately close the descriptor).

Index: lib/chdir-safer.h
===================================================================
RCS file: /fetish/cu/lib/chdir-safer.h,v
retrieving revision 1.1
diff -p -u -r1.1 chdir-safer.h
--- lib/chdir-safer.h   21 Dec 2005 09:42:36 -0000      1.1
+++ lib/chdir-safer.h   25 Dec 2005 21:33:53 -0000
@@ -1,4 +1,5 @@
-/* like chdir(2), but safer, if possible
+/* much like chdir(2), but safer
+
    Copyright (C) 2005 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
@@ -17,9 +18,4 @@
 
 /* Written by Jim Meyering.  */
 
-#ifndef FCHMOD_SAFER_H
-# define FCHMOD_SAFER_H 1
-
 int chdir_no_follow (char const *file);
-
-#endif /* FCHMOD_SAFER_H */
Index: lib/chdir-safer.c
===================================================================
RCS file: /fetish/cu/lib/chdir-safer.c,v
retrieving revision 1.4
diff -p -u -r1.4 chdir-safer.c
--- lib/chdir-safer.c   25 Dec 2005 17:33:57 -0000      1.4
+++ lib/chdir-safer.c   25 Dec 2005 21:33:53 -0000
@@ -1,4 +1,5 @@
-/* like chdir(2), but safer, if possible
+/* much like chdir(2), but safer
+
    Copyright (C) 2005 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
@@ -24,17 +25,16 @@
 #include "chdir-safer.h"
 
 #include <stdbool.h>
-#include <stdio.h>
-#include <assert.h>
 #include <fcntl.h>
 #include <errno.h>
-#include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
-#include "fcntl--.h" /* for the open->open_safer mapping */
+#ifndef O_DIRECTORY
+# define O_DIRECTORY 0
+#endif
 
-#if !defined O_NOFOLLOW
+#ifndef O_NOFOLLOW
 # define O_NOFOLLOW 0
 #endif
 
@@ -42,51 +42,52 @@
   ((Stat_buf_1).st_ino == (Stat_buf_2).st_ino \
    && (Stat_buf_1).st_dev == (Stat_buf_2).st_dev)
 
-/* Just like chmod, but fail if DIR is a symbolic link.
-   This can avoid a minor race condition between when a
-   directory is created or stat'd and when we chdir into it.
-
-   Note that this function fails (while chdir would succeed)
-   if DIR cannot be opened with O_RDONLY.  */
+/* Like chdir, but fail if DIR is a symbolic link to a directory (or
+   similar funny business), or if DIR is neither readable nor
+   writeable.  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 fail = -1;
-  struct stat sb;
-  struct stat sb_init;
-  int saved_errno = 0;
-  int fd;
-
-  bool open_dereferences_symlink = ! O_NOFOLLOW;
-
-  /* If open follows symlinks, lstat DIR, to get its device and
-     inode numbers.  */
-  if (open_dereferences_symlink && lstat (dir, &sb_init) != 0)
-    return fail;
-
-  fd = open (dir, O_NOFOLLOW | O_RDONLY | O_NDELAY);
-
-  if (0 <= fd
-      && fstat (fd, &sb) == 0
-      /* If DIR is a different directory, then someone is 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 fchdir
-        and set errno (to the same value that open uses for symlinks with
-        O_NOFOLLOW), so that the caller can report the failure.  */
-      && ( ! open_dereferences_symlink || SAME_INODE (sb_init, sb)
-         || ((errno = ELOOP), 0))
-      && fchdir (fd) == 0)
+  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);
+  if (fd < 0)
     {
-      fail = 0;
+      if (errno == EACCES)
+       fd = open (dir, O_WRONLY | open_flags);
+      if (fd < 0)
+       return fd;
     }
-  else
+
+  /* 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
+     ELOOP (the same value that open uses for symlinks with
+     O_NOFOLLOW) so the caller can report a failure.  */
+  if (! O_NOFOLLOW)
     {
-      saved_errno = errno;
+      struct stat sb1;
+      struct stat sb2;
+
+      result = lstat (dir, &sb1);
+      if (result == 0)
+       {
+         result = fstat (fd, &sb2);
+         if (result == 0 && ! SAME_INODE (sb1, sb2))
+           {
+             errno = ELOOP;
+             result = -1;
+           }
+       }
     }
 
-  if (0 < fd)
-    close (fd); /* Ignore any failure.  */
+  if (result == 0)
+    result = fchdir (fd);
 
+  saved_errno = errno;
+  close (fd);
   errno = saved_errno;
-  return fail;
+  return result;
 }