bug#12632: file permissions checking mishandled when setuid

[Paul Eggert]

On 10/13/2012 11:56 PM, Eli Zaretskii wrote:

> This won't compile on Windows, since there's no 'euidaccess' (yet).

Thanks, that should be easy enough to fix, so I gave it a whirl
(patch below).

> Emacs should be able to test whether a file exists even if it
> will be unable to access it later.

Emacs cannot do that.  What 'access' does is ask, "If Emacs were
to issue the seteuid system call, and change the effective user
ID to the real user ID, would Emacs then be able to see that the
file exists?"  This does not test whether the file exists; it tests
only whether Emacs could see that the file exists in a hypothetical
situation that never actually happens (because Emacs never issues
the seteuid system call).  But this isn't what is wanted here:
what is wanted is a test whether Emacs can currently see that the
file exists, and that is what euidaccess does.

> In any case, using 'euidaccess' here subtly changes the semantics of
> file-exists-p

Currently file-exists-p uses 'stat', and 'euidaccess' uses
the same access check that 'stat' does -- they both use the
effective uid.  So the semantics aren't changing here.

>> > 'dir' can't be nil there.
> file-name-directory can return nil

It's an absolute file name, so file-name-directory can't return nil.
(This point is moot now, since the patch doesn't remove the
unnecessary NILP check.)

Here's the incremental patch to add euidaccess to Windows.
I'm attaching the entire revised patch, relative to trunk
bzr 110544.

=== modified file 'nt/ChangeLog'
--- nt/ChangeLog        2012-10-08 14:14:22 +0000
+++ nt/ChangeLog        2012-10-14 18:06:01 +0000
@@ -1,3 +1,9 @@
+2012-10-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+       Use euidaccess, not access, when checking file permissions (Bug#12632).
+       * inc/ms-w32.h (euidaccess): Define this, not 'access', since
+       the mainline code now uses euidaccess.
+
 2012-10-08  Juanma Barranquero  <lekktu@gmail.com>
 
        * config.nt: Sync with autogen/config.in.

=== modified file 'nt/inc/ms-w32.h'
--- nt/inc/ms-w32.h     2012-09-30 21:36:42 +0000
+++ nt/inc/ms-w32.h     2012-10-14 18:06:01 +0000
@@ -160,8 +160,6 @@
 #endif
 
 /* Calls that are emulated or shadowed.  */
-#undef access
-#define access  sys_access
 #undef chdir
 #define chdir   sys_chdir
 #undef chmod
@@ -176,6 +174,7 @@
 #define dup     sys_dup
 #undef dup2
 #define dup2    sys_dup2
+#define euidaccess  sys_access
 #define fopen   sys_fopen
 #define link    sys_link
 #define localtime sys_localtime

=== modified file 'src/ChangeLog'
--- src/ChangeLog       2012-10-14 06:02:52 +0000
+++ src/ChangeLog       2012-10-14 18:06:01 +0000
@@ -5,14 +5,13 @@
        (LIBES): Use it.
        * callproc.c (init_callproc):
        * charset.c (init_charset):
-       * fileio.c (check_existing) [!DOS_NT]:
-       (check_executable) [!DOS_NT && !HAVE_EUIDACCESS]:
+       * fileio.c (check_existing, check_executable):
        * lread.c (openp, load_path_check):
        * process.c (allocate_pty):
        * xrdb.c (file_p):
        Use euidaccess, not access.  Use symbolic names instead of integers
        for the flags, as they're portable now.
-       * fileio.c (Ffile_readable_p) [!DOS_NT && !macintosh]:
+       * fileio.c (Ffile_readable_p):
        Use euidaccess, not stat + open + close.
        (file_directory_p): New function, which uses 'stat' on most places
        but 'access' (for efficiency) if WINDOWSNT.
@@ -21,6 +20,7 @@
        * lread.c (openp): When opening a file, use fstat rather than
        stat, as that avoids a permissions race.  When not opening a file,
        use file_directory_p rather than stat.
+       * msdos.c (init_environment, readlink): Use sys_access, not access.
 
 2012-10-13  Jan Djärv  <jan.h.d@swipnet.se>
 

=== modified file 'src/fileio.c'
--- src/fileio.c        2012-10-14 06:02:52 +0000
+++ src/fileio.c        2012-10-14 18:06:01 +0000
@@ -2424,16 +2424,7 @@
 bool
 check_existing (const char *filename)
 {
-#ifdef DOS_NT
-  /* The full emulation of Posix 'stat' is too expensive on
-     DOS/Windows, when all we want to know is whether the file exists.
-     So we use 'access' instead, which is much more lightweight.  */
-  /* FIXME: an euidaccess implementation should be added to the
-     DOS/Windows ports and this #ifdef branch should be removed.  */
-  return (access (filename, F_OK) >= 0);
-#else
   return euidaccess (filename, F_OK) == 0;
-#endif
 }
 
 /* Return true if file FILENAME exists and can be executed.  */
@@ -2441,16 +2432,7 @@
 static bool
 check_executable (char *filename)
 {
-#ifdef DOS_NT
-  /* FIXME: an euidaccess implementation should be added to the
-     DOS/Windows ports and this #ifdef branch should be removed.  */
-  struct stat st;
-  if (stat (filename, &st) < 0)
-    return 0;
-  return ((st.st_mode & S_IEXEC) != 0);
-#else /* not DOS_NT */
   return euidaccess (filename, X_OK) == 0;
-#endif /* not DOS_NT */
 }
 
 /* Return true if file FILENAME exists and can be written.  */
@@ -2546,16 +2528,7 @@
     return call2 (handler, Qfile_readable_p, absname);
 
   absname = ENCODE_FILE (absname);
-
-#if defined (DOS_NT) || defined (macintosh)
-  /* FIXME: an euidaccess implementation should be added to the
-     DOS/Windows ports and this #ifdef branch should be removed.  */
-  if (access (SDATA (absname), 0) == 0)
-    return Qt;
-  return Qnil;
-#else /* not DOS_NT and not macintosh */
   return euidaccess (SSDATA (absname), R_OK) == 0 ? Qt : Qnil;
-#endif /* not DOS_NT and not macintosh */
 }
 
 /* Having this before file-symlink-p mysteriously caused it to be forgotten
@@ -2592,7 +2565,7 @@
   /* The read-only attribute of the parent directory doesn't affect
      whether a file or directory can be created within it.  Some day we
      should check ACLs though, which do affect this.  */
-  return (access (SDATA (dir), D_OK) < 0) ? Qnil : Qt;
+  return file_directory_p (SDATA (dir)) ? Qt : Qnil;
 #else
   return (check_writable (!NILP (dir) ? SSDATA (dir) : "")
          ? Qt : Qnil);
@@ -2694,8 +2667,8 @@
 file_directory_p (char const *file)
 {
 #ifdef WINDOWSNT
-  /* 'access' is cheaper than 'stat'.  */
-  return access (file, D_OK) == 0;
+  /* This is cheaper than 'stat'.  */
+  return euidaccess (file, D_OK) == 0;
 #else
   struct stat st;
   return stat (file, &st) == 0 && S_ISDIR (st.st_mode);

=== modified file 'src/msdos.c'
--- src/msdos.c 2012-09-23 08:44:20 +0000
+++ src/msdos.c 2012-10-14 18:06:01 +0000
@@ -3557,7 +3557,7 @@
         read-only filesystem, like CD-ROM or a write-protected floppy.
         The only way to be really sure is to actually create a file and
         see if it succeeds.  But I think that's too much to ask.  */
-      if (tmp && access (tmp, D_OK) == 0)
+      if (tmp && sys_access (tmp, D_OK) == 0)
        {
          setenv ("TMPDIR", tmp, 1);
          break;
@@ -3935,7 +3935,7 @@
 readlink (const char *name, char *dummy1, size_t dummy2)
 {
   /* `access' is much faster than `stat' on MS-DOS.  */
-  if (access (name, F_OK) == 0)
+  if (sys_access (name, F_OK) == 0)
     errno = EINVAL;
   return -1;
 }

euidaccess.txt
Description: Text document