test -x should use faccessat, not stat

test -x should use faccessat, not stat

[Eric Blake]

This report was originally raised on the cygwin list:

http://cygwin.com/ml/cygwin/2010-02/msg00239.html

In short, in the presence of ACLs, dash's implementation of test -r, test -w, 
and test -x gives incorrect answers, when the current user has permissions to 
access a file that were granted by ACLs but not by the current stat() 
permissions.  dash should be using faccessat(,AT_EACCESS) (or 
eaccess/euidaccess) if available, rather than stat(), to determine whether a 
file is accessible.

-- 
Eric Blake

Re: test -x should use faccessat, not stat

[Herbert Xu]

Eric Blake <ebb9@byu.net> wrote:
> This report was originally raised on the cygwin list:
> 
> http://cygwin.com/ml/cygwin/2010-02/msg00239.html
> 
> In short, in the presence of ACLs, dash's implementation of test -r, test -w, 
> and test -x gives incorrect answers, when the current user has permissions to 
> access a file that were granted by ACLs but not by the current stat() 
> permissions.  dash should be using faccessat(,AT_EACCESS) (or 
> eaccess/euidaccess) if available, rather than stat(), to determine whether a 
> file is accessible.

What does bash to in this case?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: test -x should use faccessat, not stat

[Eric Blake]

According to Herbert Xu on 2/13/2010 11:11 PM:
> Eric Blake <ebb9@byu.net> wrote:
>> This report was originally raised on the cygwin list:
>>
>> http://cygwin.com/ml/cygwin/2010-02/msg00239.html
>>
>> In short, in the presence of ACLs, dash's implementation of test -r, test -w, 
>> and test -x gives incorrect answers, when the current user has permissions to 
>> access a file that were granted by ACLs but not by the current stat() 
>> permissions.  dash should be using faccessat(,AT_EACCESS) (or 
>> eaccess/euidaccess) if available, rather than stat(), to determine whether a 
>> file is accessible.
> 
> What does bash to in this case?

The bash source code shows the following:

In test.c, unary_test() calls sh_eaccess for test -r, -w, and -x.

In lib/sh/eaccess.c, bash currently uses:

int
sh_eaccess (path, mode)
     char *path;
     int mode;
{
  if (path_is_devfd (path))
    return (sh_stataccess (path, mode));

#if defined (HAVE_EACCESS)		/* FreeBSD */
  return (eaccess (path, mode));
#elif defined (EFF_ONLY_OK)		/* SVR4(?), SVR4.2 */
  return access (path, mode|EFF_ONLY_OK);
#else
  if (mode == F_OK)
    return (sh_stataccess (path, mode));

#  if HAVE_DECL_SETREGID
  if (current_user.uid != current_user.euid || current_user.gid !=
current_user.egid)
    return (sh_euidaccess (path, mode));
#  endif

  if (current_user.uid == current_user.euid && current_user.gid ==
current_user.egid)
    return (access (path, mode));

  return (sh_stataccess (path, mode));
#endif
}

But this could probably be improved to take advantage of the standardized
faccessat(path,mode,AT_EACCESS) in the case where that exists.

Furthermore, the link to the post on the cygwin list shows that bash, zsh,
and pdksh all honored ACLs, and that dash is the odd man out for not
recognizing when the current user has rights due to ACLs that are not
visible through the stat mode bits.

Finally, it is worth pointing out that on at least cygwin, faccessat and
friends are faster than stat.  Do you want me to prepare the patch?

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

Re: test -x should use faccessat, not stat

[Herbert Xu]

On Mon, Feb 15, 2010 at 06:31:14AM -0700, Eric Blake wrote:
> 
> Finally, it is worth pointing out that on at least cygwin, faccessat and
> friends are faster than stat.  Do you want me to prepare the patch?

First of all I completely agree with you that the current dash
behaviour is busted.  As to how we should fix it, let me think
about this a little more before making a decision.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: test -x should use faccessat, not stat

[Herbert Xu]

On Tue, Feb 16, 2010 at 08:04:13PM +0800, Herbert Xu wrote:
> 
> First of all I completely agree with you that the current dash
> behaviour is busted.  As to how we should fix it, let me think
> about this a little more before making a decision.

After much deliberation (alright, I've simply been busy elsewhere :)
I've committed this patch.

commit 1d68712ba2e439f36874c4ed1e3d9ffec177a06c
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Fri Apr 2 22:02:22 2010 +0800

    [BUILTIN] Use faccessat if available
    
    Eric Blake suggested that we should use faccessat so that ACLs
    and other corner cases are handled correctly.  This patch does
    exactly that.
    
    Note that faccessat doesn't handle ACLs when euid != uid, as
    this case is currently implemented by glibc instead of the kernel,
    using code similar to the existing dash test.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 7af5070..9f63819 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2010-04-02  Herbert Xu <herbert@gondor.apana.org.au>
 
+	* Use faccessat if available.
+
+2010-04-02  Herbert Xu <herbert@gondor.apana.org.au>
+
 	* Make trap signal name/number errors non-fatal.
 	* Release 0.5.6.
 
diff --git a/configure.ac b/configure.ac
index df6e099..c943725 100644
--- a/configure.ac
+++ b/configure.ac
@@ -46,7 +46,8 @@ dnl Checks for header files.
 AC_CHECK_HEADERS(alloca.h)
 
 dnl Checks for library functions.
-AC_CHECK_FUNCS(bsearch getpwnam getrlimit imaxdiv isalpha killpg mempcpy \
+AC_CHECK_FUNCS(bsearch faccessat getpwnam getrlimit imaxdiv isalpha killpg \
+	       mempcpy \
 	       sigsetmask stpcpy strchrnul strsignal strtod strtoimax \
 	       strtoumax sysconf)
 
diff --git a/src/bltin/test.c b/src/bltin/test.c
index 8e7077a..7888f38 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -11,6 +11,7 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 
+#include <fcntl.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
@@ -147,8 +148,12 @@ static int isoperand(char **);
 static int newerf(const char *, const char *);
 static int olderf(const char *, const char *);
 static int equalf(const char *, const char *);
+#ifdef HAVE_FACCESSAT
+static int test_file_access(const char *, int);
+#else
 static int test_st_mode(const struct stat64 *, int);
 static int bash_group_member(gid_t);
+#endif
 
 static inline intmax_t getn(const char *s)
 {
@@ -295,6 +300,14 @@ primary(enum token n)
 			return strlen(*t_wp) != 0;
 		case FILTT:
 			return isatty(getn(*t_wp));
+#ifdef HAVE_FACCESSAT
+		case FILRD:
+			return test_file_access(*t_wp, R_OK);
+		case FILWR:
+			return test_file_access(*t_wp, W_OK);
+		case FILEX:
+			return test_file_access(*t_wp, X_OK);
+#endif
 		default:
 			return filstat(*t_wp, n);
 		}
@@ -364,12 +377,14 @@ filstat(char *nm, enum token mode)
 		return 0;
 
 	switch (mode) {
+#ifndef HAVE_FACCESSAT
 	case FILRD:
 		return test_st_mode(&s, R_OK);
 	case FILWR:
 		return test_st_mode(&s, W_OK);
 	case FILEX:
 		return test_st_mode(&s, X_OK);
+#endif
 	case FILEXIST:
 		return 1;
 	case FILREG:
@@ -469,6 +484,12 @@ equalf (const char *f1, const char *f2)
 		b1.st_ino == b2.st_ino);
 }
 
+#ifdef HAVE_FACCESSAT
+static int test_file_access(const char *path, int mode)
+{
+	return !faccessat(AT_FDCWD, path, mode, AT_EACCESS);
+}
+#else	/* HAVE_FACCESSAT */
 /*
  * Similar to what access(2) does, but uses the effective uid and gid.
  * Doesn't make the mistake of telling root that any file is executable.
@@ -519,3 +540,4 @@ bash_group_member(gid_t gid)
 
 	return (0);
 }
+#endif	/* HAVE_FACCESSAT */

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: test -x should use faccessat, not stat

[Eric Blake]

According to Herbert Xu on 4/2/2010 8:03 AM:
> After much deliberation (alright, I've simply been busy elsewhere :)
> I've committed this patch.
> 
> commit 1d68712ba2e439f36874c4ed1e3d9ffec177a06c
>     Note that faccessat doesn't handle ACLs when euid != uid, as
>     this case is currently implemented by glibc instead of the kernel,
>     using code similar to the existing dash test.

That faccessat bug is only true for current Linux kernels.  Cygwin
faccessat does the correct thing, even when euid != uid.

Thanks for applying this.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net