kern/125009: access(2) grants root execute perms for non-executable files

Tue Jul 8 13:30:05 UTC 2008

The following reply was made to PR kern/125009; it has been noted by GNATS.

From: Jaakko Heinonen <jh at saunalahti.fi>
To: clemens fischer <ino-news at spotteswoode.dnsalias.org>,
	bug-followup at 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