From owner-freebsd-hackers@FreeBSD.ORG Wed Jul 21 13:13:56 2010 Return-Path: Delivered-To: hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D07DE106566C; Wed, 21 Jul 2010 13:13:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id 4F69F8FC14; Wed, 21 Jul 2010 13:13:55 +0000 (UTC) Received: from c122-106-145-25.carlnfd1.nsw.optusnet.com.au (c122-106-145-25.carlnfd1.nsw.optusnet.com.au [122.106.145.25]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o6LDDqo5002550 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 21 Jul 2010 23:13:53 +1000 Date: Wed, 21 Jul 2010 23:13:52 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Garrett Cooper In-Reply-To: Message-ID: <20100721221551.B7582@delplex.bde.org> References: <20100721162044.N7348@delplex.bde.org> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-468481727-1279718032=:7582" X-Mailman-Approved-At: Wed, 21 Jul 2010 15:43:39 +0000 Cc: standards@FreeBSD.org, hackers@FreeBSD.org, Bruce Evans Subject: Re: Chasing down bugs with access(2) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Jul 2010 13:13:56 -0000 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-468481727-1279718032=:7582 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE 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(). =A0(Th= e >> =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.. 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() =3D check r/o ffs check quota (bogusly in the clause whose comment says that it checks =09for a r/o fs, deep in the case statement for that clause. This =09was readable when that clause was only 16 lines long, but now =09it has messy locking for quotas, and large comments about this, =09so it is 44 lines long, with the number of lines for locking =09up from 4 to 32) check immutability. Another bug in access()'s man page is that it =09doesn't mention either immutability or the errno EPERM that at =09least 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. > ... >> % =A0 =A0 =A0 /* >> % =A0 =A0 =A0 =A0* 1) Check if file execution is disabled for the filesy= stem 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 - other= wise root >> % =A0 =A0 =A0 =A0* =A0 =A0 =A0will always succeed, and we don't want to = happen 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 } 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. =A0We 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). =A0The usual weasel= ing >> 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 syst= em. 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 connectio= n with the nosuid mount option in a wrong man page: % Index: execve.2 % =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D % 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=0911 Jan 1998 21:43:38 -0000=091.11 % +++ execve.2=0927 Apr 1999 03:56:10 -0000=091.12 % @@ -31,5 +31,5 @@ % .\" % .\" @(#)execve.2=098.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. Sysca= ll % +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 --0-468481727-1279718032=:7582--