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 5c6e02e37bd28009dcdb365371837ba9c6b6da51..c6da72003e639ba2a9ed4d3e871ccc7a4efc648f 100644 (file)
--- 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 cce117934f90e859c347a4de332a9b2a5394edac..f32c3af87f8e8efa7afb5b736975d769964a4bbf 100644 (file)
--- 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 479c0aa0527d89e7e6a195a719474dde5c8ec49d..529a8d59a9280501275322b085f303ee88d8216f 100644 (file)
--- 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;