* [PATCH] [BUILTIN] Make "test -x" sane again when run as root @ 2011-09-26 21:16 Jonathan Nieder 2011-09-26 22:16 ` Jonathan Nieder 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Nieder @ 2011-09-26 21:16 UTC (permalink / raw) To: dash; +Cc: Christoph Egger, Petr Salinger, Herbert Xu When dash switched from its own emulation to the true faccessat in v0.5.7~54 (2010-04-02), on some platforms (e.g., old versions of glibc-bsd), "test -x <path>" started returning true on all files when run as root. This violates POSIX.1-2008 §4.4 "File Access Permission", which says: If execute permission is requested, access shall be granted if execute permission is granted to at least one user by the file permission bits or by an alternate access control mechanism; otherwise, access shall be denied. Unfortunately, for historical reasons, access() and faccessat() are allowed by POSIX to return success for X_OK when the current process is privileged even when the above condition is not fulfilled and actual execution would fail. Work around this by checking the permissions bits when mode == X_OK and geteuid() == 0. Reported-by: Christoph Egger <christoph@debian.org> Analysis-by: Petr Salinger <Petr.Salinger@seznam.cz> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Thanks, all, and sorry to take so long in sending this patch out. Thoughts? src/bltin/test.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/src/bltin/test.c b/src/bltin/test.c index 90135e14..1093b59f 100644 --- a/src/bltin/test.c +++ b/src/bltin/test.c @@ -485,8 +485,19 @@ equalf (const char *f1, const char *f2) } #ifdef HAVE_FACCESSAT +static int has_exec_bit_set(const char *path) +{ + struct stat64 st; + + if (stat64(path, &st)) + return 0; + return st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH); +} + static int test_file_access(const char *path, int mode) { + if (mode == X_OK && geteuid() == 0 && !has_exec_bit_set(path)) + return 0; return !faccessat(AT_FDCWD, path, mode, AT_EACCESS); } #else /* HAVE_FACCESSAT */ -- 1.7.7.rc1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [BUILTIN] Make "test -x" sane again when run as root 2011-09-26 21:16 [PATCH] [BUILTIN] Make "test -x" sane again when run as root Jonathan Nieder @ 2011-09-26 22:16 ` Jonathan Nieder 2011-09-27 13:45 ` Herbert Xu 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Nieder @ 2011-09-26 22:16 UTC (permalink / raw) To: dash; +Cc: Christoph Egger, Petr Salinger, Herbert Xu Jonathan Nieder wrote: > [PATCH] [BUILTIN] Make "test -x" sane again when run as root Quick correction: a better title would be [BUILTIN] Fix "test -x" as root on platforms with old-fashioned faccessat for brevity ("fix" contains just as much information as "make sane") and to make it more obvious that this is supposed to have no effect on platforms like Linux. Sorry for the lack of clarity. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [BUILTIN] Make "test -x" sane again when run as root 2011-09-26 22:16 ` Jonathan Nieder @ 2011-09-27 13:45 ` Herbert Xu 2011-09-27 23:19 ` [PATCH v2] [BUILTIN] Fix "test -x" as root on FreeBSD 8 Jonathan Nieder 0 siblings, 1 reply; 5+ messages in thread From: Herbert Xu @ 2011-09-27 13:45 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Christoph Egger, Petr Salinger On Mon, Sep 26, 2011 at 05:16:49PM -0500, Jonathan Nieder wrote: > Jonathan Nieder wrote: > > > [PATCH] [BUILTIN] Make "test -x" sane again when run as root > > Quick correction: a better title would be > > [BUILTIN] Fix "test -x" as root on platforms with old-fashioned faccessat > > for brevity ("fix" contains just as much information as "make sane") > and to make it more obvious that this is supposed to have no effect on > platforms like Linux. Sorry for the lack of clarity. So pleas exclude the changes on Linux. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] [BUILTIN] Fix "test -x" as root on FreeBSD 8 2011-09-27 13:45 ` Herbert Xu @ 2011-09-27 23:19 ` Jonathan Nieder 2014-11-17 14:49 ` Herbert Xu 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Nieder @ 2011-09-27 23:19 UTC (permalink / raw) To: Herbert Xu; +Cc: dash, Christoph Egger, Petr Salinger POSIX.1-2008 §4.4 "File Access Permission" sayeth: If execute permission is requested, access shall be granted if execute permission is granted to at least one user by the file permission bits or by an alternate access control mechanism; otherwise, access shall be denied. For historical reasons, POSIX unfortunately also allows access() and faccessat() to return success for X_OK if the current process is privileged, even when the above condition is not fulfilled and actual execution would fail. On the affected platforms, "test -x <path>" as root started returning true on nonexecutable files when dash switched from its own emulation to the true faccessat in v0.5.7~54 (2010-04-02). Work around this by checking the permissions bits when mode == X_OK and geteuid() == 0 on such platforms. Unfortunately the behavior seems to vary from one kernel version to another, so we cannot just check the behavior at compile time and rely on that. A survey of some affected kernels: - NetBSD's kernel moved to the sane semantics in 1997 - OpenBSD's kernel made the same change in version 4.4, three years ago - FreeBSD 9's kernel fixes this but hasn't been released yet It seems safe to only apply the workaround on systems using the FreeBSD kernel for now, and to push for standardization on the expected access()/faccessat() semantics so we can drop the workaround altogether in a few years. To try it on other platforms, use "./configure --enable-test-workaround". Reported-by: Christoph Egger <christoph@debian.org> Analysis-by: Petr Salinger <Petr.Salinger@seznam.cz> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Herbert Xu wrote: > So pleas exclude the changes on Linux. Good idea. Even if the person checking can get root permissions to try it, a check at compile time is not enough to predict behavior at run time. For example: - glibc-bsd changed to the modern behavior on 2011-09-04 without a corresponding ABI bump - the kernel of FreeBSD changed to the modern behavior in 2010-08-30, r212002, and the change will be included in version 9 (but there is no plan to backport to version 8): http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/125009 - according to that FreeBSD problem report, Mac OS 10.4 uses the pre-modern behavior only for device files (!), OpenBSD 4.3 always uses the pre-modern behavior, and NetBSD uses the modern behavior - of course they all inherited that behavior from BSD as far as I can tell, which is why bash 1.11 and hence dash's !HAVE_FACCESSAT codepath defend against it. I don't know what AT&T unix did, but it probably doesn't matter. Probably best to use an explicit list of platforms and let the person building override it. How about this? configure.ac | 31 +++++++++++++++++++++++++++++++ src/bltin/test.c | 20 ++++++++++++++++++++ 2 files changed, 51 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index c6fb401a..980f553b 100644 --- a/configure.ac +++ b/configure.ac @@ -90,6 +90,37 @@ AC_CHECK_FUNCS(bsearch faccessat getpwnam getrlimit isalpha killpg \ sigsetmask stpcpy strchrnul strsignal strtod strtoimax \ strtoumax sysconf) +dnl Check whether it's worth working around FreeBSD PR kern/125009. +dnl The traditional behavior of access/faccessat is crazy, but +dnl POSIX.1-2008 explicitly allows those functions to misbehave. +dnl +dnl Unaffected kernels: +dnl +dnl - all versions of Linux +dnl - NetBSD sys/kern/vfs_subr.c 1.64, 1997-04-23 +dnl - FreeBSD 9 (r212002), 2010-09-10 +dnl - OpenBSD sys/kern/vfs_subr.c 1.166, 2008-06-09 +dnl +dnl Also worked around in Debian's libc0.1 2.13-19 when using +dnl kFreeBSD 8. + +AC_ARG_ENABLE(test-workaround, AS_HELP_STRING(--enable-test-workaround, \ + [Guard against faccessat(2) that tells root all files are executable]),, + [enable_test_workaround=auto]) + +if test "enable_test_workaround" = "auto" && + test "$ac_cv_func_faccessat" = yes; then + case `uname -s 2>/dev/null` in + GNU/kFreeBSD | \ + FreeBSD) + enable_test_workaround=yes + esac +fi +if test "$enable_test_workaround" = "yes"; then + AC_DEFINE([HAVE_TRADITIONAL_FACCESSAT], [1], + [Define if your faccessat tells root all files are executable]) +fi + if test "$enable_fnmatch" = yes; then use_fnmatch= AC_CHECK_FUNCS(fnmatch, use_fnmatch=yes) diff --git a/src/bltin/test.c b/src/bltin/test.c index 90135e14..a73d3a27 100644 --- a/src/bltin/test.c +++ b/src/bltin/test.c @@ -155,6 +155,14 @@ static int test_st_mode(const struct stat64 *, int); static int bash_group_member(gid_t); #endif +#ifdef HAVE_FACCESSAT +# ifdef HAVE_TRADITIONAL_FACCESSAT +static inline int faccessat_confused_about_superuser(void) { return 1; } +# else +static inline int faccessat_confused_about_superuser(void) { return 0; } +# endif +#endif + static inline intmax_t getn(const char *s) { return atomax10(s); @@ -485,8 +493,20 @@ equalf (const char *f1, const char *f2) } #ifdef HAVE_FACCESSAT +static int has_exec_bit_set(const char *path) +{ + struct stat64 st; + + if (stat64(path, &st)) + return 0; + return st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH); +} + static int test_file_access(const char *path, int mode) { + if (faccessat_confused_about_superuser() && + mode == X_OK && geteuid() == 0 && !has_exec_bit_set(path)) + return 0; return !faccessat(AT_FDCWD, path, mode, AT_EACCESS); } #else /* HAVE_FACCESSAT */ -- 1.7.7.rc1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] [BUILTIN] Fix "test -x" as root on FreeBSD 8 2011-09-27 23:19 ` [PATCH v2] [BUILTIN] Fix "test -x" as root on FreeBSD 8 Jonathan Nieder @ 2014-11-17 14:49 ` Herbert Xu 0 siblings, 0 replies; 5+ messages in thread From: Herbert Xu @ 2014-11-17 14:49 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Christoph Egger, Petr Salinger On Tue, Sep 27, 2011 at 06:19:06PM -0500, Jonathan Nieder wrote: > POSIX.1-2008 §4.4 "File Access Permission" sayeth: > > If execute permission is requested, access shall be granted > if execute permission is granted to at least one user by the > file permission bits or by an alternate access control > mechanism; otherwise, access shall be denied. > > For historical reasons, POSIX unfortunately also allows access() and > faccessat() to return success for X_OK if the current process is > privileged, even when the above condition is not fulfilled and actual > execution would fail. On the affected platforms, "test -x <path>" as > root started returning true on nonexecutable files when dash switched > from its own emulation to the true faccessat in v0.5.7~54 > (2010-04-02). > > Work around this by checking the permissions bits when mode == X_OK > and geteuid() == 0 on such platforms. > > Unfortunately the behavior seems to vary from one kernel version to > another, so we cannot just check the behavior at compile time and rely > on that. A survey of some affected kernels: > > - NetBSD's kernel moved to the sane semantics in 1997 > - OpenBSD's kernel made the same change in version 4.4, three years > ago > - FreeBSD 9's kernel fixes this but hasn't been released yet > > It seems safe to only apply the workaround on systems using the > FreeBSD kernel for now, and to push for standardization on the > expected access()/faccessat() semantics so we can drop the workaround > altogether in a few years. > > To try it on other platforms, use "./configure --enable-test-workaround". > > Reported-by: Christoph Egger <christoph@debian.org> > Analysis-by: Petr Salinger <Petr.Salinger@seznam.cz> > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> This patch seems to have slipped through the cracks. Nevertheless it is now applied. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-17 14:49 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-26 21:16 [PATCH] [BUILTIN] Make "test -x" sane again when run as root Jonathan Nieder 2011-09-26 22:16 ` Jonathan Nieder 2011-09-27 13:45 ` Herbert Xu 2011-09-27 23:19 ` [PATCH v2] [BUILTIN] Fix "test -x" as root on FreeBSD 8 Jonathan Nieder 2014-11-17 14:49 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).