From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 1 Aug 2023 00:00:33 +0000 (+1200)
Subject: libcli/security: Allow empty composites and resource attribute lists
X-Git-Tag: talloc-2.4.2~886
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4131179a04858e87f205532ebdd1568d57a10cc8;p=thirdparty%2Fsamba.git

libcli/security: Allow empty composites and resource attribute lists

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
---

diff --git a/libcli/security/sddl_conditional_ace.c b/libcli/security/sddl_conditional_ace.c
index 1e054f5ad17..3d9db329aea 100644
--- a/libcli/security/sddl_conditional_ace.c
+++ b/libcli/security/sddl_conditional_ace.c
@@ -2409,6 +2409,7 @@ static bool parse_composite(struct ace_condition_sddl_compiler_context *comp)
 	 * where write_sddl_token() writes.
 	 */
 	bool ok;
+	bool first = true;
 	struct ace_condition_token token = {
 		.type = CONDITIONAL_ACE_TOKEN_COMPOSITE
 	};
@@ -2451,9 +2452,9 @@ static bool parse_composite(struct ace_condition_sddl_compiler_context *comp)
 	 * in this loop we are looking for:
 	 *
 	 * a) possible whitespace.
-	 * b) a literal
+	 * b) a comma (or terminating '}')
 	 * c) more possible whitespace
-	 * d) a comma (or terminating '}')
+	 * d) a literal
 	 *
 	 * Failures use a goto to reset comp->target, just in case we ever try
 	 * continuing after error.
@@ -2464,6 +2465,25 @@ static bool parse_composite(struct ace_condition_sddl_compiler_context *comp)
 		if (! ok) {
 			goto fail;
 		}
+		c = comp->sddl[comp->offset];
+		if (c == '}') {
+			comp->offset++;
+			break;
+		}
+		if (!first) {
+			if (c != ',') {
+				comp_error(comp,
+					   "malformed composite (expected comma)");
+				goto fail;
+			}
+			comp->offset++;
+
+			ok = eat_whitespace(comp, false);
+			if (! ok) {
+				goto fail;
+			}
+		}
+		first = false;
 		if (*comp->target_len >= alloc_size) {
 			comp_error(comp,
 				   "Too many tokens in composite "
@@ -2475,21 +2495,6 @@ static bool parse_composite(struct ace_condition_sddl_compiler_context *comp)
 		if (!ok) {
 			goto fail;
 		}
-		ok = eat_whitespace(comp, false);
-		if (! ok) {
-			goto fail;
-		}
-		c = comp->sddl[comp->offset];
-		if (c == '}') {
-			comp->offset++;
-			break;
-		}
-		if (c != ',') {
-			comp_error(comp,
-				   "malformed composite (expected comma)");
-			goto fail;
-		}
-		comp->offset++;
 	}
 	comp->target = old_target;
 	comp->target_len = old_target_len;
@@ -2869,6 +2874,7 @@ static bool parse_resource_attr_list(
 	 * - SIDs are not written with SID(...) around them.
 	 */
 	bool ok;
+	bool first = true;
 	struct ace_condition_token composite = {
 		.type = CONDITIONAL_ACE_TOKEN_COMPOSITE
 	};
@@ -2904,9 +2910,9 @@ static bool parse_resource_attr_list(
 	 * in this loop we are looking for:
 	 *
 	 * a) possible whitespace.
-	 * b) a literal, of the right type (checked after)
+	 * b) a comma (or terminating ')')
 	 * c) more possible whitespace
-	 * d) a comma
+	 * d) a literal, of the right type (checked after)
 	 *
 	 * Failures use a goto to reset comp->target, just in case we ever try
 	 * continuing after error.
@@ -2917,6 +2923,24 @@ static bool parse_resource_attr_list(
 		if (! ok) {
 			goto fail;
 		}
+		c = comp->sddl[comp->offset];
+		if (c == ')') {
+			break;
+		}
+		if (!first) {
+			if (c != ',') {
+				comp_error(comp,
+					   "malformed composite (expected comma)");
+				goto fail;
+			}
+			comp->offset++;
+
+			ok = eat_whitespace(comp, false);
+			if (! ok) {
+				goto fail;
+			}
+		}
+		first = false;
 		if (*comp->target_len >= alloc_size) {
 			comp_error(comp,
 				   "Too many tokens in composite "
@@ -2939,21 +2963,6 @@ static bool parse_resource_attr_list(
 		if (! ok) {
 			goto fail;
 		}
-
-		ok = eat_whitespace(comp, false);
-		if (! ok) {
-			goto fail;
-		}
-		c = comp->sddl[comp->offset];
-		if (c == ')') {
-			break;
-		}
-		if (c != ',') {
-			comp_error(comp,
-				   "malformed composite (expected comma)");
-			goto fail;
-		}
-		comp->offset++;
 	}
 	comp->target = old_target;
 	comp->target_len = old_target_len;
diff --git a/libcli/security/tests/test_sddl_conditional_ace.c b/libcli/security/tests/test_sddl_conditional_ace.c
index 61e219614b9..0fddf198105 100644
--- a/libcli/security/tests/test_sddl_conditional_ace.c
+++ b/libcli/security/tests/test_sddl_conditional_ace.c
@@ -590,6 +590,8 @@ static void test_round_trips(void **state)
 		 "Device_Member_of{SID(BA), 7, 1, 3} "
 		 "|| Exists hooly)"),
 		("(!(!(!(!(!((!(x==1))))))))"),
+		("(@User.a == {})"),
+		("(Member_of{})"),
 		("(Member_of {SID(S-1-33-5), "
 		 "SID(BO)} && @Device.Bitlocker)"),
 		"(@USER.ad://ext/AuthenticationSilo == \"siloname\")",
@@ -786,6 +788,12 @@ static void test_a_number_of_invalid_strings(void **state)
 		("(@Device.%002e == 3)"),
 		("(@Device.%002f == 3)"),
 		("(@Device.%003a == 3)"),
+		/* trailing comma in composite */
+		"(Member_of{SID(AA),})",
+		/* missing comma between elements of a composite */
+		"(Member_of{SID(AA) SID(AC)})",
+		/* unexpected comma in composite */
+		"(Member_of{,})",
 	};
 	size_t i, length;
 	TALLOC_CTX *mem_ctx = talloc_new(NULL);