Date: Tue, 8 Jul 2008 13:30:04 GMT From: Jaakko Heinonen <jh@saunalahti.fi> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/125009: access(2) grants root execute perms for non-executable files Message-ID: <200807081330.m68DU49p055442@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/125009; it has been noted by GNATS.
From: Jaakko Heinonen <jh@saunalahti.fi>
To: clemens fischer <ino-news@spotteswoode.dnsalias.org>,
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200807081330.m68DU49p055442>