Date: Wed, 21 Jul 2010 03:34:40 -0700 From: Garrett Cooper <yanegomi@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: standards@freebsd.org, hackers@freebsd.org Subject: Re: Chasing down bugs with access(2) Message-ID: <AANLkTin8wsMnttQdqyMAqmYAPjFvfvQ5fbX-mGM9z3w8@mail.gmail.com> In-Reply-To: <20100721162044.N7348@delplex.bde.org> References: <AANLkTilXPg03r3eMJQKUeFIDhabA634lYu5K03Xue-kE@mail.gmail.com> <20100721162044.N7348@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jul 21, 2010 at 1:40 AM, Bruce Evans <brde@optusnet.com.au> wrote: > On Tue, 20 Jul 2010, Garrett Cooper wrote: ... > This seems wrong for directories. =A0It should say "... unless the file > is 'executable'". =A0'executable' means searchable for directories, and > the above shouldn't apply. =A0'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. =A0The 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. =A0The spec goes too far in the other > direction for the access function. Agreed (for all of the above). >> FreeBSD says: >> >> =A0 =A0Even if a process's real or effective user has appropriate privil= eges >> and >> =A0 =A0indicates success for X_OK, the file may not actually have execut= e per- >> =A0 =A0mission bits set. =A0Likewise for R_OK and W_OK. > > Perhaps it is time to fix this. =A0The part about X_OK never applied to a= ny > version of FreeBSD. =A0Perhaps it applied to the <body deleted> version o= f > execve() in Net/2 and 4.4BSD, but FreeBSD had to reimplement execve() and > it never had this bug. =A0But^2, the access() syscall and man page weren'= t > changed to match. =A0See 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. =A0Everone knows that root can rea= d > =A0and 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. =A0access() should tr= ack > =A0the capability, not the bits. =A0The bits used to map to the capabilit= y > =A0directly for non-root, but now with ACLs, MAC, etc. they don't even do > =A0that. > - 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(). =A0(The > =A0comment says that the code is always ifdefed out, but it now always > =A0unifdefed in.) =A0 I don't quite understand this code -- does it give > =A0all 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.. >> =A0 This results in: >> >> =A0 sh/test - ok-ish (a guy on bash-bugs challenged the fact that the >> syscall was buggy based on the details returned). >> =A0 bash - broken (builtin test(1) always returns true) >> =A0 csh =A0- not really ok (uses whacky stat-like detection; doesn't >> check for ACLs, or MAC info) >> =A0 perl - ok (uses eaccess(2) for our OS). >> =A0 python - broken (uses straight access(2), so os.access is broken). ... > -current also has a MAC check here. =A0I 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. > % =A0 =A0 =A0 /* > % =A0 =A0 =A0 =A0* 1) Check if file execution is disabled for the filesys= tem that > this > % =A0 =A0 =A0 =A0* =A0 =A0 =A0file resides on. > % =A0 =A0 =A0 =A0* 2) Insure that at least one execute bit is on - otherw= ise root > % =A0 =A0 =A0 =A0* =A0 =A0 =A0will always succeed, and we don't want to h= appen unless the > % =A0 =A0 =A0 =A0* =A0 =A0 =A0file really is executable. > % =A0 =A0 =A0 =A0* 3) Insure that the file is a regular file. > % =A0 =A0 =A0 =A0*/ > % =A0 =A0 =A0 if ((vnodep->v_mount->mnt_flag & MNT_NOEXEC) || > % =A0 =A0 =A0 =A0 =A0 ((attr->va_mode & 0111) =3D=3D 0) || > % =A0 =A0 =A0 =A0 =A0 (attr->va_type !=3D VREG)) { > % =A0 =A0 =A0 =A0 =A0 =A0 =A0 return (EACCES); > % =A0 =A0 =A0 } > > 0111 is an old spelling of the S_IX* bits. =A0We check these directly > since we know that VOP_ACCESS() is broken for root. =A0It 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). =A0The usual weaseli= ng > 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 syste= m. ... Yes. The usual warning about the `hinting' being done by *access(2) is fine= :). -Garrett
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTin8wsMnttQdqyMAqmYAPjFvfvQ5fbX-mGM9z3w8>