From 27c9e6cfb3492830423bc21a49506452ac98fbe0 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik Date: Fri, 18 Jan 2019 16:37:24 +0100 Subject: [PATCH 1/6] CVE-2019-3824 ldb: Out of bound read in ldb_wildcard_compare There is valgrind error in few tests tests/test-generic.sh 91 echo "Test wildcard match" 92 $VALGRIND ldbadd $LDBDIR/tests/test-wildcard.ldif || exit 1 93 $VALGRIND ldbsearch '(cn=test*multi)' || exit 1 95 $VALGRIND ldbsearch '(cn=*test_multi)' || exit 1 97 $VALGRIND ldbsearch '(cn=test*multi*test*multi)' || exit 1 e.g. ==3098== Memcheck, a memory error detector ==3098== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==3098== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info ==3098== Command: ./bin/ldbsearch (cn=test*multi) ==3098== ==3098== Invalid read of size 1 ==3098== at 0x483CEE7: memchr (vg_replace_strmem.c:890) ==3098== by 0x49A9073: memmem (in /usr/lib64/libc-2.28.9000.so) ==3098== by 0x485DFE9: ldb_wildcard_compare (ldb_match.c:313) ==3098== by 0x485DFE9: ldb_match_substring (ldb_match.c:360) ==3098== by 0x485DFE9: ldb_match_message (ldb_match.c:572) ==3098== by 0x558F8FA: search_func (ldb_kv_search.c:549) ==3098== by 0x48C78CA: ??? (in /usr/lib64/libtdb.so.1.3.17) ==3098== by 0x48C7A60: tdb_traverse_read (in /usr/lib64/libtdb.so.1.3.17) ==3098== by 0x557B7C4: ltdb_traverse_fn (ldb_tdb.c:274) ==3098== by 0x558FBFA: ldb_kv_search_full (ldb_kv_search.c:594) ==3098== by 0x558FBFA: ldb_kv_search (ldb_kv_search.c:854) ==3098== by 0x558E497: ldb_kv_callback (ldb_kv.c:1713) ==3098== by 0x48FCD58: tevent_common_invoke_timer_handler (in /usr/lib64/libtevent.so.0.9.38) ==3098== by 0x48FCEFD: tevent_common_loop_timer_delay (in /usr/lib64/libtevent.so.0.9.38) ==3098== by 0x48FE14A: ??? (in /usr/lib64/libtevent.so.0.9.38) ==3098== Address 0x4b4ab81 is 0 bytes after a block of size 129 alloc'd ==3098== at 0x483880B: malloc (vg_replace_malloc.c:309) ==3098== by 0x491048B: talloc_strndup (in /usr/lib64/libtalloc.so.2.1.15) ==3098== by 0x48593CA: ldb_casefold_default (ldb_utf8.c:59) ==3098== by 0x485F68D: ldb_handler_fold (attrib_handlers.c:64) ==3098== by 0x485DB88: ldb_wildcard_compare (ldb_match.c:257) ==3098== by 0x485DB88: ldb_match_substring (ldb_match.c:360) ==3098== by 0x485DB88: ldb_match_message (ldb_match.c:572) ==3098== by 0x558F8FA: search_func (ldb_kv_search.c:549) ==3098== by 0x48C78CA: ??? (in /usr/lib64/libtdb.so.1.3.17) ==3098== by 0x48C7A60: tdb_traverse_read (in /usr/lib64/libtdb.so.1.3.17) ==3098== by 0x557B7C4: ltdb_traverse_fn (ldb_tdb.c:274) ==3098== by 0x558FBFA: ldb_kv_search_full (ldb_kv_search.c:594) ==3098== by 0x558FBFA: ldb_kv_search (ldb_kv_search.c:854) ==3098== by 0x558E497: ldb_kv_callback (ldb_kv.c:1713) ==3098== by 0x48FCD58: tevent_common_invoke_timer_handler (in /usr/lib64/libtevent.so.0.9.38) ==3098== # record 1 dn: cn=test_multi_test_multi_test_multi,o=University of Michigan,c=TEST cn: test_multi_test_multi_test_multi description: test multi wildcards matching objectclass: person sn: multi_test name: test_multi_test_multi_test_multi distinguishedName: cn=test_multi_test_multi_test_multi,o=University of Michiga n,c=TEST # returned 1 records # 1 entries # 0 referrals BUG: https://bugzilla.samba.org/show_bug.cgi?id=13773 Signed-off-by: Lukas Slebodnik --- lib/ldb/common/ldb_match.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index e83ad637dff..71f8b9671d5 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -308,9 +308,10 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, if (p == NULL) goto mismatch; if ( (! tree->u.substring.chunks[c + 1]) && (! tree->u.substring.end_with_wildcard) ) { uint8_t *g; + uint8_t *end = val.data + val.length; do { /* greedy */ g = memmem(p + cnk.length, - val.length - (p - val.data), + end - (p + cnk.length), (const uint8_t *)cnk.data, cnk.length); if (g) p = g; -- 2.17.1 From 23f386668b4e2b28f69bf6d227cc2210562afae2 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 4 Feb 2019 11:22:34 +1300 Subject: [PATCH 2/6] CVE-2019-3824 ldb: Extra comments to clarify no pointer wrap in wildcard processing BUG: https://bugzilla.samba.org/show_bug.cgi?id=13773 Signed-off-by: Andrew Bartlett --- lib/ldb/common/ldb_match.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index 71f8b9671d5..b49085286a2 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -306,12 +306,33 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, p = memmem((const void *)val.data,val.length, (const void *)cnk.data, cnk.length); if (p == NULL) goto mismatch; + + /* + * At this point we know cnk.length <= val.length as + * otherwise there could be no match + */ + if ( (! tree->u.substring.chunks[c + 1]) && (! tree->u.substring.end_with_wildcard) ) { uint8_t *g; uint8_t *end = val.data + val.length; do { /* greedy */ - g = memmem(p + cnk.length, - end - (p + cnk.length), + + /* + * haystack is a valid pointer in val + * because the memmem() can only + * succeed if the needle (cnk.length) + * is <= haystacklen + * + * p will be a pointer at least + * cnk.length from the end of haystack + */ + uint8_t *haystack + = p + cnk.length; + size_t haystacklen + = end - (haystack); + + g = memmem(haystack, + haystacklen, (const uint8_t *)cnk.data, cnk.length); if (g) p = g; -- 2.17.1 From fb005204b4248012f49e31e4c2c452a667f3fc87 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 4 Feb 2019 11:22:50 +1300 Subject: [PATCH 3/6] CVE-2019-3824 ldb: Improve code style and layout in wildcard processing BUG: https://bugzilla.samba.org/show_bug.cgi?id=13773 Signed-off-by: Andrew Bartlett --- lib/ldb/common/ldb_match.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index b49085286a2..e332ba0da53 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -333,9 +333,11 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, g = memmem(haystack, haystacklen, - (const uint8_t *)cnk.data, - cnk.length); - if (g) p = g; + (const uint8_t *)cnk.data, + cnk.length); + if (g) { + p = g; + } } while(g); } val.length = val.length - (p - (uint8_t *)(val.data)) - cnk.length; -- 2.17.1 From 832993c20c96e51faa2e25f2d0581737fb2bfabf Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 19 Feb 2019 10:25:24 +1300 Subject: [PATCH 4/6] CVE-2019-3824 ldb: ldb_parse_tree use talloc_zero Initialise the created ldb_parse_tree with talloc_zero, this ensures that it is correctly initialised if inadvertently passed to a function expecting a different operation type. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13773 Signed-off-by: Gary Lockyer --- lib/ldb/common/ldb_parse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_parse.c b/lib/ldb/common/ldb_parse.c index 5fa5a74afa9..db420091311 100644 --- a/lib/ldb/common/ldb_parse.c +++ b/lib/ldb/common/ldb_parse.c @@ -389,7 +389,7 @@ static struct ldb_parse_tree *ldb_parse_simple(TALLOC_CTX *mem_ctx, const char * struct ldb_parse_tree *ret; enum ldb_parse_op filtertype; - ret = talloc(mem_ctx, struct ldb_parse_tree); + ret = talloc_zero(mem_ctx, struct ldb_parse_tree); if (!ret) { errno = ENOMEM; return NULL; -- 2.17.1 From 52e26aaf19720ae73a9a2c683b8ddfbfd765ce1c Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 19 Feb 2019 10:26:25 +1300 Subject: [PATCH 5/6] CVE-2019-3824 ldb: wildcard_match check tree operation Check the operation type of the passed parse tree, and return LDB_INAPPROPRIATE_MATCH if the operation is not LDB_OP_SUBSTRING. A query of "attribute=*" gets parsed as LDB_OP_PRESENT, checking the operation and failing ldb_wildcard_match should help prevent confusion writing tests. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13773 Signed-off-by: Gary Lockyer --- lib/ldb/common/ldb_match.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index e332ba0da53..e529b5e26a5 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -244,6 +244,11 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, uint8_t *save_p = NULL; unsigned int c = 0; + if (tree->operation != LDB_OP_SUBSTRING) { + *matched = false; + return LDB_ERR_INAPPROPRIATE_MATCHING; + } + a = ldb_schema_attribute_by_name(ldb, tree->u.substring.attr); if (!a) { return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX; -- 2.17.1 From 146bf02a8ecc81177011dd1075b37883ef0bd5e1 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 19 Feb 2019 10:26:56 +1300 Subject: [PATCH 6/6] CVE-2019-3824 ldb: wildcard_match end of data check ldb_handler_copy and ldb_val_dup over allocate by one and add a trailing '\0' to the data, to make them safe to use the C string functions on. However testing for the trailing '\0' is not the correct way to test for the end of a value, the length should be checked instead. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13773 Signed-off-by: Gary Lockyer --- lib/ldb/common/ldb_match.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index e529b5e26a5..79894d0c89b 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -353,7 +353,7 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, } /* last chunk may not have reached end of string */ - if ( (! tree->u.substring.end_with_wildcard) && (*(val.data) != 0) ) goto mismatch; + if ( (! tree->u.substring.end_with_wildcard) && (val.length != 0) ) goto mismatch; talloc_free(save_p); *matched = true; return LDB_SUCCESS; -- 2.17.1