In my first reading of POSIX.1e, I misinterpreted handling of the

ACL_USER_OBJ and ACL_GROUP_OBJ fields, believing that modification of the
access ACL could be used by privileged processes to change file/directory
ownership.  In fact, this is incorrect; ACL_*_OBJ (+ ACL_MASK and
ACL_OTHER) should have undefined ae_id fields; this commit attempts
to correct that misunderstanding.

o Modify arguments to vaccess_acl_posix1e() to accept the uid and gid
  associated with the vnode, as those can no longer be extracted from
  the ACL passed as an argument.  Perform all comparisons against
  the passed arguments.  This actually has the effect of simplifying
  a number of components of this call, as well as reducing the indent
  level, but now seperates handling of ACL_GROUP_OBJ from ACL_GROUP.

o Modify acl_posix1e_check() to return EINVAL if the ae_id field of
  any of the ACL_{USER_OBJ,GROUP_OBJ,MASK,OTHER} entries is a value
  other than ACL_UNDEFINED_ID.  As a temporary work-around to allow
  clean upgrades, set the ae_id field to ACL_UNDEFINED_ID before
  each check so that this cannot cause a failure in the short term
  (this work-around will be removed when the userland libraries and
  utilities are updated to take this change into account).

o Modify ufs_sync_acl_from_inode() so that it forces
  ACL_{USER_OBJ,GROUP_OBJ,MASK,OTHER} ae_id fields to ACL_UNDEFINED_ID
  when synchronizing the ACL from the inode.

o Modify ufs_sync_inode_from_acl to not propagate uid and gid
  information to the inode from the ACL during ACL update.  Also
  modify the masking of permission bits that may be set from
  ALLPERMS to (S_IRWXU|S_IRWXG|S_IRWXO), as ACLs currently do not
  carry none-ACCESSPERMS (S_ISUID, S_ISGID, S_ISTXT).

o Modify ufs_getacl() so that when it emulates an access ACL from
  the inode, it initializes the ae_id fields to ACL_UNDEFINED_ID.

o Clean up ufs_setacl() substantially since it is no longer possible
  to perform chown/chgrp operations using vop_setacl(), so all the
  access control for that can be eliminated.

o Modify ufs_access() so that it passes owner uid and gid information
  into vaccess_acl_posix1e().

Pointed out by:	jedger
Obtained from:	TrustedBSD Project

3 files changed, 297 insertions, 135 deletions

diff --git a/sys/kern/kern_acl.c b/sys/kern/kern_acl.c
index d2c878a..2ae39e0 100644
--- a/sys/kern/kern_acl.c
+++ b/sys/kern/kern_acl.c
@@ -51,8 +51,8 @@ static int	vacl_set_acl(struct proc *p, struct vnode *vp, acl_type_t type,
 	    struct acl *aclp);
 static int	vacl_get_acl(struct proc *p, struct vnode *vp, acl_type_t type,
 	    struct acl *aclp);
-static int	vacl_aclcheck(struct proc *p, struct vnode *vp, acl_type_t type,
-	    struct acl *aclp);
+static int	vacl_aclcheck(struct proc *p, struct vnode *vp,
+	    acl_type_t type, struct acl *aclp);
 
 /*
  * Implement a version of vaccess() that understands POSIX.1e ACL semantics.
@@ -60,8 +60,8 @@ static int	vacl_aclcheck(struct proc *p, struct vnode *vp, acl_type_t type,
  * vaccess() eventually.
  */
 int
-vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
-    struct ucred *cred, int *privused)
+vaccess_acl_posix1e(enum vtype type, uid_t file_uid, gid_t file_gid,
+    struct acl *acl, mode_t acc_mode, struct ucred *cred, int *privused)
 {
 	struct acl_entry *acl_other, *acl_mask;
 	mode_t dac_granted;
@@ -123,7 +123,7 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 	for (i = 0; i < acl->acl_cnt; i++) {
 		switch (acl->acl_entry[i].ae_tag) {
 		case ACL_USER_OBJ:
-			if (acl->acl_entry[i].ae_id != cred->cr_uid)
+			if (file_uid != cred->cr_uid)
 				break;
 			dac_granted = 0;
 			dac_granted |= VADMIN;
@@ -209,13 +209,13 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 			dac_granted &= acl_mask_granted;
 			if ((acc_mode & dac_granted) == acc_mode)
 				return (0);
-			if ((acc_mode & (dac_granted | cap_granted)) ==
-			    acc_mode) {
-				if (privused != NULL)
-					*privused = 1;
-				return (0);
-			}
-			goto error;
+			if ((acc_mode & (dac_granted | cap_granted)) !=
+			    acc_mode)
+				goto error;
+
+			if (privused != NULL)
+				*privused = 1;
+			return (0);
 		}
 	}
 
@@ -229,22 +229,41 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 	for (i = 0; i < acl->acl_cnt; i++) {
 		switch (acl->acl_entry[i].ae_tag) {
 		case ACL_GROUP_OBJ:
+			if (file_gid != cred->cr_groups[0])
+				break;
+			dac_granted = 0;
+			if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+			if (acl->acl_entry[i].ae_perm & ACL_READ)
+				dac_granted |= VREAD;
+			if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+				dac_granted |= VWRITE;
+			dac_granted  &= acl_mask_granted;
+
+			if ((acc_mode & dac_granted) == acc_mode)
+				return (0);
+
+			group_matched = 1;
+			break;
+
 		case ACL_GROUP:
-			if (groupmember(acl->acl_entry[i].ae_id, cred)) {
-				dac_granted = 0;
-				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
-					dac_granted |= VEXEC;
-				if (acl->acl_entry[i].ae_perm & ACL_READ)
-					dac_granted |= VREAD;
-				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
-					dac_granted |= VWRITE;
-				dac_granted  &= acl_mask_granted;
+			if (!groupmember(acl->acl_entry[i].ae_id, cred))
+				break;
+			dac_granted = 0;
+			if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+			if (acl->acl_entry[i].ae_perm & ACL_READ)
+				dac_granted |= VREAD;
+			if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+				dac_granted |= VWRITE;
+			dac_granted  &= acl_mask_granted;
 
-				if ((acc_mode & dac_granted) == acc_mode)
-					return (0);
+			if ((acc_mode & dac_granted) == acc_mode)
+				return (0);
+
+			group_matched = 1;
+			break;
 
-				group_matched = 1;
-			}
 		default:
 		}
 	}
@@ -257,27 +276,46 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 		for (i = 0; i < acl->acl_cnt; i++) {
 			switch (acl->acl_entry[i].ae_tag) {
 			case ACL_GROUP_OBJ:
-			case ACL_GROUP:
-				if (groupmember(acl->acl_entry[i].ae_id,
-				    cred)) {
-					dac_granted = 0;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_EXECUTE)
+				if (file_gid != cred->cr_groups[0])
+					break;
+				dac_granted = 0;
+				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
 					dac_granted |= VEXEC;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_READ)
-						dac_granted |= VREAD;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_WRITE)
-						dac_granted |= VWRITE;
-					dac_granted &= acl_mask_granted;
-					if ((acc_mode & (dac_granted |
-					    cap_granted)) == acc_mode) {
-						if (privused != NULL)
-							*privused = 1;
-						return (0);
-					}
-				}
+				if (acl->acl_entry[i].ae_perm & ACL_READ)
+					dac_granted |= VREAD;
+				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+					dac_granted |= VWRITE;
+				dac_granted &= acl_mask_granted;
+
+				if ((acc_mode & (dac_granted | cap_granted)) !=
+				    acc_mode)
+					break;
+
+				if (privused != NULL)
+					*privused = 1;
+				return (0);
+
+			case ACL_GROUP:
+				if (!groupmember(acl->acl_entry[i].ae_id,
+				    cred))
+					break;
+				dac_granted = 0;
+				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+				if (acl->acl_entry[i].ae_perm & ACL_READ)
+					dac_granted |= VREAD;
+				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+					dac_granted |= VWRITE;
+				dac_granted &= acl_mask_granted;
+
+				if ((acc_mode & (dac_granted | cap_granted)) !=
+				    acc_mode)
+					break;
+
+				if (privused != NULL)
+					*privused = 1;
+				return (0);
+
 			default:
 			}
 		}
@@ -454,21 +492,37 @@ acl_posix1e_check(struct acl *acl)
 		 */
 		switch(acl->acl_entry[i].ae_tag) {
 		case ACL_USER_OBJ:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_user_obj++;
 			break;
 		case ACL_GROUP_OBJ:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_group_obj++;
 			break;
 		case ACL_USER:
+			if (acl->acl_entry[i].ae_id == ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_user++;
 			break;
 		case ACL_GROUP:
+			if (acl->acl_entry[i].ae_id == ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_group++;
 			break;
 		case ACL_OTHER:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_other++;
 			break;
 		case ACL_MASK:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_mask++;
 			break;
 		default:
diff --git a/sys/kern/subr_acl_posix1e.c b/sys/kern/subr_acl_posix1e.c
index d2c878a..2ae39e0 100644
--- a/sys/kern/subr_acl_posix1e.c
+++ b/sys/kern/subr_acl_posix1e.c
@@ -51,8 +51,8 @@ static int	vacl_set_acl(struct proc *p, struct vnode *vp, acl_type_t type,
 	    struct acl *aclp);
 static int	vacl_get_acl(struct proc *p, struct vnode *vp, acl_type_t type,
 	    struct acl *aclp);
-static int	vacl_aclcheck(struct proc *p, struct vnode *vp, acl_type_t type,
-	    struct acl *aclp);
+static int	vacl_aclcheck(struct proc *p, struct vnode *vp,
+	    acl_type_t type, struct acl *aclp);
 
 /*
  * Implement a version of vaccess() that understands POSIX.1e ACL semantics.
@@ -60,8 +60,8 @@ static int	vacl_aclcheck(struct proc *p, struct vnode *vp, acl_type_t type,
  * vaccess() eventually.
  */
 int
-vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
-    struct ucred *cred, int *privused)
+vaccess_acl_posix1e(enum vtype type, uid_t file_uid, gid_t file_gid,
+    struct acl *acl, mode_t acc_mode, struct ucred *cred, int *privused)
 {
 	struct acl_entry *acl_other, *acl_mask;
 	mode_t dac_granted;
@@ -123,7 +123,7 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 	for (i = 0; i < acl->acl_cnt; i++) {
 		switch (acl->acl_entry[i].ae_tag) {
 		case ACL_USER_OBJ:
-			if (acl->acl_entry[i].ae_id != cred->cr_uid)
+			if (file_uid != cred->cr_uid)
 				break;
 			dac_granted = 0;
 			dac_granted |= VADMIN;
@@ -209,13 +209,13 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 			dac_granted &= acl_mask_granted;
 			if ((acc_mode & dac_granted) == acc_mode)
 				return (0);
-			if ((acc_mode & (dac_granted | cap_granted)) ==
-			    acc_mode) {
-				if (privused != NULL)
-					*privused = 1;
-				return (0);
-			}
-			goto error;
+			if ((acc_mode & (dac_granted | cap_granted)) !=
+			    acc_mode)
+				goto error;
+
+			if (privused != NULL)
+				*privused = 1;
+			return (0);
 		}
 	}
 
@@ -229,22 +229,41 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 	for (i = 0; i < acl->acl_cnt; i++) {
 		switch (acl->acl_entry[i].ae_tag) {
 		case ACL_GROUP_OBJ:
+			if (file_gid != cred->cr_groups[0])
+				break;
+			dac_granted = 0;
+			if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+			if (acl->acl_entry[i].ae_perm & ACL_READ)
+				dac_granted |= VREAD;
+			if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+				dac_granted |= VWRITE;
+			dac_granted  &= acl_mask_granted;
+
+			if ((acc_mode & dac_granted) == acc_mode)
+				return (0);
+
+			group_matched = 1;
+			break;
+
 		case ACL_GROUP:
-			if (groupmember(acl->acl_entry[i].ae_id, cred)) {
-				dac_granted = 0;
-				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
-					dac_granted |= VEXEC;
-				if (acl->acl_entry[i].ae_perm & ACL_READ)
-					dac_granted |= VREAD;
-				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
-					dac_granted |= VWRITE;
-				dac_granted  &= acl_mask_granted;
+			if (!groupmember(acl->acl_entry[i].ae_id, cred))
+				break;
+			dac_granted = 0;
+			if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+			if (acl->acl_entry[i].ae_perm & ACL_READ)
+				dac_granted |= VREAD;
+			if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+				dac_granted |= VWRITE;
+			dac_granted  &= acl_mask_granted;
 
-				if ((acc_mode & dac_granted) == acc_mode)
-					return (0);
+			if ((acc_mode & dac_granted) == acc_mode)
+				return (0);
+
+			group_matched = 1;
+			break;
 
-				group_matched = 1;
-			}
 		default:
 		}
 	}
@@ -257,27 +276,46 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 		for (i = 0; i < acl->acl_cnt; i++) {
 			switch (acl->acl_entry[i].ae_tag) {
 			case ACL_GROUP_OBJ:
-			case ACL_GROUP:
-				if (groupmember(acl->acl_entry[i].ae_id,
-				    cred)) {
-					dac_granted = 0;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_EXECUTE)
+				if (file_gid != cred->cr_groups[0])
+					break;
+				dac_granted = 0;
+				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
 					dac_granted |= VEXEC;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_READ)
-						dac_granted |= VREAD;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_WRITE)
-						dac_granted |= VWRITE;
-					dac_granted &= acl_mask_granted;
-					if ((acc_mode & (dac_granted |
-					    cap_granted)) == acc_mode) {
-						if (privused != NULL)
-							*privused = 1;
-						return (0);
-					}
-				}
+				if (acl->acl_entry[i].ae_perm & ACL_READ)
+					dac_granted |= VREAD;
+				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+					dac_granted |= VWRITE;
+				dac_granted &= acl_mask_granted;
+
+				if ((acc_mode & (dac_granted | cap_granted)) !=
+				    acc_mode)
+					break;
+
+				if (privused != NULL)
+					*privused = 1;
+				return (0);
+
+			case ACL_GROUP:
+				if (!groupmember(acl->acl_entry[i].ae_id,
+				    cred))
+					break;
+				dac_granted = 0;
+				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+				if (acl->acl_entry[i].ae_perm & ACL_READ)
+					dac_granted |= VREAD;
+				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+					dac_granted |= VWRITE;
+				dac_granted &= acl_mask_granted;
+
+				if ((acc_mode & (dac_granted | cap_granted)) !=
+				    acc_mode)
+					break;
+
+				if (privused != NULL)
+					*privused = 1;
+				return (0);
+
 			default:
 			}
 		}
@@ -454,21 +492,37 @@ acl_posix1e_check(struct acl *acl)
 		 */
 		switch(acl->acl_entry[i].ae_tag) {
 		case ACL_USER_OBJ:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_user_obj++;
 			break;
 		case ACL_GROUP_OBJ:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_group_obj++;
 			break;
 		case ACL_USER:
+			if (acl->acl_entry[i].ae_id == ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_user++;
 			break;
 		case ACL_GROUP:
+			if (acl->acl_entry[i].ae_id == ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_group++;
 			break;
 		case ACL_OTHER:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_other++;
 			break;
 		case ACL_MASK:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_mask++;
 			break;
 		default:
diff --git a/sys/kern/vfs_acl.c b/sys/kern/vfs_acl.c
index d2c878a..2ae39e0 100644
--- a/sys/kern/vfs_acl.c
+++ b/sys/kern/vfs_acl.c
@@ -51,8 +51,8 @@ static int	vacl_set_acl(struct proc *p, struct vnode *vp, acl_type_t type,
 	    struct acl *aclp);
 static int	vacl_get_acl(struct proc *p, struct vnode *vp, acl_type_t type,
 	    struct acl *aclp);
-static int	vacl_aclcheck(struct proc *p, struct vnode *vp, acl_type_t type,
-	    struct acl *aclp);
+static int	vacl_aclcheck(struct proc *p, struct vnode *vp,
+	    acl_type_t type, struct acl *aclp);
 
 /*
  * Implement a version of vaccess() that understands POSIX.1e ACL semantics.
@@ -60,8 +60,8 @@ static int	vacl_aclcheck(struct proc *p, struct vnode *vp, acl_type_t type,
  * vaccess() eventually.
  */
 int
-vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
-    struct ucred *cred, int *privused)
+vaccess_acl_posix1e(enum vtype type, uid_t file_uid, gid_t file_gid,
+    struct acl *acl, mode_t acc_mode, struct ucred *cred, int *privused)
 {
 	struct acl_entry *acl_other, *acl_mask;
 	mode_t dac_granted;
@@ -123,7 +123,7 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 	for (i = 0; i < acl->acl_cnt; i++) {
 		switch (acl->acl_entry[i].ae_tag) {
 		case ACL_USER_OBJ:
-			if (acl->acl_entry[i].ae_id != cred->cr_uid)
+			if (file_uid != cred->cr_uid)
 				break;
 			dac_granted = 0;
 			dac_granted |= VADMIN;
@@ -209,13 +209,13 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 			dac_granted &= acl_mask_granted;
 			if ((acc_mode & dac_granted) == acc_mode)
 				return (0);
-			if ((acc_mode & (dac_granted | cap_granted)) ==
-			    acc_mode) {
-				if (privused != NULL)
-					*privused = 1;
-				return (0);
-			}
-			goto error;
+			if ((acc_mode & (dac_granted | cap_granted)) !=
+			    acc_mode)
+				goto error;
+
+			if (privused != NULL)
+				*privused = 1;
+			return (0);
 		}
 	}
 
@@ -229,22 +229,41 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 	for (i = 0; i < acl->acl_cnt; i++) {
 		switch (acl->acl_entry[i].ae_tag) {
 		case ACL_GROUP_OBJ:
+			if (file_gid != cred->cr_groups[0])
+				break;
+			dac_granted = 0;
+			if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+			if (acl->acl_entry[i].ae_perm & ACL_READ)
+				dac_granted |= VREAD;
+			if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+				dac_granted |= VWRITE;
+			dac_granted  &= acl_mask_granted;
+
+			if ((acc_mode & dac_granted) == acc_mode)
+				return (0);
+
+			group_matched = 1;
+			break;
+
 		case ACL_GROUP:
-			if (groupmember(acl->acl_entry[i].ae_id, cred)) {
-				dac_granted = 0;
-				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
-					dac_granted |= VEXEC;
-				if (acl->acl_entry[i].ae_perm & ACL_READ)
-					dac_granted |= VREAD;
-				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
-					dac_granted |= VWRITE;
-				dac_granted  &= acl_mask_granted;
+			if (!groupmember(acl->acl_entry[i].ae_id, cred))
+				break;
+			dac_granted = 0;
+			if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+			if (acl->acl_entry[i].ae_perm & ACL_READ)
+				dac_granted |= VREAD;
+			if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+				dac_granted |= VWRITE;
+			dac_granted  &= acl_mask_granted;
 
-				if ((acc_mode & dac_granted) == acc_mode)
-					return (0);
+			if ((acc_mode & dac_granted) == acc_mode)
+				return (0);
+
+			group_matched = 1;
+			break;
 
-				group_matched = 1;
-			}
 		default:
 		}
 	}
@@ -257,27 +276,46 @@ vaccess_acl_posix1e(enum vtype type, struct acl *acl, mode_t acc_mode,
 		for (i = 0; i < acl->acl_cnt; i++) {
 			switch (acl->acl_entry[i].ae_tag) {
 			case ACL_GROUP_OBJ:
-			case ACL_GROUP:
-				if (groupmember(acl->acl_entry[i].ae_id,
-				    cred)) {
-					dac_granted = 0;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_EXECUTE)
+				if (file_gid != cred->cr_groups[0])
+					break;
+				dac_granted = 0;
+				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
 					dac_granted |= VEXEC;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_READ)
-						dac_granted |= VREAD;
-					if (acl->acl_entry[i].ae_perm &
-					    ACL_WRITE)
-						dac_granted |= VWRITE;
-					dac_granted &= acl_mask_granted;
-					if ((acc_mode & (dac_granted |
-					    cap_granted)) == acc_mode) {
-						if (privused != NULL)
-							*privused = 1;
-						return (0);
-					}
-				}
+				if (acl->acl_entry[i].ae_perm & ACL_READ)
+					dac_granted |= VREAD;
+				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+					dac_granted |= VWRITE;
+				dac_granted &= acl_mask_granted;
+
+				if ((acc_mode & (dac_granted | cap_granted)) !=
+				    acc_mode)
+					break;
+
+				if (privused != NULL)
+					*privused = 1;
+				return (0);
+
+			case ACL_GROUP:
+				if (!groupmember(acl->acl_entry[i].ae_id,
+				    cred))
+					break;
+				dac_granted = 0;
+				if (acl->acl_entry[i].ae_perm & ACL_EXECUTE)
+				dac_granted |= VEXEC;
+				if (acl->acl_entry[i].ae_perm & ACL_READ)
+					dac_granted |= VREAD;
+				if (acl->acl_entry[i].ae_perm & ACL_WRITE)
+					dac_granted |= VWRITE;
+				dac_granted &= acl_mask_granted;
+
+				if ((acc_mode & (dac_granted | cap_granted)) !=
+				    acc_mode)
+					break;
+
+				if (privused != NULL)
+					*privused = 1;
+				return (0);
+
 			default:
 			}
 		}
@@ -454,21 +492,37 @@ acl_posix1e_check(struct acl *acl)
 		 */
 		switch(acl->acl_entry[i].ae_tag) {
 		case ACL_USER_OBJ:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_user_obj++;
 			break;
 		case ACL_GROUP_OBJ:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_group_obj++;
 			break;
 		case ACL_USER:
+			if (acl->acl_entry[i].ae_id == ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_user++;
 			break;
 		case ACL_GROUP:
+			if (acl->acl_entry[i].ae_id == ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_group++;
 			break;
 		case ACL_OTHER:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_other++;
 			break;
 		case ACL_MASK:
+			acl->acl_entry[i].ae_id = ACL_UNDEFINED_ID; /* XXX */
+			if (acl->acl_entry[i].ae_id != ACL_UNDEFINED_ID)
+				return (EINVAL);
 			num_acl_mask++;
 			break;
 		default: