From owner-freebsd-bugs@FreeBSD.ORG Tue Jul 8 13:30:05 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3C7F01065673 for ; Tue, 8 Jul 2008 13:30:05 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 28C7D8FC25 for ; Tue, 8 Jul 2008 13:30:05 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id m68DU4Ls055443 for ; Tue, 8 Jul 2008 13:30:04 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.2/8.14.1/Submit) id m68DU49p055442; Tue, 8 Jul 2008 13:30:04 GMT (envelope-from gnats) Date: Tue, 8 Jul 2008 13:30:04 GMT Message-Id: <200807081330.m68DU49p055442@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Jaakko Heinonen Cc: Subject: Re: kern/125009: access(2) grants root execute perms for non-executable files X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Jaakko Heinonen List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Jul 2008 13:30:05 -0000 The following reply was made to PR kern/125009; it has been noted by GNATS. From: Jaakko Heinonen To: clemens fischer , bug-followup@FreeBSD.org Cc: Subject: Re: kern/125009: access(2) grants root execute perms for non-executable files Date: Tue, 8 Jul 2008 16:29:15 +0300 On 2008-06-26, clemens fischer wrote: > this check works correctly for non-root, but any file accessible in > any way for root _passes_ the "(access(argv[0], X_OK) < 0)" check, a > subsequent execvp(3) fails. doing "git-init; date > fileA; git-commit > -a" as a non-privileged user works as expected. At first sight it may look like that it's not a bug. From FreeBSD access(2) manual page: 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. This is line with SUSv3 which states following: If any access permissions are checked, each shall be checked individually, as described in the Base Definitions volume of IEEE Std 1003.1-2001, Chapter 3, Definitions. If the process has appropriate privileges, an implementation may indicate success for X_OK even if none of the execute file permission bits are set. However it boils down to how one defines "appropriate privileges". execve(2) has a special check for root: a file must have at least one execute bit set, otherwise execve(2) returns EACCES even for root. Thus I think that it's reasonable to say that there aren't "appropriate privileges" for root unless at least one execute bit is set for a (regular) file. This patch adds a similar execute bit check that execve(2) has to access(2). %%% Index: sys/kern/vfs_syscalls.c =================================================================== --- sys/kern/vfs_syscalls.c (revision 180121) +++ sys/kern/vfs_syscalls.c (working copy) @@ -2037,6 +2037,7 @@ vn_access(vp, user_flags, cred, td) /* Flags == 0 means only check for existence. */ error = 0; if (user_flags) { + struct vattr vattr; flags = 0; if (user_flags & R_OK) flags |= VREAD; @@ -2051,6 +2052,17 @@ vn_access(vp, user_flags, cred, td) #endif if ((flags & VWRITE) == 0 || (error = vn_writechk(vp)) == 0) error = VOP_ACCESS(vp, flags, cred, td); + if (error) + return (error); + /* + * When checking execute permission insure that at least one + * execute bit is on (except for directories) - otherwise root + * will always succeed. + */ + if ((flags & VEXEC) && + ((error = VOP_GETATTR(vp, &vattr, cred, td)) == 0) && + (vattr.va_type != VDIR) && ((vattr.va_mode & 0111) == 0)) + error = EACCES; } return (error); } %%% However I think that it would be more logical to move that check down to vaccess() and remove the extra check from exec_check_permissions(). But there is a thing you have to note: vaccess() is not called when ACLs are used. Thus the check must be added to vaccess_acl_posix1e() too. Following patch should do it. %%% Index: sys/kern/kern_exec.c =================================================================== --- sys/kern/kern_exec.c (revision 180335) +++ sys/kern/kern_exec.c (working copy) @@ -1291,13 +1291,9 @@ exec_check_permissions(imgp) /* * 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. + * 2) Insure that the file is a regular file. */ if ((vp->v_mount->mnt_flag & MNT_NOEXEC) || - ((attr->va_mode & 0111) == 0) || (attr->va_type != VREG)) return (EACCES); Index: sys/kern/subr_acl_posix1e.c =================================================================== --- sys/kern/subr_acl_posix1e.c (revision 180335) +++ sys/kern/subr_acl_posix1e.c (working copy) @@ -85,8 +85,14 @@ vaccess_acl_posix1e(enum vtype type, uid PRIV_VFS_LOOKUP, 0)) priv_granted |= VEXEC; } else { + /* + * Insure that at least one execute bit is on - otherwise + * privileged user will always succeed, and we don't want to + * happen unless the file really is executable. + */ if ((acc_mode & VEXEC) && !priv_check_cred(cred, - PRIV_VFS_EXEC, 0)) + PRIV_VFS_EXEC, 0) && (acl_posix1e_acl_to_mode(acl) & + (S_IXUSR | S_IXGRP | S_IXOTH))) priv_granted |= VEXEC; } Index: sys/kern/vfs_subr.c =================================================================== --- sys/kern/vfs_subr.c (revision 180335) +++ sys/kern/vfs_subr.c (working copy) @@ -3517,8 +3517,14 @@ privcheck: !priv_check_cred(cred, PRIV_VFS_LOOKUP, 0)) priv_granted |= VEXEC; } else { + /* + * Insure that at least one execute bit is on - otherwise + * privileged user will always succeed, and we don't want to + * happen unless the file really is executable. + */ if ((acc_mode & VEXEC) && ((dac_granted & VEXEC) == 0) && - !priv_check_cred(cred, PRIV_VFS_EXEC, 0)) + !priv_check_cred(cred, PRIV_VFS_EXEC, 0) && + (file_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) priv_granted |= VEXEC; } %%% I also tested how some other operating systems behave: (access(2) X_OK call as root for a regular file which has no execute bits set) Mac OS X 10.4 returns EPERM (for users it returns EACCES and strangely it returns 0 (OK) for device files) Linux (recent Ubuntu kernel) returns EACCES NetBSD returns EACCES OpenBSD 4.3 returns 0 (same as FreeBSD) > but this isn't a git-problem anyway: i took > /usr/src/tools/regression/security/access/* and changed every check for > "access(path, R_OK)" into "access(path, X_OK)". as the test files in > this test are created modes 0400 and 0040, one would expect every check > to fail when using X_OK, but the output is: > > : /src/localcode/test/access > : 0 # ./testaccess > : Effective uid was used instead of real uid in access(). > : Effective gid was used instead of real gid in access(). > : 2 errors seen. I don't think that your change to the test and these errors are relevant to the actual problem. I didn't check thoroughly though. -- Jaakko