From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date: Wed, 22 Nov 2023 03:40:12 +0000 (+1300)
Subject: libcli/security: claim_v1_to_ace_token(): avoid unnecessary re-sort
X-Git-Tag: talloc-2.4.2~499
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4f56c702834072d0353072b28a317208eeabb886;p=thirdparty%2Fsamba.git

libcli/security: claim_v1_to_ace_token(): avoid unnecessary re-sort

If it is a wire claim (which is probably most common), the checking
and sorting has already happened. We don't need to make a copy to
sort and check.

In either case, there is still a copy step to make the conditional ACE
token.

This shuffles around some knownfails because the claim_v1_copy()
function we were using is checking for duplicates, which we don't
always want. That will be fixed soon.

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
---

diff --git a/libcli/security/claims-conversions.c b/libcli/security/claims-conversions.c
index 770795e29e7..6c55420fff3 100644
--- a/libcli/security/claims-conversions.c
+++ b/libcli/security/claims-conversions.c
@@ -301,9 +301,13 @@ bool claim_v1_to_ace_token(TALLOC_CTX *mem_ctx,
 {
 	size_t i;
 	struct ace_condition_token *tokens = NULL;
-	struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *sorted_claim = NULL;
+	struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *claim_copy = NULL;
+	const struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1 *sorted_claim = NULL;
 	NTSTATUS status;
 	bool ok;
+	bool case_sensitive = claim->flags &			\
+		CLAIM_SECURITY_ATTRIBUTE_VALUE_CASE_SENSITIVE;
+
 	if (claim->value_count < 1 ||
 	    claim->value_count >= CONDITIONAL_ACE_MAX_TOKENS) {
 		DBG_WARNING("rejecting claim with %"PRIu32" tokens\n",
@@ -322,18 +326,46 @@ bool claim_v1_to_ace_token(TALLOC_CTX *mem_ctx,
 						    result);
 	}
 
-	ok = claim_v1_copy(mem_ctx, sorted_claim, claim);
-	if (!ok) {
-		return false;
-	}
+	if (claim->flags & CLAIM_SECURITY_ATTRIBUTE_UNIQUE_AND_SORTED) {
+		/*
+		 * We can avoid making a sorted copy.
+		 *
+		 * This is normal case for wire claims, where the
+		 * sorting and duplicate checking happens earlier in
+		 * token_claims_to_claims_v1().
+		*/
+		sorted_claim = claim;
+	} else {
+		/*
+		 * This is presumably a resource attribute ACE, which
+		 * is stored in the ACE as struct
+		 * CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1, and we don't
+		 * really want to mutate that copy -- even if there
+		 * aren't currently realistic pathways that read an
+		 * ACE, trigger this, and write it back (outside of
+		 * tests).
+		 */
+		claim_copy = talloc(mem_ctx, struct CLAIM_SECURITY_ATTRIBUTE_RELATIVE_V1);
+		if (claim_copy == NULL) {
+			return false;
+		}
 
-	status = claim_v1_check_and_sort(mem_ctx, sorted_claim);
-	if (!NT_STATUS_IS_OK(status)) {
-		DBG_WARNING("resource attribute claim sort failed with %s\n",
-			    nt_errstr(status));
-		return false;
-	}
+		ok = claim_v1_copy(claim_copy, claim_copy, claim);
+		if (!ok) {
+			TALLOC_FREE(claim_copy);
+			return false;
+		}
 
+		status = claim_v1_check_and_sort(claim_copy, claim_copy,
+						 case_sensitive);
+		if (!NT_STATUS_IS_OK(status)) {
+			DBG_WARNING("resource attribute claim sort failed with %s\n",
+				    nt_errstr(status));
+			TALLOC_FREE(claim_copy);
+			return false;
+		}
+		sorted_claim = claim_copy;
+	}
 	/*
 	 * The multiple values will get turned into a composite
 	 * literal in the conditional ACE. Each element of the
@@ -352,15 +384,17 @@ bool claim_v1_to_ace_token(TALLOC_CTX *mem_ctx,
 			      struct ace_condition_token,
 			      sorted_claim->value_count);
 	if (tokens == NULL) {
+		TALLOC_FREE(claim_copy);
 		return false;
 	}
 
 	for (i = 0; i < sorted_claim->value_count; i++) {
-		bool ok = claim_v1_offset_to_ace_token(tokens,
-						       sorted_claim,
-						       i,
-						       &tokens[i]);
+		ok = claim_v1_offset_to_ace_token(tokens,
+						  sorted_claim,
+						  i,
+						  &tokens[i]);
 		if (! ok) {
+			TALLOC_FREE(claim_copy);
 			TALLOC_FREE(tokens);
 			return false;
 		}
diff --git a/selftest/knownfail.d/conditional_ace_claims b/selftest/knownfail.d/conditional_ace_claims
index 8fb6730b6f8..40e342afab7 100644
--- a/selftest/knownfail.d/conditional_ace_claims
+++ b/selftest/knownfail.d/conditional_ace_claims
@@ -1,4 +1 @@
 ^samba.tests.conditional_ace_claims.+ConditionalAceLargeComposites.test_allow_002-0-vs-0
-^samba.tests.conditional_ace_claims.+ConditionalAceLargeComposites.test_deny_001-90-disorderly-strings-claim-vs-claim-case-sensitive-with-dupes
-^samba.tests.conditional_ace_claims.+ConditionalAceLargeComposites.test_deny_022-102+1-dupe-vs-102+1-dupe
-^samba.tests.conditional_ace_claims.+ConditionalAceLargeComposites.test_deny_024-2+1-dupe-vs-2+1-dupe
diff --git a/selftest/knownfail.d/run_conditional_ace b/selftest/knownfail.d/run_conditional_ace
deleted file mode 100644
index 4527c8299cf..00000000000
--- a/selftest/knownfail.d/run_conditional_ace
+++ /dev/null
@@ -1,2 +0,0 @@
-^samba.unittests.run_conditional_ace.test_composite_different_order_with_SID_dupes$
-^samba.unittests.run_conditional_ace.test_composite_different_order_with_dupes$