The branch, master has been updated via a699256f438 lib ldb: Limit depth of ldb_parse_tree via ac800011006 lib util ASN.1: Panic on ASN.1 tag mismatch via 80d9ea13867 s4 cldap server tests: request size limit tests via aa7cc50ae2a s4 ldap server tests: request size limit tests from e907f002a7f Fix clang 9 for-loop-analysis warnings
https://git.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit a699256f438527455aaff6c73c88ee87ac7083ef Author: Gary Lockyer <g...@catalyst.net.nz> Date: Tue Apr 21 15:37:40 2020 +1200 lib ldb: Limit depth of ldb_parse_tree Limit the number of nested conditionals allowed by ldb_parse tree to 128, to avoid potential stack overflow issues. Credit Oss-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=19508 Signed-off-by: Gary Lockyer <g...@catalyst.net.nz> Reviewed-by: Andrew Bartlett <abart...@samba.org> Autobuild-User(master): Gary Lockyer <g...@samba.org> Autobuild-Date(master): Sun May 10 23:21:08 UTC 2020 on sn-devel-184 commit ac8000110064055a986c0c1fee896fddd302114b Author: Gary Lockyer <g...@catalyst.net.nz> Date: Wed Apr 29 11:09:34 2020 +1200 lib util ASN.1: Panic on ASN.1 tag mismatch If the ASN.1 depth is zero in asn1_end_tag, call smb_panic. Rather than ignoring the condition. Signed-off-by: Gary Lockyer <g...@catalyst.net.nz> Reviewed-by: Andrew Bartlett <abart...@samba.org> commit 80d9ea1386707c33783b76e5bc4ec37523424439 Author: Gary Lockyer <g...@catalyst.net.nz> Date: Wed Apr 29 09:17:52 2020 +1200 s4 cldap server tests: request size limit tests Add tests for packet size limits. Signed-off-by: Gary Lockyer <g...@catalyst.net.nz> Reviewed-by: Andrew Bartlett <abart...@samba.org> commit aa7cc50ae2a99d926e495efd2982e4c656b87bf3 Author: Gary Lockyer <g...@catalyst.net.nz> Date: Tue Apr 28 13:55:04 2020 +1200 s4 ldap server tests: request size limit tests Extra tests for ldap maximum request size limits. Signed-off-by: Gary Lockyer <g...@catalyst.net.nz> Reviewed-by: Andrew Bartlett <abart...@samba.org> ----------------------------------------------------------------------- Summary of changes: lib/ldb/common/ldb_parse.c | 72 +++- lib/ldb/tests/ldb_parse_test.c | 83 ++++- lib/util/asn1.c | 5 +- python/samba/tests/ldap_raw.py | 729 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 860 insertions(+), 29 deletions(-) Changeset truncated at 500 lines: diff --git a/lib/ldb/common/ldb_parse.c b/lib/ldb/common/ldb_parse.c index 452c5830ed5..7e15206b168 100644 --- a/lib/ldb/common/ldb_parse.c +++ b/lib/ldb/common/ldb_parse.c @@ -43,6 +43,16 @@ #include "ldb_private.h" #include "system/locale.h" +/* + * Maximum depth of the filter parse tree, the value chosen is small enough to + * avoid triggering ASAN stack overflow checks. But large enough to be useful. + * + * On Windows clients the maximum number of levels of recursion allowed is 100. + * In the LDAP server, Windows restricts clients to 512 nested + * (eg) OR statements. + */ +#define LDB_MAX_PARSE_TREE_DEPTH 128 + static int ldb_parse_hex2char(const char *x) { if (isxdigit(x[0]) && isxdigit(x[1])) { @@ -231,7 +241,11 @@ static struct ldb_val **ldb_wildcard_decode(TALLOC_CTX *mem_ctx, const char *str return ret; } -static struct ldb_parse_tree *ldb_parse_filter(TALLOC_CTX *mem_ctx, const char **s); +static struct ldb_parse_tree *ldb_parse_filter( + TALLOC_CTX *mem_ctx, + const char **s, + unsigned depth, + unsigned max_depth); /* @@ -498,7 +512,11 @@ static struct ldb_parse_tree *ldb_parse_simple(TALLOC_CTX *mem_ctx, const char * <or> ::= '|' <filterlist> <filterlist> ::= <filter> | <filter> <filterlist> */ -static struct ldb_parse_tree *ldb_parse_filterlist(TALLOC_CTX *mem_ctx, const char **s) +static struct ldb_parse_tree *ldb_parse_filterlist( + TALLOC_CTX *mem_ctx, + const char **s, + unsigned depth, + unsigned max_depth) { struct ldb_parse_tree *ret, *next; enum ldb_parse_op op; @@ -533,7 +551,8 @@ static struct ldb_parse_tree *ldb_parse_filterlist(TALLOC_CTX *mem_ctx, const ch return NULL; } - ret->u.list.elements[0] = ldb_parse_filter(ret->u.list.elements, &p); + ret->u.list.elements[0] = + ldb_parse_filter(ret->u.list.elements, &p, depth, max_depth); if (!ret->u.list.elements[0]) { talloc_free(ret); return NULL; @@ -547,7 +566,8 @@ static struct ldb_parse_tree *ldb_parse_filterlist(TALLOC_CTX *mem_ctx, const ch break; } - next = ldb_parse_filter(ret->u.list.elements, &p); + next = ldb_parse_filter( + ret->u.list.elements, &p, depth, max_depth); if (next == NULL) { /* an invalid filter element */ talloc_free(ret); @@ -576,7 +596,11 @@ static struct ldb_parse_tree *ldb_parse_filterlist(TALLOC_CTX *mem_ctx, const ch /* <not> ::= '!' <filter> */ -static struct ldb_parse_tree *ldb_parse_not(TALLOC_CTX *mem_ctx, const char **s) +static struct ldb_parse_tree *ldb_parse_not( + TALLOC_CTX *mem_ctx, + const char **s, + unsigned depth, + unsigned max_depth) { struct ldb_parse_tree *ret; const char *p = *s; @@ -593,7 +617,7 @@ static struct ldb_parse_tree *ldb_parse_not(TALLOC_CTX *mem_ctx, const char **s) } ret->operation = LDB_OP_NOT; - ret->u.isnot.child = ldb_parse_filter(ret, &p); + ret->u.isnot.child = ldb_parse_filter(ret, &p, depth, max_depth); if (!ret->u.isnot.child) { talloc_free(ret); return NULL; @@ -608,7 +632,11 @@ static struct ldb_parse_tree *ldb_parse_not(TALLOC_CTX *mem_ctx, const char **s) parse a filtercomp <filtercomp> ::= <and> | <or> | <not> | <simple> */ -static struct ldb_parse_tree *ldb_parse_filtercomp(TALLOC_CTX *mem_ctx, const char **s) +static struct ldb_parse_tree *ldb_parse_filtercomp( + TALLOC_CTX *mem_ctx, + const char **s, + unsigned depth, + unsigned max_depth) { struct ldb_parse_tree *ret; const char *p = *s; @@ -617,15 +645,15 @@ static struct ldb_parse_tree *ldb_parse_filtercomp(TALLOC_CTX *mem_ctx, const ch switch (*p) { case '&': - ret = ldb_parse_filterlist(mem_ctx, &p); + ret = ldb_parse_filterlist(mem_ctx, &p, depth, max_depth); break; case '|': - ret = ldb_parse_filterlist(mem_ctx, &p); + ret = ldb_parse_filterlist(mem_ctx, &p, depth, max_depth); break; case '!': - ret = ldb_parse_not(mem_ctx, &p); + ret = ldb_parse_not(mem_ctx, &p, depth, max_depth); break; case '(': @@ -641,21 +669,34 @@ static struct ldb_parse_tree *ldb_parse_filtercomp(TALLOC_CTX *mem_ctx, const ch return ret; } - /* <filter> ::= '(' <filtercomp> ')' */ -static struct ldb_parse_tree *ldb_parse_filter(TALLOC_CTX *mem_ctx, const char **s) +static struct ldb_parse_tree *ldb_parse_filter( + TALLOC_CTX *mem_ctx, + const char **s, + unsigned depth, + unsigned max_depth) { struct ldb_parse_tree *ret; const char *p = *s; + /* + * Check the depth of the parse tree, and reject the input if + * max_depth exceeded. This avoids stack overflow + * issues. + */ + if (depth > max_depth) { + return NULL; + } + depth++; + if (*p != '(') { return NULL; } p++; - ret = ldb_parse_filtercomp(mem_ctx, &p); + ret = ldb_parse_filtercomp(mem_ctx, &p, depth, max_depth); if (*p != ')') { return NULL; @@ -679,6 +720,8 @@ static struct ldb_parse_tree *ldb_parse_filter(TALLOC_CTX *mem_ctx, const char * */ struct ldb_parse_tree *ldb_parse_tree(TALLOC_CTX *mem_ctx, const char *s) { + unsigned depth = 0; + while (s && isspace((unsigned char)*s)) s++; if (s == NULL || *s == 0) { @@ -686,7 +729,8 @@ struct ldb_parse_tree *ldb_parse_tree(TALLOC_CTX *mem_ctx, const char *s) } if (*s == '(') { - return ldb_parse_filter(mem_ctx, &s); + return ldb_parse_filter( + mem_ctx, &s, depth, LDB_MAX_PARSE_TREE_DEPTH); } return ldb_parse_simple(mem_ctx, &s); diff --git a/lib/ldb/tests/ldb_parse_test.c b/lib/ldb/tests/ldb_parse_test.c index a739d7795d1..d7442b954ea 100644 --- a/lib/ldb/tests/ldb_parse_test.c +++ b/lib/ldb/tests/ldb_parse_test.c @@ -81,10 +81,91 @@ static void test_parse_filtertype(void **state) test_roundtrip(ctx, " ", "(|(objectClass=*)(distinguishedName=*))"); } +/* + * Test that a nested query with 128 levels of nesting is accepted + */ +static void test_nested_filter_eq_limit(void **state) +{ + struct test_ctx *ctx = + talloc_get_type_abort(*state, struct test_ctx); + + /* + * 128 nested clauses + */ + const char *nested_query = "" + "(|(!(|(&(|(|(|(|(|(|(|(|(|(|(|(|" + "(|(!(|(&(|(|(|(|(|(|(!(|(!(|(|(|" + "(|(!(|(&(|(|(&(|(|(|(|(|(!(!(!(|" + "(|(!(|(&(|(|(|(|(|(|(|(|(|(|(|(|" + "(|(!(|(&(|(|(|(!(|(|(&(|(|(|(|(|" + "(|(!(|(&(|(|(&(|(|(|(|(|(&(&(|(|" + "(|(!(|(&(|(|(|(|(|(|(!(|(|(|(|(|" + "(|(!(|(&(|(|(!(|(|(|(|(|(|(|(|(|" + "(a=b)" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))"; + + struct ldb_parse_tree *tree = ldb_parse_tree(ctx, nested_query); + + assert_non_null(tree); + /* + * Check that we get the same query back + */ + test_roundtrip(ctx, nested_query, nested_query); +} + +/* + * Test that a nested query with 129 levels of nesting is rejected. + */ +static void test_nested_filter_gt_limit(void **state) +{ + struct test_ctx *ctx = + talloc_get_type_abort(*state, struct test_ctx); + + /* + * 129 nested clauses + */ + const char *nested_query = "" + "(|(!(|(|(&(|(|(|(|(&(|(|(|(|(|(|" + "(|(!(|(|(&(|(|(|(|(|(|(|(|(|(|(|" + "(|(!(|(|(&(|(|(!(|(|(|(|(!(|(|(|" + "(|(!(|(|(&(|(|(|(|(|(|(|(|(|(|(|" + "(|(!(|(|(&(|(|(|(!(&(|(|(|(|(|(|" + "(|(!(|(|(&(|(|(|(|(|(|(|(|(|(|(|" + "(|(!(|(|(&(|(|(|(|(|(|(|(|(|(|(|" + "(|(!(|(|(&(|(|(|(|(|(|(|(|(&(|(|" + "(|" + "(a=b)" + ")" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))" + "))))))))))))))))"; + + struct ldb_parse_tree *tree = ldb_parse_tree(ctx, nested_query); + + assert_null(tree); +} + int main(int argc, const char **argv) { const struct CMUnitTest tests[] = { - cmocka_unit_test_setup_teardown(test_parse_filtertype, setup, teardown), + cmocka_unit_test_setup_teardown( + test_parse_filtertype, setup, teardown), + cmocka_unit_test_setup_teardown( + test_nested_filter_eq_limit, setup, teardown), + cmocka_unit_test_setup_teardown( + test_nested_filter_gt_limit, setup, teardown), }; cmocka_set_message_output(CM_OUTPUT_SUBUNIT); diff --git a/lib/util/asn1.c b/lib/util/asn1.c index 397f6b3c271..88d96d4544b 100644 --- a/lib/util/asn1.c +++ b/lib/util/asn1.c @@ -713,9 +713,10 @@ bool asn1_end_tag(struct asn1_data *data) { struct nesting *nesting; - if (data->depth > 0) { - data->depth--; + if (data->depth == 0) { + smb_panic("Unbalanced ASN.1 Tag nesting"); } + data->depth--; /* make sure we read it all */ if (asn1_tag_remaining(data) != 0) { data->has_error = true; diff --git a/python/samba/tests/ldap_raw.py b/python/samba/tests/ldap_raw.py index 334fabce230..f4ea97d120e 100644 --- a/python/samba/tests/ldap_raw.py +++ b/python/samba/tests/ldap_raw.py @@ -19,6 +19,7 @@ # import socket +import ssl import samba.tests from samba.tests import TestCase @@ -27,11 +28,29 @@ from samba.tests import TestCase # # LDAP Operations # -SEARCH = b'\x63' +DELETE = b'\x4a' +DELETE_RES = b'\x6b' + +# Bind +BIND = b'\x60' +BIND_RES = b'\x61' +SIMPLE_AUTH = b'\x80' +SASL_AUTH = b'\xa3' +# Search +SEARCH = b'\x63' +SEARCH_RES = b'\x64' EQUALS = b'\xa3' +# +# LDAP response codes. +# +SUCCESS = b'\x00' +OPERATIONS_ERROR = b'\x01' +INVALID_CREDENTIALS = b'\x31' +INVALID_DN_SYNTAX = b'\x22' + # # ASN.1 Element types # @@ -97,15 +116,52 @@ def encode_sequence(sequence): return encode_element(SEQUENCE, sequence) +def decode_element(data): + ''' + decode an ASN.1 element + ''' + if data is None: + return None + + if len(data) < 2: + return None + + ber_type = data[0:1] + enc = int.from_bytes(data[1:2], byteorder='big') + if enc & 0x80: + l_end = 2 + (enc & ~0x80) + length = int.from_bytes(data[2:l_end], byteorder='big') + element = data[l_end:l_end + length] + rest = data[l_end + length:] + else: + length = enc + element = data[2:2 + length] + rest = data[2 + length:] + + return (ber_type, length, element, rest) + + class RawLdapTest(TestCase): - """A raw Ldap Test case.""" + """ + A raw Ldap Test case. + The ldap connections are made over https on port 636 + + Uses the following environment variables: + SERVER + USERNAME + PASSWORD + DNSNAME + """ def setUp(self): super(RawLdapTest, self).setUp() self.host = samba.tests.env_get_var_value('SERVER') - self.port = 389 + self.port = 636 self.socket = None + self.user = samba.tests.env_get_var_value('USERNAME') + self.password = samba.tests.env_get_var_value('PASSWORD') + self.dns_name = samba.tests.env_get_var_value('DNSNAME') self.connect() def tearDown(self): @@ -120,13 +176,23 @@ class RawLdapTest(TestCase): self.socket = None def connect(self): - ''' Open a socket stream connection to the server ''' + ''' Establish an ldaps connection to the test server ''' + # + # Disable host name and certificate verification + context = ssl.create_default_context() + context.check_hostname = False + context.verify_mode = ssl.CERT_NONE + + sock = None try: - self.socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - self.socket.settimeout(10) - self.socket.connect((self.host, self.port)) + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.settimeout(10) + sock.connect((self.host, self.port)) + self.socket = context.wrap_socket(sock, server_hostname=self.host) except socket.error: - self.socket.close() + sock.close() + if self.socket is not None: + self.socket.close() raise def send(self, req): @@ -138,7 +204,7 @@ class RawLdapTest(TestCase): raise def recv(self, num_recv=0xffff, timeout=None): - ''' recv an array of bytes from the server ''' + ''' receive an array of bytes from the server ''' data = None try: if timeout is not None: @@ -158,10 +224,126 @@ class RawLdapTest(TestCase): raise return data + def bind(self): + ''' + Perform a simple bind + ''' + + user = self.user.encode('UTF8') + ou = self.dns_name.replace('.', ',dc=').encode('UTF8') + dn = b'cn=' + user + b',cn=users,dc=' + ou + + password = self.password.encode('UTF8') + + # Lets build an simple bind request + bind = encode_integer(3) # ldap version + bind += encode_string(dn) + bind += encode_element(SIMPLE_AUTH, password) + + bind_op = encode_element(BIND, bind) + + msg_no = encode_integer(1) + packet = encode_sequence(msg_no + bind_op) + + self.send(packet) + data = self.recv() + self.assertIsNotNone(data) + + # + # Decode and validate the response + + # Should be a sequence + (ber_type, length, element, rest) = decode_element(data) + self.assertEqual(SEQUENCE.hex(), ber_type.hex()) + self.assertTrue(length > 0) + self.assertEqual(0, len(rest)) + + # message id should be 1 + (ber_type, length, element, rest) = decode_element(element) + self.assertEqual(INTEGER.hex(), ber_type.hex()) + msg_no = int.from_bytes(element, byteorder='big') + self.assertEqual(1, msg_no) + self.assertGreater(len(rest), 0) + + # Should have a Bind response element + (ber_type, length, element, rest) = decode_element(rest) + self.assertEqual(BIND_RES.hex(), ber_type.hex()) + self.assertEqual(0, len(rest)) + + # Check the response code + (ber_type, length, element, rest) = decode_element(element) + self.assertEqual(ENUMERATED.hex(), ber_type.hex()) + self.assertEqual(SUCCESS.hex(), element.hex()) + self.assertGreater(len(rest), 0) + + def test_decode_element(self): + ''' Tests for the decode_element method ''' + + # Boolean true value + data = b'\x01\x01\xff' + (ber_type, length, element, rest) = decode_element(data) + self.assertEqual(BOOLEAN.hex(), ber_type.hex()) + self.assertEqual(1, length) + self.assertEqual(b'\xff'.hex(), element.hex()) + self.assertEqual(0, len(rest)) + + # Boolean false value + data = b'\x01\x01\x00' + (ber_type, length, element, rest) = decode_element(data) + self.assertEqual(BOOLEAN.hex(), ber_type.hex()) + self.assertEqual(1, length) -- Samba Shared Repository