Attachment 14857 Details for Bug 13773 – Proposed patch for V4.5 V2

[patch] Proposed patch for V4.5 V2

CVE-2019-3824-master-v4-5-02.patch (text/plain), 10.33 KB, created by Gary Lockyer on 2019-02-20 20:30:27 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2019-02-20 20:30:27 UTC

Size: 10.33 KB

patch

obsolete

>From 27c9e6cfb3492830423bc21a49506452ac98fbe0 Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn@fedoraproject.org>
>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 <lslebodn@fedoraproject.org>
>---
> 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 <abartlet@samba.org>
>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 <abartlet@samba.org>
>---
> 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 <abartlet@samba.org>
>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 <abartlet@samba.org>
>---
> 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 <gary@catalyst.net.nz>
>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 <gary@catalyst.net.nz>
>---
> 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 <gary@catalyst.net.nz>
>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 <gary@catalyst.net.nz>
>---
> 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 <gary@catalyst.net.nz>
>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 <gary@catalyst.net.nz>
>---
> 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
>

Flags:

abartlet: review+

Actions: View

Attachments on bug 13773: 14814 | 14815 | 14819 | 14845 | 14852 | 14853 | 14854 | 14855 | 14856 | 14857 | 14858 | 14859