Chasing down bugs with access(2)

[Jaakko Heinonen]

On 2010-07-21, Bruce Evans wrote:
> > See PR kern/125009 (http://www.freebsd.org/cgi/query-pr.cgi?pr=125009).
> 
> I looked at the patches in the PR.  It seems reasonable to require an X
> but for VEXEC for all file types except directories, like I think the
> vaccess() version of your patch does.

Thanks for looking at. Both patches require it for non-directories only.

I have updated the vaccess*() version of the patch. It now preserves the
check in exec_check_permissions() to avoid causing regressions for file
systems which are not using vaccess*() functions.

%%%
Index: sys/kern/kern_exec.c
===================================================================
--- sys/kern/kern_exec.c	(revision 210492)
+++ sys/kern/kern_exec.c	(working copy)
@@ -1328,13 +1328,13 @@ 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
+	 * 2) Ensure 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.
+	 * 3) Ensure that the file is a regular file.
 	 */
 	if ((vp->v_mount->mnt_flag & MNT_NOEXEC) ||
-	    ((attr->va_mode & 0111) == 0) ||
+	    (attr->va_mode & (S_IXUSR | S_IXGRP | S_IXOTH)) == 0 ||
 	    (attr->va_type != VREG))
 		return (EACCES);
 
Index: sys/kern/vfs_subr.c
===================================================================
--- sys/kern/vfs_subr.c	(revision 210492)
+++ sys/kern/vfs_subr.c	(working copy)
@@ -3600,8 +3600,14 @@ privcheck:
 		    !priv_check_cred(cred, PRIV_VFS_LOOKUP, 0))
 			priv_granted |= VEXEC;
 	} else {
+		/*
+		 * Ensure 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 ((accmode & 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)) != 0)
 			priv_granted |= VEXEC;
 	}
 
Index: sys/kern/subr_acl_posix1e.c
===================================================================
--- sys/kern/subr_acl_posix1e.c	(revision 210492)
+++ sys/kern/subr_acl_posix1e.c	(working copy)
@@ -90,8 +90,14 @@ vaccess_acl_posix1e(enum vtype type, uid
 		     PRIV_VFS_LOOKUP, 0))
 			priv_granted |= VEXEC;
 	} else {
+		/*
+		 * Ensure 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 ((accmode & 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)) != 0)
 			priv_granted |= VEXEC;
 	}
 
Index: sys/kern/subr_acl_nfs4.c
===================================================================
--- sys/kern/subr_acl_nfs4.c	(revision 210492)
+++ sys/kern/subr_acl_nfs4.c	(working copy)
@@ -162,6 +162,7 @@ vaccess_acl_nfs4(enum vtype type, uid_t 
 	accmode_t priv_granted = 0;
 	int denied, explicitly_denied, access_mask, is_directory,
 	    must_be_owner = 0;
+	mode_t file_mode = 0;
 
 	KASSERT((accmode & ~(VEXEC | VWRITE | VREAD | VADMIN | VAPPEND |
 	    VEXPLICIT_DENY | VREAD_NAMED_ATTRS | VWRITE_NAMED_ATTRS |
@@ -236,8 +237,15 @@ vaccess_acl_nfs4(enum vtype type, uid_t 
 		    PRIV_VFS_LOOKUP, 0))
 			priv_granted |= VEXEC;
 	} else {
+		/*
+		 * Ensure 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.
+		 */
+		acl_nfs4_sync_mode_from_acl(&file_mode, aclp);
 		if ((accmode & VEXEC) && !priv_check_cred(cred,
-		    PRIV_VFS_EXEC, 0))
+		    PRIV_VFS_EXEC, 0) && (file_mode &
+		    (S_IXUSR | S_IXGRP | S_IXOTH)) != 0)
 			priv_granted |= VEXEC;
 	}
 
Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
===================================================================
--- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	(revision 210492)
+++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	(working copy)
@@ -4193,6 +4193,9 @@ zfs_freebsd_access(ap)
 		struct thread *a_td;
 	} */ *ap;
 {
+	vnode_t *vp = ap->a_vp;
+	znode_t *zp = VTOZ(vp);
+	znode_phys_t *zphys = zp->z_phys;
 	accmode_t accmode;
 	int error = 0;
 
@@ -4209,16 +4212,20 @@ zfs_freebsd_access(ap)
 	if (error == 0) {
 		accmode = ap->a_accmode & ~(VREAD|VWRITE|VEXEC|VAPPEND);
 		if (accmode != 0) {
-			vnode_t *vp = ap->a_vp;
-			znode_t *zp = VTOZ(vp);
-			znode_phys_t *zphys = zp->z_phys;
-
 			error = vaccess(vp->v_type, zphys->zp_mode,
 			    zphys->zp_uid, zphys->zp_gid, accmode, ap->a_cred,
 			    NULL);
 		}
 	}
 
+	/*
+	 * For VEXEC, ensure that at least one execute bit is set for
+	 * non-directories.
+	 */
+	if (error == 0 && (ap->a_accmode & VEXEC) != 0 && vp->v_type != VDIR &&
+	    (zphys->zp_mode & (S_IXUSR | S_IXGRP | S_IXOTH)) == 0)
+		error = EACCES;
+
 	return (error);
 }
 
%%%

-- 
Jaakko