libcli/security: correct access check and maximum access calculation for Owner Rights ACEs

We basically must process the Owner Rights ACEs as any other ACE wrt to the
order of adding granted permissions and checking denied permissions. According
to MS-DTYP 2.5.3.2 Owner Rights ACEs must be evaluated in the main loop over
the ACEs in an ACL and the corresponding access_mask must be directly applied
to bits_remaining. We currently defer this to after the loop over the ACEs in
ACL, this is wrong.

We just have to do some initial magic to determine if an ACL contains and
Owner Rights ACEs, and in case it doesn't we grant SEC_STD_WRITE_DAC |
SEC_STD_READ_CONTROL at the *beginning*. MS-DTYP:

-- the owner of an object is always granted READ_CONTROL and WRITE_DAC.
CALL SidInToken(Token, SecurityDescriptor.Owner, PrincipalSelfSubst)
IF SidInToken returns True THEN
   IF DACL does not contain ACEs from object owner THEN
       Remove READ_CONTROL and WRITE_DAC from RemainingAccess
       Set GrantedAccess to GrantedAccess or READ_CONTROL or WRITE_OWNER
   END IF
END IF

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13812

Signed-off-by: Ralph Boehme <slow@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
(cherry picked from commit 9722f75757c0e38c7f42c7cc310d56aa6eaf6392)

2 files changed, 73 insertions, 69 deletions

diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
index 5d49b718f0c..d1d57eecef2 100644
--- a/libcli/security/access_check.c
+++ b/libcli/security/access_check.c
@@ -109,10 +109,9 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
 					const struct security_token *token)
 {
 	uint32_t denied = 0, granted = 0;
+	bool am_owner = false;
+	bool have_owner_rights_ace = false;
 	unsigned i;
-	uint32_t owner_rights_allowed = 0;
-	uint32_t owner_rights_denied = 0;
-	bool owner_rights_default = true;
 
 	if (sd->dacl == NULL) {
 		if (security_token_has_sid(token, sd->owner_sid)) {
@@ -121,26 +120,50 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
 		return granted;
 	}
 
+	if (security_token_has_sid(token, sd->owner_sid)) {
+		/*
+		 * Check for explicit owner rights: if there are none, we remove
+		 * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL
+		 * from remaining_access. Otherwise we just process the
+		 * explicitly granted rights when processing the ACEs.
+		 */
+		am_owner = true;
+
+		for (i=0; i < sd->dacl->num_aces; i++) {
+			struct security_ace *ace = &sd->dacl->aces[i];
+
+			if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
+				continue;
+			}
+
+			have_owner_rights_ace = dom_sid_equal(
+				&ace->trustee, &global_sid_Owner_Rights);
+			if (have_owner_rights_ace) {
+				break;
+			}
+		}
+	}
+
+	if (am_owner && !have_owner_rights_ace) {
+		granted |= SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL;
+	}
+
 	for (i = 0;i<sd->dacl->num_aces; i++) {
 		struct security_ace *ace = &sd->dacl->aces[i];
+		bool is_owner_rights_ace = false;
 
 		if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
 			continue;
 		}
 
-		if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
-			if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
-				owner_rights_allowed |= ace->access_mask;
-				owner_rights_default = false;
-			} else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
-				owner_rights_denied |= (owner_rights_allowed &
-							ace->access_mask);
-				owner_rights_default = false;
-			}
-			continue;
+		if (am_owner) {
+			is_owner_rights_ace = dom_sid_equal(
+				&ace->trustee, &global_sid_Owner_Rights);
 		}
 
-		if (!security_token_has_sid(token, &ace->trustee)) {
+		if (!is_owner_rights_ace &&
+		    !security_token_has_sid(token, &ace->trustee))
+		{
 			continue;
 		}
 
@@ -157,15 +180,6 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
 		}
 	}
 
-	if (security_token_has_sid(token, sd->owner_sid)) {
-		if (owner_rights_default) {
-			granted |= SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL;
-		} else {
-			granted |= owner_rights_allowed;
-			granted &= ~owner_rights_denied;
-		}
-	}
-
 	return granted & ~denied;
 }
 
@@ -182,16 +196,8 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
 	uint32_t i;
 	uint32_t bits_remaining;
 	uint32_t explicitly_denied_bits = 0;
-	/*
-	 * Up until Windows Server 2008, owner always had these rights. Now
-	 * we have to use Owner Rights perms if they are on the file.
-	 *
-	 * In addition we have to accumulate these bits and apply them
-	 * correctly. See bug #8795
-	 */
-	uint32_t owner_rights_allowed = 0;
-	uint32_t owner_rights_denied = 0;
-	bool owner_rights_default = true;
+	bool am_owner = false;
+	bool have_owner_rights_ace = false;
 
 	*access_granted = access_desired;
 	bits_remaining = access_desired;
@@ -221,35 +227,50 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
 		goto done;
 	}
 
+	if (security_token_has_sid(token, sd->owner_sid)) {
+		/*
+		 * Check for explicit owner rights: if there are none, we remove
+		 * the default owner right SEC_STD_WRITE_DAC|SEC_STD_READ_CONTROL
+		 * from remaining_access. Otherwise we just process the
+		 * explicitly granted rights when processing the ACEs.
+		 */
+		am_owner = true;
+
+		for (i=0; i < sd->dacl->num_aces; i++) {
+			struct security_ace *ace = &sd->dacl->aces[i];
+
+			if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
+				continue;
+			}
+
+			have_owner_rights_ace = dom_sid_equal(
+				&ace->trustee, &global_sid_Owner_Rights);
+			if (have_owner_rights_ace) {
+				break;
+			}
+		}
+	}
+	if (am_owner && !have_owner_rights_ace) {
+		bits_remaining &= ~(SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL);
+	}
+
 	/* check each ace in turn. */
 	for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) {
 		struct security_ace *ace = &sd->dacl->aces[i];
+		bool is_owner_rights_ace = false;
 
 		if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
 			continue;
 		}
 
-		/*
-		 * We need the Owner Rights permissions to ensure we
-		 * give or deny the correct permissions to the owner. Replace
-		 * owner_rights with the perms here if it is present.
-		 *
-		 * We don't care if we are not the owner because that is taken
-		 * care of below when we check if our token has the owner SID.
-		 *
-		 */
-		if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
-			if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
-				owner_rights_allowed |= ace->access_mask;
-				owner_rights_default = false;
-			} else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
-				owner_rights_denied |= (bits_remaining & ace->access_mask);
-				owner_rights_default = false;
-			}
-			continue;
+		if (am_owner) {
+			is_owner_rights_ace = dom_sid_equal(
+				&ace->trustee, &global_sid_Owner_Rights);
 		}
 
-		if (!security_token_has_sid(token, &ace->trustee)) {
+		if (!is_owner_rights_ace &&
+		    !security_token_has_sid(token, &ace->trustee))
+		{
 			continue;
 		}
 
@@ -269,21 +290,6 @@ NTSTATUS se_access_check(const struct security_descriptor *sd,
 	/* Explicitly denied bits always override */
 	bits_remaining |= explicitly_denied_bits;
 
-	/* The owner always gets owner rights as defined above. */
-	if (security_token_has_sid(token, sd->owner_sid)) {
-		if (owner_rights_default) {
-			/*
-			 * Just remove them, no need to check if they are
-			 * there.
-			 */
-			bits_remaining &= ~(SEC_STD_WRITE_DAC |
-						SEC_STD_READ_CONTROL);
-		} else {
-			bits_remaining &= ~owner_rights_allowed;
-			bits_remaining |= owner_rights_denied;
-		}
-	}
-
 	/*
 	 * We check privileges here because they override even DENY entries.
 	 */
diff --git a/selftest/knownfail.d/smb2.acls b/selftest/knownfail.d/smb2.acls
deleted file mode 100644
index e1b98cec606..00000000000
--- a/selftest/knownfail.d/smb2.acls
+++ /dev/null
@@ -1,2 +0,0 @@
-^samba3.smb2.acls.OWNER-RIGHTS-DENY\(ad_dc\)
-^samba3.smb2.acls.OWNER-RIGHTS-DENY\(nt4_dc\)