libcli/sec/sddl decode: allow hex numbers in SIDs

These occur canonically when the indentifier authority is > 2^32, but
also are accepted by Windows for any number.

There is a tricky case with an "O:" or "G:" SID that is immediately
followed by a "D:" dacl, because the "D" looks like a hex digit. When
we detect this we need to subtract one from the length.

We also need to do look out for trailing garbage. This was not an
issue before because any string caught by the strspn(...,
"-0123456789") would be either rejected or fully comsumed by
dom_sid_parse_talloc(), but with hex digits, a string like
"S-1-1-2x0xabcxxx-X" would be successfully parsed as "S-1-1-2", and
the "x0xabcxxx-X" would be skipped over. That's why we switch to using
dom_sid_parse_endp(), so we can compare the consumed length to the
expected length.

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

diff --git a/libcli/security/sddl.c b/libcli/security/sddl.c
index 3b2cdfae17a69edb3601734e5e01690fdbe95fa0..6a9e6bdb22ccace138629eab5bb7aa9e2cadb83f 100644 (file)
--- a/libcli/security/sddl.c
+++ b/libcli/security/sddl.c
@@ -191,16 +191,47 @@ static struct dom_sid *sddl_decode_sid(TALLOC_CTX *mem_ctx, const char **sddlp,
 
        /* see if its in the numeric format */
        if (strncmp(sddl, "S-", 2) == 0) {
-               struct dom_sid *sid;
-               char *sid_str;
-               size_t len = strspn(sddl+2, "-0123456789");
-               sid_str = talloc_strndup(mem_ctx, sddl, len+2);
-               if (!sid_str) {
+               struct dom_sid *sid = NULL;
+               char *sid_str = NULL;
+                const char *end = NULL;
+                bool ok;
+               size_t len = strspn(sddl + 2, "-0123456789ABCDEFabcdefxX") + 2;
+               if (len < 5) { /* S-1-x */
                        return NULL;
                }
-               (*sddlp) += len+2;
-               sid = dom_sid_parse_talloc(mem_ctx, sid_str);
-               talloc_free(sid_str);
+               if (sddl[len - 1] == 'D' && sddl[len] == ':') {
+                       /*
+                        * we have run into the "D:" dacl marker, mistaking it
+                        * for a hex digit. There is no other way for this
+                        * pair to occur at the end of a SID in SDDL.
+                        */
+                       len--;
+               }
+
+               sid_str = talloc_strndup(mem_ctx, sddl, len);
+               if (sid_str == NULL) {
+                       return NULL;
+               }
+                sid = talloc(mem_ctx, struct dom_sid);
+                if (sid == NULL) {
+                       TALLOC_FREE(sid_str);
+                        return NULL;
+                };
+                ok = dom_sid_parse_endp(sid_str, sid, &end);
+                if (!ok) {
+                       DBG_WARNING("could not parse SID '%s'\n", sid_str);
+                       TALLOC_FREE(sid_str);
+                        TALLOC_FREE(sid);
+                        return NULL;
+                }
+               if (end - sid_str != len) {
+                       DBG_WARNING("trailing junk after SID '%s'\n", sid_str);
+                       TALLOC_FREE(sid_str);
+                        TALLOC_FREE(sid);
+                        return NULL;
+               }
+               TALLOC_FREE(sid_str);
+               (*sddlp) += len;
                return sid;
        }
 

diff --git a/selftest/knownfail.d/sid-strings b/selftest/knownfail.d/sid-strings
index 4fc0e4127b96b7d27135e2399fddcb76ae1beed2..9acc2b51a5a23db57cc77d210d92ff03d27ae3b5 100644 (file)
--- a/selftest/knownfail.d/sid-strings
+++ b/selftest/knownfail.d/sid-strings
@@ -68,14 +68,8 @@
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-000000000001-5-20-243.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-000000001-5-32-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-0.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-0x05-32-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-0x5-0x20-0x243.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-0x50000000-32-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-0x500000000-32-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-0xABcDef123-0xABCDef-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-22.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-281474976710656-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-5-0x20-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-5-3.2-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-5-32--579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_S-1-5-32-.579.ad_dc
@@ -84,12 +78,6 @@
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-000000000001-5-20-243.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-000000001-5-32-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-0.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-0x05-32-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-0x5-0x20-0x243.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-0x50000000-32-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-0x500000000-32-579.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-0xABcDef123-0xABCDef-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-22.ad_dc
-^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_S-1-5-0x20-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_internal_s-1-5-32-579.ad_dc
 ^samba.tests.sid_strings.+.SidStringsThatStartWithS.test_sid_string_s-1-5-32-579.ad_dc