6445680 Having write_acl allowed in an ACL doesn't give the ability to set the mode via chmod

6450672 chmod acl parse error gave poor help for incorrect delimeter (,) instead of (/)

4 files changed, 109 insertions, 69 deletions

diff --git a/usr/src/lib/libsec/common/acl_lex.l b/usr/src/lib/libsec/common/acl_lex.l
index ae633e2870..509e527e66 100644
--- a/usr/src/lib/libsec/common/acl_lex.l
+++ b/usr/src/lib/libsec/common/acl_lex.l
@@ -71,8 +71,7 @@ int yybufpos;
  */
 
 ID	[1-9][0-9]*
-LOGNAME [a-z0-9A-Z]+:
-BADLOGNAME [a-z0-9A-Z]+
+LOGNAME [\.a-z0-9A-Z\-_]+:
 PERM_STR [rRwWxpdDaAcCos-]+
 INHERIT_STR [fdinFS-]+
 
@@ -205,13 +204,6 @@ INHERIT_STR [fdinFS-]+
 				BEGIN PS;
 				return (IDNAME);
 			}
-<NS>{BADLOGNAME}	{
-				acl_error(dgettext(TEXT_DOMAIN,
-				    "Missing fields after "
-				    "user/group '%s'.\n"), yytext);
-				yylval.val = EACL_MISSING_FIELDS;
-				return (ERROR);
-			}
 <NS>"\n"		{
 				acl_error(dgettext(TEXT_DOMAIN,
 				    "Missing user/group name"
@@ -238,71 +230,71 @@ INHERIT_STR [fdinFS-]+
 				}
 				return (ERROR);
 			}
-<PS>read_data/[:/]	{
+<PS>read_data/[:/,]	{
 				yylval.val = ACE_READ_DATA;
 				return (ACE_PERM);
 			}
-<PS>list_directory/[:/] {
+<PS>list_directory/[:/,] {
 				yylval.val = ACE_LIST_DIRECTORY;
 			 	return (ACE_PERM);
 			}
-<PS>write_data/[:/]	{
+<PS>write_data/[:/,]	{
 				yylval.val = ACE_WRITE_DATA;
 				return (ACE_PERM);
 			}
-<PS>add_file/[:/]	{
+<PS>add_file/[:/,]	{
 				yylval.val = ACE_ADD_FILE;
 				return (ACE_PERM);
 			}
-<PS>append_data/[:/]	{
+<PS>append_data/[:/,]	{
 				yylval.val = ACE_APPEND_DATA;
 				return (ACE_PERM);
 			}
-<PS>add_subdirectory/[:/] {
+<PS>add_subdirectory/[:/,] {
 				yylval.val = ACE_ADD_SUBDIRECTORY;
 				return (ACE_PERM);
 			}
-<PS>read_xattr/[:/]	{
+<PS>read_xattr/[:/,]	{
 				yylval.val = ACE_READ_NAMED_ATTRS;
 				return (ACE_PERM);
 			}
-<PS>write_xattr/[:/]	{
+<PS>write_xattr/[:/,]	{
 				yylval.val = ACE_WRITE_NAMED_ATTRS;
 				return (ACE_PERM);
 			}
-<PS>execute/[:/]	{
+<PS>execute/[:/,]	{
 				yylval.val = ACE_EXECUTE;
 				return (ACE_PERM);
 			}
-<PS>delete_child/[:/]	{
+<PS>delete_child/[:/,]	{
 				yylval.val = ACE_DELETE_CHILD;
 				return (ACE_PERM);
 			}
-<PS>read_attributes/[:/] {
+<PS>read_attributes/[:/,] {
 				yylval.val = ACE_READ_ATTRIBUTES;
 				return (ACE_PERM);
 			}
-<PS>write_attributes/[:/] {
+<PS>write_attributes/[:/,] {
 				yylval.val = ACE_WRITE_ATTRIBUTES;
 			 	return (ACE_PERM);
 			}
-<PS>delete/[:/]		{
+<PS>delete/[:/,]		{
 				yylval.val = ACE_DELETE;
 				return (ACE_PERM);
 			}
-<PS>read_acl/[:/]	{
+<PS>read_acl/[:/,]	{
 				yylval.val = ACE_READ_ACL;
 				return (ACE_PERM);
 			}
-<PS>write_acl/[:/]	{
+<PS>write_acl/[:/,]	{
 				yylval.val = ACE_WRITE_ACL;
 				return (ACE_PERM);
 			}
-<PS>write_owner/[:/]	{
+<PS>write_owner/[:/,]	{
 				yylval.val = ACE_WRITE_OWNER;
 				return (ACE_PERM);
 			}
-<PS>synchronize/[:/]	{
+<PS>synchronize/[:/,]	{
 				yylval.val = ACE_SYNCHRONIZE;
 				return (ACE_PERM);
 			}
@@ -354,6 +346,16 @@ INHERIT_STR [fdinFS-]+
 				yylval.val = EACL_MISSING_FIELDS;
 				return (ERROR);
 			}
+<PS>","			{
+				acl_error(
+				    dgettext(TEXT_DOMAIN,
+				    "The ',' is not a valid permission field "
+				    "separator.\nThe comma is used to separate "
+				    "access control entries.\nSee acl(5) for "
+				    "examples of specifying ACL entries.\n"));
+				yylval.val = EACL_PERM_MASK_ERROR;
+				return (ERROR);
+			}
 <PS>. 			{
 				if (grab_string("/:,\n") != 0) {
 					acl_error(dgettext(TEXT_DOMAIN,
@@ -464,19 +466,19 @@ INHERIT_STR [fdinFS-]+
 				yylval.val = ACE_ACCESS_DENIED_ACE_TYPE;
 				return (ACCESS_TYPE);
 			}
-<AIS>file_inherit/[:/] {
+<AIS>file_inherit/[:/,] {
 				yylval.val = ACE_FILE_INHERIT_ACE;
 				return (ACE_INHERIT);
 			}
-<AIS>dir_inherit/[:/]	{
+<AIS>dir_inherit/[:/,]	{
 				yylval.val = ACE_DIRECTORY_INHERIT_ACE;
 				return (ACE_INHERIT);
 			}
-<AIS>no_propagate/[/:]	{
+<AIS>no_propagate/[/:,]	{
 				yylval.val = ACE_NO_PROPAGATE_INHERIT_ACE;
 				return (ACE_INHERIT);
 			}
-<AIS>inherit_only/[/:]	{
+<AIS>inherit_only/[/:,]	{
 				yylval.val = ACE_INHERIT_ONLY_ACE;
 				return (ACE_INHERIT);
 			}
@@ -510,6 +512,16 @@ INHERIT_STR [fdinFS-]+
 				yylval.val = EACL_UNKNOWN_DATA;
 				return (ERROR);
 			}
+<AIS>","		{
+				acl_error(
+				    dgettext(TEXT_DOMAIN,
+				    "The ',' is not a valid inheritance field "
+				    "separator.\nThe comma is used to separate "
+				    "access control entries.\nSee acl(5) for "
+				    "examples of specifying ACL entries.\n"));
+				yylval.val = EACL_INVALID_ACCESS_TYPE;
+				return (ERROR);
+			}
 <AIS>.			{
 				if (yytext[0] != '\n' && yytext[0] != '\0') {
 					if (grab_string(":,\n") != 0) {
diff --git a/usr/src/uts/common/fs/zfs/zfs_vnops.c b/usr/src/uts/common/fs/zfs/zfs_vnops.c
index 39cafd7329..6404717083 100644
--- a/usr/src/uts/common/fs/zfs/zfs_vnops.c
+++ b/usr/src/uts/common/fs/zfs/zfs_vnops.c
@@ -1929,7 +1929,7 @@ zfs_setattr(vnode_t *vp, vattr_t *vap, int flags, cred_t *cr,
 	vattr_t		oldva;
 	uint_t		mask = vap->va_mask;
 	uint_t		saved_mask;
-	int		trim_mask = FALSE;
+	int		trim_mask = 0;
 	uint64_t	new_mode;
 	znode_t		*attrzp;
 	int		need_policy = FALSE;
@@ -2023,8 +2023,7 @@ top:
 				 * Remove setuid/setgid for non-privileged users
 				 */
 				secpolicy_setid_clear(vap, cr);
-				trim_mask = TRUE;
-				saved_mask = vap->va_mask;
+				trim_mask = (mask & (AT_UID|AT_GID));
 			} else {
 				need_policy =  TRUE;
 			}
@@ -2033,25 +2032,40 @@ top:
 		}
 	}
 
-	if (mask & AT_MODE)
-		need_policy = TRUE;
+	mutex_enter(&zp->z_lock);
+	oldva.va_mode = pzp->zp_mode;
+	oldva.va_uid = zp->z_phys->zp_uid;
+	oldva.va_gid = zp->z_phys->zp_gid;
+	mutex_exit(&zp->z_lock);
 
-	if (need_policy) {
-		mutex_enter(&zp->z_lock);
-		oldva.va_mode = pzp->zp_mode;
-		oldva.va_uid = zp->z_phys->zp_uid;
-		oldva.va_gid = zp->z_phys->zp_gid;
-		mutex_exit(&zp->z_lock);
+	if (mask & AT_MODE) {
+		if (zfs_zaccess_v4_perm(zp, ACE_WRITE_ACL, cr) == 0) {
+			err = secpolicy_setid_setsticky_clear(vp, vap,
+			    &oldva, cr);
+			if (err) {
+				ZFS_EXIT(zfsvfs);
+				return (err);
+			}
+			trim_mask |= AT_MODE;
+		} else {
+			need_policy = TRUE;
+		}
+	}
 
+	if (need_policy) {
 		/*
 		 * If trim_mask is set then take ownership
-		 * has been granted.  In that case remove
-		 * UID|GID from mask so that
+		 * has been granted or write_acl is present and user
+		 * has the ability to modify mode.  In that case remove
+		 * UID|GID and or MODE from mask so that
 		 * secpolicy_vnode_setattr() doesn't revoke it.
 		 */
-		if (trim_mask)
-			vap->va_mask &= ~(AT_UID|AT_GID);
 
+		if (trim_mask) {
+			saved_mask = vap->va_mask;
+			vap->va_mask &= ~trim_mask;
+
+		}
 		err = secpolicy_vnode_setattr(cr, vp, vap, &oldva, flags,
 		    (int (*)(void *, int, cred_t *))zfs_zaccess_rwx, zp);
 		if (err) {
@@ -2060,7 +2074,7 @@ top:
 		}
 
 		if (trim_mask)
-			vap->va_mask |= (saved_mask & (AT_UID|AT_GID));
+			vap->va_mask |= saved_mask;
 	}
 
 	/*
diff --git a/usr/src/uts/common/os/policy.c b/usr/src/uts/common/os/policy.c
index 2f804b1d0e..59795156bf 100644
--- a/usr/src/uts/common/os/policy.c
+++ b/usr/src/uts/common/os/policy.c
@@ -925,6 +925,39 @@ secpolicy_setid_clear(vattr_t *vap, cred_t *cr)
 	}
 }
 
+int
+secpolicy_setid_setsticky_clear(vnode_t *vp, vattr_t *vap, const vattr_t *ovap,
+    cred_t *cr)
+{
+	int error;
+
+	if ((vap->va_mode & S_ISUID) != 0 &&
+	    (error = secpolicy_vnode_setid_modify(cr,
+	    ovap->va_uid)) != 0) {
+		return (error);
+	}
+
+	/*
+	 * Check privilege if attempting to set the
+	 * sticky bit on a non-directory.
+	 */
+	if (vp->v_type != VDIR && (vap->va_mode & S_ISVTX) != 0 &&
+	    secpolicy_vnode_stky_modify(cr) != 0) {
+	    vap->va_mode &= ~S_ISVTX;
+	}
+
+	/*
+	 * Check for privilege if attempting to set the
+	 * group-id bit.
+	 */
+	if ((vap->va_mode & S_ISGID) != 0 &&
+	    secpolicy_vnode_setids_setgids(cr, ovap->va_gid) != 0) {
+	    vap->va_mode &= ~S_ISGID;
+	}
+
+	return (0);
+}
+
 /*
  * This function checks the policy decisions surrounding the
  * vop setattr call.
@@ -986,30 +1019,9 @@ secpolicy_vnode_setattr(cred_t *cr, struct vnode *vp, struct vattr *vap,
 		if ((error = secpolicy_vnode_setdac(cr, ovap->va_uid)) != 0)
 			goto out;
 
-		if ((vap->va_mode & S_ISUID) != 0 &&
-		    (error = secpolicy_vnode_setid_modify(cr,
-							ovap->va_uid)) != 0) {
+		if ((error = secpolicy_setid_setsticky_clear(vp, vap,
+		    ovap, cr)) != 0)
 			goto out;
-		}
-
-		/*
-		 * Check privilege if attempting to set the
-		 * sticky bit on a non-directory.
-		 */
-		if (vp->v_type != VDIR && (vap->va_mode & S_ISVTX) != 0 &&
-		    secpolicy_vnode_stky_modify(cr) != 0) {
-			vap->va_mode &= ~S_ISVTX;
-		}
-
-		/*
-		 * Check for privilege if attempting to set the
-		 * group-id bit.
-		 */
-		if ((vap->va_mode & S_ISGID) != 0 &&
-		    secpolicy_vnode_setids_setgids(cr, ovap->va_gid) != 0) {
-			vap->va_mode &= ~S_ISGID;
-		}
-
 	} else
 		vap->va_mode = ovap->va_mode;
 
diff --git a/usr/src/uts/common/sys/policy.h b/usr/src/uts/common/sys/policy.h
index 6492501176..1b86a5507f 100644
--- a/usr/src/uts/common/sys/policy.h
+++ b/usr/src/uts/common/sys/policy.h
@@ -146,6 +146,8 @@ int secpolicy_vnode_stky_modify(const cred_t *);
 int secpolicy_zinject(const cred_t *);
 int secpolicy_zfs(const cred_t *);
 void secpolicy_setid_clear(vattr_t *, cred_t *);
+int secpolicy_setid_setsticky_clear(vnode_t *, vattr_t *,
+    const vattr_t *, cred_t *);
 
 int secpolicy_basic_exec(const cred_t *);
 int secpolicy_basic_fork(const cred_t *);