Re: Chasing down bugs with access(2)
Hi, On 2010-07-20, Garrett Cooper wrote: > I ran into an issue last night where apparently several apps make > faulty assumptions w.r.t. whether or not access(2) returns functional > data when running as a superuser. > New implementations are discouraged from returning X_OK unless at > least one execution permission bit is set. See PR kern/125009 (http://www.freebsd.org/cgi/query-pr.cgi?pr=125009). Here is the latest version of the vaccess*() patch which also changes vaccess_acl_nfs4(): http://people.freebsd.org/~jh/patches/vaccess-VEXEC.diff The patch is not a complete fix however. Not all file systems use vaccess*() for VEXEC in their VOP_ACCESS() (ZFS confirmed). Thus the patch doesn't work with ZFS. -- Jaakko ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Chasing down bugs with access(2)
On Wed, Jul 21, 2010 at 1:40 AM, Bruce Evans wrote:
> On Tue, 20 Jul 2010, Garrett Cooper wrote:
...
> This seems wrong for directories. It should say "... unless the file
> is 'executable'". 'executable' means searchable for directories, and
> the above shouldn't apply. 'executable' actually means executable for
> regular files, and the above should only apply indirectly: it is
> executability that should be required to have an X perm bit set, and
> then access() should just track the capability. The usual weaseling
> with "appropriate privilege" allows the X perm bits to have any control
> on executablity including none, and at least the old POSIX spec doesn't
> get in the way of this, since it doesn't mention the X perm bits in
> connection with the exec functions. The spec goes too far in the other
> direction for the access function.
Agreed (for all of the above).
>> FreeBSD says:
>>
>> Even if a process's real or effective user has appropriate privileges
>> and
>> indicates success for X_OK, the file may not actually have execute per-
>> mission bits set. Likewise for R_OK and W_OK.
>
> Perhaps it is time to fix this. The part about X_OK never applied to any
> version of FreeBSD. Perhaps it applied to the version of
> execve() in Net/2 and 4.4BSD, but FreeBSD had to reimplement execve() and
> it never had this bug. But^2, the access() syscall and man page weren't
> changed to match. See the end of this reply for more details on execve().
> See the next paragraph about more bugs in the above paragraph.
>
> Other bugs:
> - R_OK and W_OK are far from likewise. Everone knows that root can read
> and write any file.
Yes. Thankfully Linux also agrees on this point (I say this, because
portability between Linux and BSD helps ease porting pains).
> - The permission bits are relatively uninteresting. access() should track
> the capability, not the bits. The bits used to map to the capability
> directly for non-root, but now with ACLs, MAC, etc. they don't even do
> that.
> - access(2) has no mention of ACLs, MAC, etc.
No, sadly it doesn't (and of course POSIX leaves that open in the File
permissions section, for good reason:
http://www.opengroup.org/onlinepubs/95399/basedefs/xbd_chap04.html#tag_04_04
). That's the one thing that one of the folks on the bash list brought
up that was a valid argument for using access(2), eaccess(2), or
faccessat(2) vs stat(2). If you use a straight stat(2) call to
determine whether or not a file is executable, the `hint' could be
completely bogus as the file itself could be non-executable when the
ACL or MAC denies the capability, whereas many implementations of
access(2) support this additional permissions checking.
> - See a recent PR about unifdefed CAPABILITIES code in vaccess(). (The
> comment says that the code is always ifdefed out, but it now always
> unifdefed in.) I don't quite understand this code -- does it give
> all of ACLs, MAC and etc. at this level?
Interestingly standard permissions bypass ACLs/MAC if standard
permissions on the file/directory allow the requested permissions to
succeed; note the return (0) vs the priv_check_cred calls -- this is
where the the ACL/MAC for the inode is checked. This seems backwards,
but I could be missing something..
>> This results in:
>>
>> sh/test - ok-ish (a guy on bash-bugs challenged the fact that the
>> syscall was buggy based on the details returned).
>> bash - broken (builtin test(1) always returns true)
>> csh - not really ok (uses whacky stat-like detection; doesn't
>> check for ACLs, or MAC info)
>> perl - ok (uses eaccess(2) for our OS).
>> python - broken (uses straight access(2), so os.access is broken).
...
> -current also has a MAC check here. I can't see how vaccess(9) does an
> equivalent check, or if it does, but if it did then we wouldn't need a
> special MAC check here.
Agreed.
> % /*
> % * 1) Check if file execution is disabled for the filesystem that
> this
> % * file resides on.
> % * 2) Insure that at least one execute bit is on - otherwise root
> % * will always succeed, and we don't want to happen unless the
> % * file really is executable.
> % * 3) Insure that the file is a regular file.
> % */
> % if ((vnodep->v_mount->mnt_flag & MNT_NOEXEC) ||
> % ((attr->va_mode & 0111) == 0) ||
> % (attr->va_type != VREG)) {
> % return (EACCES);
> % }
>
> 0111 is an old spelling of the S_IX* bits. We check these directly
> since we know that VOP_ACCESS() is broken for root. It is also good
> to avoid calling VOP_ACCESS() first, since VOP_ACCESS() would record
> our use of suser() privilege when in fact we won't use it.
>
> Yet 2 more bugs: not just point 2, but points 1 and 3 in the above
> comment are undocumented in execve(2) and access(2). The usual weaseling
> with "appropriate privilege" should allow these too, but (as I forgo
Re: Chasing down bugs with access(2)
On Wed, 21 Jul 2010, Jaakko Heinonen wrote: On 2010-07-20, Garrett Cooper wrote: I ran into an issue last night where apparently several apps make faulty assumptions w.r.t. whether or not access(2) returns functional data when running as a superuser. New implementations are discouraged from returning X_OK unless at least one execution permission bit is set. See PR kern/125009 (http://www.freebsd.org/cgi/query-pr.cgi?pr=125009). Here is the latest version of the vaccess*() patch which also changes vaccess_acl_nfs4(): http://people.freebsd.org/~jh/patches/vaccess-VEXEC.diff The patch is not a complete fix however. Not all file systems use vaccess*() for VEXEC in their VOP_ACCESS() (ZFS confirmed). Thus the patch doesn't work with ZFS. I looked at the patches in the PR. It seems reasonable to require an X but for VEXEC for all file types except directories, like I think the vaccess() version of your patch does. Keeping the existing behaviour for directories seems necessary. E.g., suppose a user changes all his files and directories to mode 000. It should still be possible for root to search, not to mention back up, all those files and directories, without clobbering any of their metadata (including atimes, but those are a different problem). Bruce ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Chasing down bugs with access(2)
On Tue, 20 Jul 2010, Garrett Cooper wrote:
Hi Hackers,
I ran into an issue last night where apparently several apps make
faulty assumptions w.r.t. whether or not access(2) returns functional
data when running as a superuser.
POSIX says:
In early proposals, some inadequacies in the access() function led
to the creation of an eaccess() function because:
1. Historical implementations of access() do not test file
access correctly when the process' real user ID is superuser. In
particular, they always return zero when testing execute permissions
without regard to whether the file is executable.
2. The superuser has complete access to all files on a system.
As a consequence, programs started by the superuser and switched to
the effective user ID with lesser privileges cannot use access() to
test their file access permissions.
However, the historical model of eaccess() does not resolve
problem (1), so this volume of IEEE Std 1003.1-2001 now allows
access() to behave in the desired way because several implementations
have corrected the problem. It was also argued that problem (2) is
more easily solved by using open(), chdir(), or one of the exec
functions as appropriate and responding to the error, rather than
creating a new function that would not be as reliable. Therefore,
eaccess() is not included in this volume of IEEE Std 1003.1-2001.
The sentence concerning appropriate privileges and execute
permission bits reflects the two possibilities implemented by
historical implementations when checking superuser access for X_OK.
New implementations are discouraged from returning X_OK unless at
least one execution permission bit is set.
This seems wrong for directories. It should say "... unless the file
is 'executable'". 'executable' means searchable for directories, and
the above shouldn't apply. 'executable' actually means executable for
regular files, and the above should only apply indirectly: it is
executability that should be required to have an X perm bit set, and
then access() should just track the capability. The usual weaseling
with "appropriate privilege" allows the X perm bits to have any control
on executablity including none, and at least the old POSIX spec doesn't
get in the way of this, since it doesn't mention the X perm bits in
connection with the exec functions. The spec goes too far in the other
direction for the access function.
FreeBSD says:
Even if a process's real or effective user has appropriate privileges and
indicates success for X_OK, the file may not actually have execute per-
mission bits set. Likewise for R_OK and W_OK.
Perhaps it is time to fix this. The part about X_OK never applied to any
version of FreeBSD. Perhaps it applied to the version of
execve() in Net/2 and 4.4BSD, but FreeBSD had to reimplement execve() and
it never had this bug. But^2, the access() syscall and man page weren't
changed to match. See the end of this reply for more details on execve().
See the next paragraph about more bugs in the above paragraph.
Other bugs:
- R_OK and W_OK are far from likewise. Everone knows that root can read
and write any file.
- The permission bits are relatively uninteresting. access() should track
the capability, not the bits. The bits used to map to the capability
directly for non-root, but now with ACLs, MAC, etc. they don't even do
that.
- access(2) has no mention of ACLs, MAC, etc.
- See a recent PR about unifdefed CAPABILITIES code in vaccess(). (The
comment says that the code is always ifdefed out, but it now always
unifdefed in.) I don't quite understand this code -- does it give
all of ACLs, MAC and etc. at this level?
This results in:
sh/test - ok-ish (a guy on bash-bugs challenged the fact that the
syscall was buggy based on the details returned).
bash - broken (builtin test(1) always returns true)
csh - not really ok (uses whacky stat-like detection; doesn't
check for ACLs, or MAC info)
perl - ok (uses eaccess(2) for our OS).
python - broken (uses straight access(2), so os.access is broken).
So now the question is how to fix this? Linux reports the correct
mode for the file (when operating as superuser or not), and there's a
lot of code outside of *BSD that builds upon that assumption because
stat(2) doesn't test for permissions via POSIX ACLs, MAC, etc.
I tried munging through the code to determine where VOP_ACCESS was
coming from but I got lost. Where should I look for this?
Mostly it is in vaccess(9). But execve() mostly doesn't use VOP_ACCESS().
Here is the FreeBSD-1 version, which is a bit shorter and thus easier to
understand than the current version, and shows that FreeBSD has always
required 1 exec bit for execve():
% /*
% * Check permissions of file to execute.
% *Return 0 for success or error code on failure.
% */
% int
% exec_check_permissions(iparams)
% struct image_params *iparams;
% {
% struct proc *p = iparams->proc;
%
Re: Chasing down bugs with access(2)
On Wed, 21 Jul 2010, Garrett Cooper wrote:
On Wed, Jul 21, 2010 at 1:40 AM, Bruce Evans wrote:
- See a recent PR about unifdefed CAPABILITIES code in vaccess(). ?(The
?comment says that the code is always ifdefed out, but it now always
?unifdefed in.) ? I don't quite understand this code -- does it give
?all of ACLs, MAC and etc. at this level?
Interestingly standard permissions bypass ACLs/MAC if standard
permissions on the file/directory allow the requested permissions to
succeed; note the return (0) vs the priv_check_cred calls -- this is
where the the ACL/MAC for the inode is checked. This seems backwards,
but I could be missing something..
I was wrong to say that vaccess() does most of the checking. This is only
correct if there are no ACLs, etc. Otherwise, e.g., for ffs, the layering
is more like VOP_ACCESS() -> ufs_access() =
check r/o ffs
check quota (bogusly in the clause whose comment says that it checks
for a r/o fs, deep in the case statement for that clause. This
was readable when that clause was only 16 lines long, but now
it has messy locking for quotas, and large comments about this,
so it is 44 lines long, with the number of lines for locking
up from 4 to 32)
check immutability. Another bug in access()'s man page is that it
doesn't mention either immutability or the errno EPERM that at
least ufs_access() returns for it.
check acls
call vaccess(). The MAC checks seem to be at the end of this and
are unrelated to acls. But for exec_check_permissions(), the
MAC checks are first.
...
% ? ? ? /*
% ? ? ? ?* 1) Check if file execution is disabled for the filesystem that
this
% ? ? ? ?* ? ? ?file resides on.
% ? ? ? ?* 2) Insure that at least one execute bit is on - otherwise root
% ? ? ? ?* ? ? ?will always succeed, and we don't want to happen unless the
% ? ? ? ?* ? ? ?file really is executable.
% ? ? ? ?* 3) Insure that the file is a regular file.
% ? ? ? ?*/
% ? ? ? if ((vnodep->v_mount->mnt_flag & MNT_NOEXEC) ||
% ? ? ? ? ? ((attr->va_mode & 0111) == 0) ||
% ? ? ? ? ? (attr->va_type != VREG)) {
% ? ? ? ? ? ? ? return (EACCES);
% ? ? ? }
Your mail program seems to be responsible for making the above unreadable
by changing tabs to hard \xa0's (which are displayed as tabs by my mail
client(s?), but soft \xa0's followed by a space by my editor.
0111 is an old spelling of the S_IX* bits. ?We check these directly
Maybe the changes weren't of tabs. In this paragraphs, sentence breaks of
2 spaces got changed to 1 space followed by a hard \xa0.
Yet 2 more bugs: not just point 2, but points 1 and 3 in the above
comment are undocumented in execve(2) and access(2). ?The usual weaseling
with "appropriate privilege" should allow these too, but (as I forgot
to mention above) I think "appropriate privilege" is supposed to be
documented somewhere, so the man pages are still missing details.
Agreed on the former statement, and I understand the reasoning for the
latter statement, but at least for 1., this is a feature of mount(2)
(of course):
MNT_NOEXEC Do not allow files to be executed from the file system.
How is a newbie supposed to know to look in mount(2) to find extra failure
cases? execve(2) even cross-references mount(8), but this was in connection
with the nosuid mount option in a wrong man page:
% Index: execve.2
% ===
% RCS file: /home/ncvs/src/lib/libc/sys/execve.2,v
% retrieving revision 1.11
% retrieving revision 1.12
% diff -u -2 -r1.11 -r1.12
% --- execve.2 11 Jan 1998 21:43:38 - 1.11
% +++ execve.2 27 Apr 1999 03:56:10 - 1.12
% @@ -31,5 +31,5 @@
% .\"
% .\" @(#)execve.28.5 (Berkeley) 6/1/94
% -.\" $Id$
% +.\" $Id: execve.2,v 1.11 1998/01/11 21:43:38 alex Exp $
% .\"
% .Dd June 1, 1994
% @@ -144,4 +144,9 @@
% .ne 1i
% .Pp
% +The set-ID bits are not honored if the respective file system has the
% +.Ar nosuid
% +option enabled or if the new process file is an interpreter file. Syscall
% +tracing is disabled if effective IDs are changed.
% +.Pp
% The new process also inherits the following attributes from
% the calling process:
% @@ -274,4 +279,6 @@
% .Xr exit 3 ,
% .Xr sysctl 3 ,
% +.Xr mount 1 ,
This dangling pointer was fixed to mount(8) in the next commit. But it
should have been to mount(2) for MNT_NOSUID.
% +.Xr ktrace 1 ,
% .Xr environ 7
% .Sh HISTORY
I strongly dislike general references to man pages for 1 detail. There
should be an Xr where each of the nosuid and tracing details is described
and none at the end.
There are actually many details of interest here, but how is a newbie
going to notice this when the committers didn't? Details of interest
are at least:
- MNT_RDONLY (related to EROFS error)
- MNT_NOSUID
- MNT_NOEXEC
- MNT_ACLS (new)
- MNT_QUOTA
Better yet, nmount() allows any number of new mount options that might
affect access(),
