Chasing down bugs with access(2)

[Bruce Evans]

On Wed, 21 Jul 2010, Garrett Cooper wrote:

> On Wed, Jul 21, 2010 at 1:40 AM, Bruce Evans <brde at optusnet.com.au> 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 -0000	1.11
% +++ execve.2	27 Apr 1999 03:56:10 -0000	1.12
% @@ -31,5 +31,5 @@
%  .\"
%  .\"     @(#)execve.2	8.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(), and you should have to read all the section 5 or 7 man
pages for file systems to find them all (unless access() documents them
all), but these man pages mostly don't exist and/or have few details,
so you would have to read all section 8 man pages for mount utilities.

Bruce