From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date: Fri, 22 Sep 2023 00:53:42 +0000 (+1200)
Subject: libcli/security/sddl_conditional_ace: ban empty expressions in SDDL
X-Git-Tag: tevent-0.16.0~405
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5650b511c1fb98106942ca2829bd4fcfdae4eca1;p=thirdparty%2Fsamba.git

libcli/security/sddl_conditional_ace: ban empty expressions in SDDL

The trouble is with expressions like "(!(()))", which boil down to a
single NOT operation with no argument, which is invalid and can't be
run or expressed as SDDL.

Signed-off-by: Douglas Bagnall <douglas.bagnall@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 5c6e02e37bd..c6da72003e6 100644
--- a/libcli/security/sddl_conditional_ace.c
+++ b/libcli/security/sddl_conditional_ace.c
@@ -39,6 +39,7 @@
 #define SDDL_FLAG_EXPECTING_PAREN            64
 #define SDDL_FLAG_EXPECTING_END             128
 #define SDDL_FLAG_EXPECTING_PAREN_LITERAL   256
+#define SDDL_FLAG_NOT_EXPECTING_END_PAREN   512
 
 #define SDDL_FLAG_IS_UNARY_OP               (1 << 20)
 #define SDDL_FLAG_IS_BINARY_OP              (1 << 21)
@@ -2559,6 +2560,8 @@ static bool parse_expression(struct ace_condition_sddl_compiler_context *comp)
 	comp->state = SDDL_FLAGS_EXPR_START;
 	DBG_INFO("%3"PRIu32": (\n", comp->offset);
 
+	comp->state |= SDDL_FLAG_NOT_EXPECTING_END_PAREN;
+
 	while (comp->offset < comp->length) {
 		uint8_t c;
 		ok = eat_whitespace(comp, false);
@@ -2578,6 +2581,13 @@ static bool parse_expression(struct ace_condition_sddl_compiler_context *comp)
 					   "operator lacks right hand argument");
 				return false;
 			}
+			if (comp->state & SDDL_FLAG_NOT_EXPECTING_END_PAREN) {
+				/*
+				 * You can't have "( )"
+				 */
+				comp_error(comp, "empty expression");
+				return false;
+			}
 			break;
 		} else if (c == '@') {
 			ok = parse_attr2(comp);
diff --git a/libcli/security/tests/test_run_conditional_ace.c b/libcli/security/tests/test_run_conditional_ace.c
index cce117934f9..f32c3af87f8 100644
--- a/libcli/security/tests/test_run_conditional_ace.c
+++ b/libcli/security/tests/test_run_conditional_ace.c
@@ -570,16 +570,7 @@ static void test_resource_ace_multi_any_of(void **state)
 	ALLOW_CHECK(0x10);
 }
 
-
 static void test_horrible_fuzz_derived_test_3(void **state)
-{
-	INIT();
-	USER_SIDS("WD", "AA", "IS");
-	SD("S:PPPPPPD:(XD;OI;0x1;;;IS;())(OL;;GR;;;S-1-5-75-552)");
-	DENY_CHECK(0x1);
-}
-
-static void test_horrible_fuzz_derived_test_4(void **state)
 {
 	INIT();
 	USER_SIDS("WD", "AA", "IS");
@@ -637,7 +628,6 @@ int main(_UNUSED_ int argc, _UNUSED_ const char **argv)
 		cmocka_unit_test(test_not_Not_Any_of_1),
 		cmocka_unit_test(test_not_any_of_composite_1),
 		cmocka_unit_test(test_resource_ace_single),
-		cmocka_unit_test(test_horrible_fuzz_derived_test_4),
 		cmocka_unit_test(test_horrible_fuzz_derived_test_3),
 		cmocka_unit_test(test_Device_Member_of_and_Member_of),
 		cmocka_unit_test(test_resource_ace_multi),
diff --git a/libcli/security/tests/test_sddl_conditional_ace.c b/libcli/security/tests/test_sddl_conditional_ace.c
index 479c0aa0527..529a8d59a92 100644
--- a/libcli/security/tests/test_sddl_conditional_ace.c
+++ b/libcli/security/tests/test_sddl_conditional_ace.c
@@ -754,18 +754,29 @@ static void test_a_number_of_invalid_strings(void **state)
 	 * These expressions should fail to parse.
 	 */
 	static const char *sddl[] = {
+		/* '!' is only allowed before parens or @attr */
 		"(!!! !!!  !!! Not_Member_of{SID(AA)}))",
+		/* overflowing numbers can't be sensibly interpreted */
 		("(@Device.bb == 055555624677746777766777767)"),
 		("(@Device.bb == 0x624677746777766777767)"),
 		("(@Device.bb == 624677746777766777767)"),
+		/* insufficient arguments */
 		"(!)",
 		"(x >)",
+		"(> 3)",
+		/* keyword as local attribute name */
 		"( Member_of Contains 3)",
+		/* no parens */
 		" x < 3",
+		/* wants '==' */
 		"( x = SID(BA))",
+		/* invalid SID strings */
 		"( x == SID(ZZ))",
+		"( x == SID(S-1-))",
 		"( x == SID())",
+		/* literal on LHS */
 		"(\"x\" == \"x\")",
+		/* odd number of digits following '#' */
 		"(OctetStringType==#1#2#3##))",
 	};
 	size_t i, length;