Chasing down bugs with access(2)

Wed Jul 21 10:34:42 UTC 2010

On Wed, Jul 21, 2010 at 1:40 AM, Bruce Evans <brde at optusnet.com.au> 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 <body deleted> 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/000095399/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 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.

...

Yes. The usual warning about the `hinting' being done by *access(2) is fine :).

-Garrett