From owner-freebsd-hackers@FreeBSD.ORG  Wed Jul 21 10:34:41 2010
Return-Path: <owner-freebsd-hackers@FreeBSD.ORG>
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 ED3BA1065676;
	Wed, 21 Jul 2010 10:34:41 +0000 (UTC)
	(envelope-from yanegomi@gmail.com)
Received: from mail-iw0-f182.google.com (mail-iw0-f182.google.com
	[209.85.214.182])
	by mx1.freebsd.org (Postfix) with ESMTP id A4E7E8FC12;
	Wed, 21 Jul 2010 10:34:41 +0000 (UTC)
Received: by iwn35 with SMTP id 35so8465736iwn.13
	for <multiple recipients>; Wed, 21 Jul 2010 03:34:41 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;
	h=domainkey-signature:mime-version:received:received:in-reply-to
	:references:date:message-id:subject:from:to:cc:content-type
	:content-transfer-encoding;
	bh=iiGO5+L2mjGmUIsd0GgnQK44tBX79u0TgzmdJXPe0TI=;
	b=UKsLGO2Axf3NJtnKaNmKygJFMbn5+VMfjMD3Lvo6DHElQbdNaBhCw+zW5s+E4szi7M
	5/3eSZJ2p1z3LiZlkAvp27n6dyAF9zLJ8XNTQplkfd3G0mwN71X1W0XKC6ru2F5/4JkS
	QTmqHVBaZdWxHcfAcUrTy87AlBNz1UVg7C9QI=
DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;
	h=mime-version:in-reply-to:references:date:message-id:subject:from:to
	:cc:content-type:content-transfer-encoding;
	b=dcScXkXK9byPpyGDeXkrMirq/vtYXKjzm5bLqlkieNgPNOLNKtEB1R6KGgwODvc1Be
	W9dGA42iHgCoHEB4rzEwCz5Nt8RoC/EXalagndc6YWKXlTLXS3j3edsutzhdBSqtJfoT
	I38SXQQB/fQ3788QRG0b5I0M6+MTpCfVR8u3o=
MIME-Version: 1.0
Received: by 10.231.14.194 with SMTP id h2mr8986308iba.67.1279708480674; Wed, 
	21 Jul 2010 03:34:40 -0700 (PDT)
Received: by 10.231.169.18 with HTTP; Wed, 21 Jul 2010 03:34:40 -0700 (PDT)
In-Reply-To: <20100721162044.N7348@delplex.bde.org>
References: <AANLkTilXPg03r3eMJQKUeFIDhabA634lYu5K03Xue-kE@mail.gmail.com>
	<20100721162044.N7348@delplex.bde.org>
Date: Wed, 21 Jul 2010 03:34:40 -0700
Message-ID: <AANLkTin8wsMnttQdqyMAqmYAPjFvfvQ5fbX-mGM9z3w8@mail.gmail.com>
From: Garrett Cooper <yanegomi@gmail.com>
To: Bruce Evans <brde@optusnet.com.au>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Cc: standards@freebsd.org, hackers@freebsd.org
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
	<freebsd-hackers.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-hackers>, 
	<mailto:freebsd-hackers-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-hackers>
List-Post: <mailto:freebsd-hackers@freebsd.org>
List-Help: <mailto:freebsd-hackers-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-hackers>,
	<mailto:freebsd-hackers-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Wed, 21 Jul 2010 10:34:42 -0000

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