[Bug 1817959] Re: "test -e" inaccurately returns false when stat() is disallowed
To be clear, having stat() return an error is not "breaking UNIX" (re https://bazaar.launchpad.net/~mirabilos/mksh/MAIN/revision/2874). UNIX, as defined by IEEE Std 1003.1-2017, says: http://pubs.opengroup.org/onlinepubs/9699919799/functions/stat.html An implementation that provides additional or alternate file access control mechanisms may, under implementation-defined conditions, cause stat() to fail. In particular, the system may deny the existence of the file specified by path. Returning an error for stat() is well defined, UNIX standard behavior. Not properly handling a UNIX defined, standardized behavior is a bug. -- You received this bug notification because you are a member of mksh Mailing List, which is subscribed to mksh. Matching subscriptions: mkshlist-to-mksh-bugmail https://bugs.launchpad.net/bugs/1817959 Title: "test -e" inaccurately returns false when stat() is disallowed Status in mksh: Invalid Bug description: From "man 1 test" NAME test - check file types and compare values DESCRIPTION Exit with the status determined by EXPRESSION. [deleted] -e FILE FILE exists When "test -e" is called, it is intended to determine the existence or non-existence of a file. However, the "test" command is implemented using stat(), which may be disallowed by security policy. If stat() is disallowed, "test" will falsely claim a file doesn't exist when it really exists. Replacing "stat() == 0" with "access(F_OK) == 0" fixes this problem. See attached patch. To manage notifications about this bug go to: https://bugs.launchpad.net/mksh/+bug/1817959/+subscriptions
[Bug 1817789] Re: misleading error message for SELinux denials
To be clear, the current implementation of using stat(), reading the
permissions, then later exec()ing is subject to the same race conditions
described in the access() man page. Just because stat() doesn't include
these warnings in the man page shouldn't be interpreted that the current
usage is race condition free.
In fact, if the code wants to be race condition free, the appropriate
thing to do is to skip the access(X_OK) check *and* the stat() check,
and just do the exec(). This is the solution I proposed in comment #10
bullet #5. Since you're raising security concerns, I assume you want to
be race condition free?
As an alternative solution to deal with buggy or incomplete access()
implementations, we could also consider making the "stat()" call
conditional on the operating system. For Linux, in particular,
access(X_OK) is known to be robust, so we can omit the stat() check on
that operating system. I proposed a similar solution in
https://bugs.launchpad.net/mksh/+bug/1817959 comment #6.
--
You received this bug notification because you are a member of mksh
Mailing List, which is subscribed to mksh.
Matching subscriptions: mkshlist-to-mksh-bugmail
https://bugs.launchpad.net/bugs/1817789
Title:
misleading error message for SELinux denials
Status in mksh:
Opinion
Bug description:
Given a stat(2) failure caused by an SELinux denial (rather than a
stat(2) success and an access(2) failure, as with a regular `chmod
a-x` failure), mksh reports "not found" rather than the more correct
"Permission denied".
Expected:
* Permission Denied error message
Actual:
$ sh -c /system/bin/vold
sh: /system/bin/vold: not found
"not found" error message.
here's the behind-the-scenes SELinux denial:
02-25 22:37:11.023 4571 4571 W sh : type=1400 audit(0.0:347):
avc: denied { getattr } for path="/system/bin/vold" dev="dm-0" ino=717
scontext=u:r:shell:s0 tcontext=u:object_r:vold_exec:s0 tclass=file
permissive=0
here's what strace says happened:
newfstatat(AT_FDCWD, "/system/bin/vold", 0x7ffcc3ef20, 0) = -1 EACCES
(Permission denied)
write(2, "/system/bin/sh: /system/bin/vold"..., 44/system/bin/sh:
/system/bin/vold: not found
) = 44
versus the normal `chmod a-x` case where stat succeeds but access
fails:
newfstatat(AT_FDCWD, "/data/local/tmp/date2", {st_mode=S_IFREG|0644,
st_size=482560, ...}, 0) = 0
faccessat(AT_FDCWD, "/data/local/tmp/date2", X_OK) = -1 EACCES (Permission
denied)
write(2, "sh: /data/local/tmp/date2: can't"..., 60sh: /data/local/tmp/date2:
can't execute: Permission denied
) = 60
this patch fixes the issue:
```
diff --git a/src/exec.c b/src/exec.c
index 8330174..3f6d876 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -1279,8 +1279,8 @@ search_access(const char *fn, int mode)
struct stat sb;
if (stat(fn, ) < 0)
- /* file does not exist */
- return (ENOENT);
+ /* file may or may not exist: check errno */
+ return errno;
/* LINTED use of access */
if (access(fn, mode) < 0) {
/* file exists, but we can't access it */
```
i don't know if you want to elaborate further in the comment along the
lines of "...for example, an SELinux denial may mean that we get
EACCES here, or if the file doesn't exist and we're allowed to know
that, we'll get ENOENT".
result with patch:
$ sh -c /system/bin/vold
sh: /system/bin/vold: can't execute: Permission denied
To manage notifications about this bug go to:
https://bugs.launchpad.net/mksh/+bug/1817789/+subscriptions
[Bug 1817959] Re: "test -e" inaccurately returns false when stat() is disallowed
Since the concern raised is about portability and buggy implementations, another strategy which addresses "test" returning inaccurate information is to conditionally use access(F_OK) only on platforms where it is known to be reliable. One way (but not the only way) is to use ifdefs, eg. #ifdef LINUX return access(F_OK) == 0; #else return stat() == 0; #endif This makes the "test" builtin more accurate at the cost of a slightly greater code maintenance burden. I'm unsure if the disagreement here is that you don't think the behavior described is a bug, or we disagree on the best way to resolve this bug. -- You received this bug notification because you are a member of mksh Mailing List, which is subscribed to mksh. Matching subscriptions: mkshlist-to-mksh-bugmail https://bugs.launchpad.net/bugs/1817959 Title: "test -e" inaccurately returns false when stat() is disallowed Status in mksh: Invalid Bug description: From "man 1 test" NAME test - check file types and compare values DESCRIPTION Exit with the status determined by EXPRESSION. [deleted] -e FILE FILE exists When "test -e" is called, it is intended to determine the existence or non-existence of a file. However, the "test" command is implemented using stat(), which may be disallowed by security policy. If stat() is disallowed, "test" will falsely claim a file doesn't exist when it really exists. Replacing "stat() == 0" with "access(F_OK) == 0" fixes this problem. See attached patch. To manage notifications about this bug go to: https://bugs.launchpad.net/mksh/+bug/1817959/+subscriptions
[Bug 1817959] Re: "test -e" inaccurately returns false when stat() is disallowed
My system, the Android operating system, uses SELinux to disallow stat() for a large number of files and directories. This prevents side channel leakage between various untrustworthy processes, helping preserve user privacy and preserve the confidentiality of the system. For good reason, the ability to disallow stat is a common operation by all Linux Security Modules. access() is defined by the relevant POSIX standards, so I'm surprised to hear you say it's not portable and buggy. AFAIK, it's not buggy on your largest platform - Linux - and it is more reliable and efficient than relying on stat(), which may be disallowed. Thank you for evaluating this patch and your time. -- You received this bug notification because you are a member of mksh Mailing List, which is subscribed to mksh. Matching subscriptions: mkshlist-to-mksh-bugmail https://bugs.launchpad.net/bugs/1817959 Title: "test -e" inaccurately returns false when stat() is disallowed Status in mksh: Invalid Bug description: From "man 1 test" NAME test - check file types and compare values DESCRIPTION Exit with the status determined by EXPRESSION. [deleted] -e FILE FILE exists When "test -e" is called, it is intended to determine the existence or non-existence of a file. However, the "test" command is implemented using stat(), which may be disallowed by security policy. If stat() is disallowed, "test" will falsely claim a file doesn't exist when it really exists. Replacing "stat() == 0" with "access(F_OK) == 0" fixes this problem. See attached patch. To manage notifications about this bug go to: https://bugs.launchpad.net/mksh/+bug/1817959/+subscriptions
[Bug 1817959] Re: "test -e" inaccurately returns false when stat() is disallowed
If you insist on stat(), then it should be fairly straight forward to check errno. File exists if stat() returns success, *or* if stat() returns failure and errno != ENOENT. -- You received this bug notification because you are a member of mksh Mailing List, which is subscribed to mksh. Matching subscriptions: mkshlist-to-mksh-bugmail https://bugs.launchpad.net/bugs/1817959 Title: "test -e" inaccurately returns false when stat() is disallowed Status in mksh: Invalid Bug description: From "man 1 test" NAME test - check file types and compare values DESCRIPTION Exit with the status determined by EXPRESSION. [deleted] -e FILE FILE exists When "test -e" is called, it is intended to determine the existence or non-existence of a file. However, the "test" command is implemented using stat(), which may be disallowed by security policy. If stat() is disallowed, "test" will falsely claim a file doesn't exist when it really exists. Replacing "stat() == 0" with "access(F_OK) == 0" fixes this problem. See attached patch. To manage notifications about this bug go to: https://bugs.launchpad.net/mksh/+bug/1817959/+subscriptions
[Bug 1817789] Re: misleading error message for SELinux denials
Are you referring to Posix 1003.1 section "C.2.8.2 Exit Status for
Commands"?
Historical shells make the distinction between ‘‘utility not found’’ and
‘‘utility found but cannot execute’’ in their error messages. By specifying
two seldomly used exit status values for these cases, 127 and 126
respectively,
this gives an application the opportunity to make use of this distinction
without having to parse an error message that would probably change from
locale to locale. The command, env, nohup, and xargs utilities in the
Shell and Utilities volume of IEEE Std 1003.1-2001 have also been specified to
use this convention.
(I may be viewing an out of date version... my apologies if so)
A few comments:
1) Based on the above, POSIX also seems to require different error
messages for "not found" and "found but cannot execute". The entire
reason this bug exists is because mksh is failing to return different
error messages when the executable exists but the stat() fails. The
proposal from comment #8 seems to acknowledge this non-compliance, yet
not address it.
2) The stat() is unnecessary for distinguishing between a 126 and 127
error status. Instead, a much simpler way is to just execve() the file,
return 127 iff errno==ENOENT. I believe this is all Posix requires.
3) Manually checking permission bits is not an accurate indicator of
whether the file can be executed or not, primarily due to LSMs.
4) Calling stat() unnecessarily inhibits certain use cases, specifically
comment #7.
5) A failure of the stat() system call is not an accurate indicator of
the exec()-ability of the file. The only way to determine if a file is
executable is to execute it.
--
You received this bug notification because you are a member of mksh
Mailing List, which is subscribed to mksh.
Matching subscriptions: mkshlist-to-mksh-bugmail
https://bugs.launchpad.net/bugs/1817789
Title:
misleading error message for SELinux denials
Status in mksh:
Opinion
Bug description:
Given a stat(2) failure caused by an SELinux denial (rather than a
stat(2) success and an access(2) failure, as with a regular `chmod
a-x` failure), mksh reports "not found" rather than the more correct
"Permission denied".
Expected:
* Permission Denied error message
Actual:
$ sh -c /system/bin/vold
sh: /system/bin/vold: not found
"not found" error message.
here's the behind-the-scenes SELinux denial:
02-25 22:37:11.023 4571 4571 W sh : type=1400 audit(0.0:347):
avc: denied { getattr } for path="/system/bin/vold" dev="dm-0" ino=717
scontext=u:r:shell:s0 tcontext=u:object_r:vold_exec:s0 tclass=file
permissive=0
here's what strace says happened:
newfstatat(AT_FDCWD, "/system/bin/vold", 0x7ffcc3ef20, 0) = -1 EACCES
(Permission denied)
write(2, "/system/bin/sh: /system/bin/vold"..., 44/system/bin/sh:
/system/bin/vold: not found
) = 44
versus the normal `chmod a-x` case where stat succeeds but access
fails:
newfstatat(AT_FDCWD, "/data/local/tmp/date2", {st_mode=S_IFREG|0644,
st_size=482560, ...}, 0) = 0
faccessat(AT_FDCWD, "/data/local/tmp/date2", X_OK) = -1 EACCES (Permission
denied)
write(2, "sh: /data/local/tmp/date2: can't"..., 60sh: /data/local/tmp/date2:
can't execute: Permission denied
) = 60
this patch fixes the issue:
```
diff --git a/src/exec.c b/src/exec.c
index 8330174..3f6d876 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -1279,8 +1279,8 @@ search_access(const char *fn, int mode)
struct stat sb;
if (stat(fn, ) < 0)
- /* file does not exist */
- return (ENOENT);
+ /* file may or may not exist: check errno */
+ return errno;
/* LINTED use of access */
if (access(fn, mode) < 0) {
/* file exists, but we can't access it */
```
i don't know if you want to elaborate further in the comment along the
lines of "...for example, an SELinux denial may mean that we get
EACCES here, or if the file doesn't exist and we're allowed to know
that, we'll get ENOENT".
result with patch:
$ sh -c /system/bin/vold
sh: /system/bin/vold: can't execute: Permission denied
To manage notifications about this bug go to:
https://bugs.launchpad.net/mksh/+bug/1817789/+subscriptions
[Bug 1817789] Re: misleading error message for SELinux denials
Additionally, this behavior also causes problems where the security
policy writer, for whatever reason, wants to allow a file to be executed
but disallow stat() operations. This could occur, for example, in high
sensitivity environments where leaking metadata (size, last update time,
etc) about the file being executed could reveal other activity on the
system. By assuming that stat() must always succeed before the file can
be executed, such environments cannot create unstat()able executables
and have mksh run the executable.
--
You received this bug notification because you are a member of mksh
Mailing List, which is subscribed to mksh.
Matching subscriptions: mkshlist-to-mksh-bugmail
https://bugs.launchpad.net/bugs/1817789
Title:
misleading error message for SELinux denials
Status in mksh:
Opinion
Bug description:
Given a stat(2) failure caused by an SELinux denial (rather than a
stat(2) success and an access(2) failure, as with a regular `chmod
a-x` failure), mksh reports "not found" rather than the more correct
"Permission denied".
Expected:
* Permission Denied error message
Actual:
$ sh -c /system/bin/vold
sh: /system/bin/vold: not found
"not found" error message.
here's the behind-the-scenes SELinux denial:
02-25 22:37:11.023 4571 4571 W sh : type=1400 audit(0.0:347):
avc: denied { getattr } for path="/system/bin/vold" dev="dm-0" ino=717
scontext=u:r:shell:s0 tcontext=u:object_r:vold_exec:s0 tclass=file
permissive=0
here's what strace says happened:
newfstatat(AT_FDCWD, "/system/bin/vold", 0x7ffcc3ef20, 0) = -1 EACCES
(Permission denied)
write(2, "/system/bin/sh: /system/bin/vold"..., 44/system/bin/sh:
/system/bin/vold: not found
) = 44
versus the normal `chmod a-x` case where stat succeeds but access
fails:
newfstatat(AT_FDCWD, "/data/local/tmp/date2", {st_mode=S_IFREG|0644,
st_size=482560, ...}, 0) = 0
faccessat(AT_FDCWD, "/data/local/tmp/date2", X_OK) = -1 EACCES (Permission
denied)
write(2, "sh: /data/local/tmp/date2: can't"..., 60sh: /data/local/tmp/date2:
can't execute: Permission denied
) = 60
this patch fixes the issue:
```
diff --git a/src/exec.c b/src/exec.c
index 8330174..3f6d876 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -1279,8 +1279,8 @@ search_access(const char *fn, int mode)
struct stat sb;
if (stat(fn, ) < 0)
- /* file does not exist */
- return (ENOENT);
+ /* file may or may not exist: check errno */
+ return errno;
/* LINTED use of access */
if (access(fn, mode) < 0) {
/* file exists, but we can't access it */
```
i don't know if you want to elaborate further in the comment along the
lines of "...for example, an SELinux denial may mean that we get
EACCES here, or if the file doesn't exist and we're allowed to know
that, we'll get ENOENT".
result with patch:
$ sh -c /system/bin/vold
sh: /system/bin/vold: can't execute: Permission denied
To manage notifications about this bug go to:
https://bugs.launchpad.net/mksh/+bug/1817789/+subscriptions
[Bug 1817789] Re: misleading error message for SELinux denials
In the SELinux case that Elliott pointed to in the initial bug report,
mksh can also "see" the file (eg, stat() returns EACCES, indicating the
file exists but security policy disallows stat() operations). Yet "not
found" is emitted by mksh vs (the IMHO more correct) "Permission
denied". The mksh code assumes any stat() failure is due to the file not
existing vs other causes. Unfortunately, this error condition can only
be replicated using one of the available Linux Mandatory Access Control
systems such as SELinux, Smack, AppArmor, or Tomoyo.
Similarly, in the following scenerio, mksh can "see" the asdf command,
but still returns "not found", when the command clearly exists but is
malformed.
nnk@nnk0:/tmp$ mkdir d
nnk@nnk0:/tmp$ ln -s ../asdf d/asdf
nnk@nnk0:/tmp$ ln -s d/asdf asdf
nnk@nnk0:/tmp$ ls -la d/asdf asdf
lrwxrwxrwx 1 nnk nnk 6 Feb 27 09:27 asdf -> d/asdf
lrwxrwxrwx 1 nnk nnk 7 Feb 27 09:27 d/asdf -> ../asdf
nnk@nnk0:/tmp$ mksh -c /tmp/asdf
mksh: /tmp/asdf: not found
Bash provides a more accurate error message in this case:
nnk@nnk0:/tmp$ bash -c /tmp/asdf
bash: /tmp/asdf: Too many levels of symbolic links
This is particularly problematic for interactive shells, where the lack
of accurate error messages inhibits end user understanding of the error
conditions.
nnk@nnk0:/tmp$ mksh
$ /tmp/asdf
mksh: /tmp/asdf: not found
$ ls -la /tmp/asdf
lrwxrwxrwx 1 nnk nnk 6 Feb 27 09:27 /tmp/asdf -> d/asdf
$ mkdir d2
$ touch d2/a2
$ chmod 000 ./d2
$ /tmp/d2/a2
mksh: /tmp/d2/a2: not found
Can you elaborate on the statement that "passing through the errno may
introduce other problems"? What other problems are you concerned about?
The statement feels unactionable.
IMHO, changing "not found" to a more generic string, without other
changes, would not improve end user understandability.
Thank you for your continued dialog on this issue.
--
You received this bug notification because you are a member of mksh
Mailing List, which is subscribed to mksh.
Matching subscriptions: mkshlist-to-mksh-bugmail
https://bugs.launchpad.net/bugs/1817789
Title:
misleading error message for SELinux denials
Status in mksh:
Opinion
Bug description:
Given a stat(2) failure caused by an SELinux denial (rather than a
stat(2) success and an access(2) failure, as with a regular `chmod
a-x` failure), mksh reports "not found" rather than the more correct
"Permission denied".
Expected:
* Permission Denied error message
Actual:
$ sh -c /system/bin/vold
sh: /system/bin/vold: not found
"not found" error message.
here's the behind-the-scenes SELinux denial:
02-25 22:37:11.023 4571 4571 W sh : type=1400 audit(0.0:347):
avc: denied { getattr } for path="/system/bin/vold" dev="dm-0" ino=717
scontext=u:r:shell:s0 tcontext=u:object_r:vold_exec:s0 tclass=file
permissive=0
here's what strace says happened:
newfstatat(AT_FDCWD, "/system/bin/vold", 0x7ffcc3ef20, 0) = -1 EACCES
(Permission denied)
write(2, "/system/bin/sh: /system/bin/vold"..., 44/system/bin/sh:
/system/bin/vold: not found
) = 44
versus the normal `chmod a-x` case where stat succeeds but access
fails:
newfstatat(AT_FDCWD, "/data/local/tmp/date2", {st_mode=S_IFREG|0644,
st_size=482560, ...}, 0) = 0
faccessat(AT_FDCWD, "/data/local/tmp/date2", X_OK) = -1 EACCES (Permission
denied)
write(2, "sh: /data/local/tmp/date2: can't"..., 60sh: /data/local/tmp/date2:
can't execute: Permission denied
) = 60
this patch fixes the issue:
```
diff --git a/src/exec.c b/src/exec.c
index 8330174..3f6d876 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -1279,8 +1279,8 @@ search_access(const char *fn, int mode)
struct stat sb;
if (stat(fn, ) < 0)
- /* file does not exist */
- return (ENOENT);
+ /* file may or may not exist: check errno */
+ return errno;
/* LINTED use of access */
if (access(fn, mode) < 0) {
/* file exists, but we can't access it */
```
i don't know if you want to elaborate further in the comment along the
lines of "...for example, an SELinux denial may mean that we get
EACCES here, or if the file doesn't exist and we're allowed to know
that, we'll get ENOENT".
result with patch:
$ sh -c /system/bin/vold
sh: /system/bin/vold: can't execute: Permission denied
To manage notifications about this bug go to:
https://bugs.launchpad.net/mksh/+bug/1817789/+subscriptions
