Attachment 15595 Details for Bug 14138 – patch for master (v6)

[patch] patch for master (v6)

CVE-2019-14861-dnsserver-master-06.patch (text/plain), 17.00 KB, created by Andrew Bartlett on 2019-11-03 21:37:25 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2019-11-03 21:37:25 UTC

Size: 17.00 KB

patch

obsolete

>From 00b25928390b2a24ce639df9579a0b4b3a7a2be7 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 29 Oct 2019 17:25:28 +1300
>Subject: [PATCH 1/4] CVE-2019-14861: s4-rpc/dnsserver: Confirm sort behaviour
> in dcesrv_DnssrvEnumRecords
>
>The sort behaviour for child records is not correct in Samba so
>we add a flapping entry.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>---
> python/samba/tests/dcerpc/dnsserver.py | 101 +++++++++++++++++++++++++
> selftest/flapping.d/dnsserver          |   2 +
> 2 files changed, 103 insertions(+)
> create mode 100644 selftest/flapping.d/dnsserver
>
>diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py
>index 0da9614d066..b2dffc924cb 100644
>--- a/python/samba/tests/dcerpc/dnsserver.py
>+++ b/python/samba/tests/dcerpc/dnsserver.py
>@@ -156,6 +156,107 @@ class DnsserverTests(RpcInterfaceTestCase):
>                                    None)
>         super(DnsserverTests, self).tearDown()
> 
>+    def test_enum_is_sorted(self):
>+        """
>+        Confirm the zone is sorted
>+        """
>+
>+        record_str = "192.168.50.50"
>+        record_type_str = "A"
>+        self.add_record(self.custom_zone, "atestrecord-1", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-2", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-3", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-4", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-0", record_type_str, record_str)
>+
>+        # This becomes an extra A on the zone itself by server-side magic
>+        self.add_record(self.custom_zone, self.custom_zone, record_type_str, record_str)
>+
>+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
>+                                                 0,
>+                                                 self.server,
>+                                                 self.custom_zone,
>+                                                 "@",
>+                                                 None,
>+                                                 self.record_type_int(record_type_str),
>+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
>+                                                 None,
>+                                                 None)
>+
>+        self.assertEqual(len(result.rec), 6)
>+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
>+        self.assertEqual(result.rec[1].dnsNodeName.str, "atestrecord-0")
>+        self.assertEqual(result.rec[2].dnsNodeName.str, "atestrecord-1")
>+        self.assertEqual(result.rec[3].dnsNodeName.str, "atestrecord-2")
>+        self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
>+        self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
>+
>+    def test_enum_is_sorted_children_prefix_first(self):
>+        """
>+        Confirm the zone returns the selected prefix first but no more
>+        as Samba is flappy for the full sort
>+        """
>+
>+        record_str = "192.168.50.50"
>+        record_type_str = "A"
>+        self.add_record(self.custom_zone, "atestrecord-1.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-2.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-3.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-4.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-0.a.b", record_type_str, record_str)
>+
>+        # Not expected to be returned
>+        self.add_record(self.custom_zone, "atestrecord-0.b.b", record_type_str, record_str)
>+
>+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
>+                                                 0,
>+                                                 self.server,
>+                                                 self.custom_zone,
>+                                                 "a.b",
>+                                                 None,
>+                                                 self.record_type_int(record_type_str),
>+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
>+                                                 None,
>+                                                 None)
>+
>+        self.assertEqual(len(result.rec), 6)
>+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
>+
>+    def test_enum_is_sorted_children(self):
>+        """
>+        Confirm the zone is sorted
>+        """
>+
>+        record_str = "192.168.50.50"
>+        record_type_str = "A"
>+        self.add_record(self.custom_zone, "atestrecord-1.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-2.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-3.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-4.a.b", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-0.a.b", record_type_str, record_str)
>+
>+        # Not expected to be returned
>+        self.add_record(self.custom_zone, "atestrecord-0.b.b", record_type_str, record_str)
>+
>+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
>+                                                 0,
>+                                                 self.server,
>+                                                 self.custom_zone,
>+                                                 "a.b",
>+                                                 None,
>+                                                 self.record_type_int(record_type_str),
>+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
>+                                                 None,
>+                                                 None)
>+
>+        self.assertEqual(len(result.rec), 6)
>+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
>+        self.assertEqual(result.rec[1].dnsNodeName.str, "atestrecord-0")
>+        self.assertEqual(result.rec[2].dnsNodeName.str, "atestrecord-1")
>+        self.assertEqual(result.rec[3].dnsNodeName.str, "atestrecord-2")
>+        self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
>+        self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
>+
>     # This test fails against Samba (but passes against Windows),
>     # because Samba does not return the record when we enum records.
>     # Records can be given DNS_RANK_NONE when the zone they are in
>diff --git a/selftest/flapping.d/dnsserver b/selftest/flapping.d/dnsserver
>new file mode 100644
>index 00000000000..9b33e8522a3
>--- /dev/null
>+++ b/selftest/flapping.d/dnsserver
>@@ -0,0 +1,2 @@
>+# This is not stable in samba due to a bug
>+^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_enum_is_sorted_children
>\ No newline at end of file
>-- 
>2.17.1
>
>
>From a9a94c651057f3c5a622214a033442d3b9360e22 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 21 Oct 2019 12:12:10 +1300
>Subject: [PATCH 2/4] CVE-2019-14861: s4-rpc_server: Remove special case for @
> in dns_build_tree()
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>---
> source4/rpc_server/dnsserver/dnsdata.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
>diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c
>index 2dc098a64a0..0739ea3f476 100644
>--- a/source4/rpc_server/dnsserver/dnsdata.c
>+++ b/source4/rpc_server/dnsserver/dnsdata.c
>@@ -801,10 +801,11 @@ struct dns_tree *dns_build_tree(TALLOC_CTX *mem_ctx, const char *name, struct ld
> 			goto failed;
> 		}
> 
>-		if (strcmp(ptr, "@") == 0) {
>-			base->data = res->msgs[i];
>-			continue;
>-		} else if (strcasecmp(ptr, name) == 0) {
>+		/*
>+		 * This might be the sub-domain in the zone being
>+		 * requested, or @ for the root of the zone
>+		 */
>+		if (strcasecmp(ptr, name) == 0) {
> 			base->data = res->msgs[i];
> 			continue;
> 		}
>-- 
>2.17.1
>
>
>From 260ea138ba5959c37b7e04529869de12ed81b392 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 29 Oct 2019 14:15:36 +1300
>Subject: [PATCH 3/4] CVE-2019-14861: s4-rpc/dnsserver: Avoid crash in
> ldb_qsort() via dcesrv_DnssrvEnumRecords)
>
>dns_name_compare() had logic to put @ and the top record in the tree being
>enumerated first, but if a domain had both then this would break the
>older qsort() implementation in ldb_qsort() and cause a read of memory
>before the base pointer.
>
>By removing this special case (not required as the base pointer
>is already seperatly located, no matter were it is in the
>returned records) the crash is avoided.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>---
> .../rpc_server/dnsserver/dcerpc_dnsserver.c   | 21 ++++++++++++-------
> source4/rpc_server/dnsserver/dnsdata.c        | 19 ++---------------
> source4/rpc_server/dnsserver/dnsserver.h      |  4 ++--
> 3 files changed, 17 insertions(+), 27 deletions(-)
>
>diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
>index 993e5dc4e56..b6389f2328a 100644
>--- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
>+++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
>@@ -1763,6 +1763,7 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
> 	struct DNS_RPC_RECORDS_ARRAY *recs;
> 	char **add_names = NULL;
> 	char *rname;
>+	const char *preference_name = NULL;
> 	int add_count = 0;
> 	int i, ret, len;
> 	WERROR status;
>@@ -1779,6 +1780,7 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
> 		ret = ldb_search(dsstate->samdb, tmp_ctx, &res, z->zone_dn,
> 				 LDB_SCOPE_ONELEVEL, attrs,
> 				 "(&(objectClass=dnsNode)(!(dNSTombstoned=TRUE)))");
>+		preference_name = "@";
> 	} else {
> 		char *encoded_name
> 			= ldb_binary_encode_string(tmp_ctx, name);
>@@ -1786,6 +1788,7 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
> 				 LDB_SCOPE_ONELEVEL, attrs,
> 				 "(&(objectClass=dnsNode)(|(name=%s)(name=*.%s))(!(dNSTombstoned=TRUE)))",
> 				 encoded_name, encoded_name);
>+		preference_name = name;
> 	}
> 	if (ret != LDB_SUCCESS) {
> 		talloc_free(tmp_ctx);
>@@ -1799,16 +1802,18 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate,
> 	recs = talloc_zero(mem_ctx, struct DNS_RPC_RECORDS_ARRAY);
> 	W_ERROR_HAVE_NO_MEMORY_AND_FREE(recs, tmp_ctx);
> 
>-	/* Sort the names, so that the first record is the parent record */
>-	ldb_qsort(res->msgs, res->count, sizeof(struct ldb_message *), name,
>-			(ldb_qsort_cmp_fn_t)dns_name_compare);
>+	/*
>+	 * Sort the names, so that the records are in order by the child
>+	 * component below "name".
>+	 *
>+	 * A full tree sort is not required, so we pass in "name" so
>+	 * we know which level to sort, as only direct children are
>+	 * eventually returned
>+	 */
>+	LDB_TYPESAFE_QSORT(res->msgs, res->count, name, dns_name_compare);
> 
> 	/* Build a tree of name components from dns name */
>-	if (strcasecmp(name, z->name) == 0) {
>-		tree = dns_build_tree(tmp_ctx, "@", res);
>-	} else {
>-		tree = dns_build_tree(tmp_ctx, name, res);
>-	}
>+	tree = dns_build_tree(tmp_ctx, preference_name, res);
> 	W_ERROR_HAVE_NO_MEMORY_AND_FREE(tree, tmp_ctx);
> 
> 	/* Find the parent record in the tree */
>diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c
>index 0739ea3f476..47d6f5d5c88 100644
>--- a/source4/rpc_server/dnsserver/dnsdata.c
>+++ b/source4/rpc_server/dnsserver/dnsdata.c
>@@ -1066,8 +1066,8 @@ WERROR dns_fill_records_array(TALLOC_CTX *mem_ctx,
> }
> 
> 
>-int dns_name_compare(const struct ldb_message **m1, const struct ldb_message **m2,
>-				char *search_name)
>+int dns_name_compare(struct ldb_message * const *m1, struct ldb_message * const *m2,
>+		     const char *search_name)
> {
> 	const char *name1, *name2;
> 	const char *ptr1, *ptr2;
>@@ -1078,21 +1078,6 @@ int dns_name_compare(const struct ldb_message **m1, const struct ldb_message **m
> 		return 0;
> 	}
> 
>-	/* '@' record and the search_name record gets preference */
>-	if (name1[0] == '@') {
>-		return -1;
>-	}
>-	if (search_name && strcasecmp(name1, search_name) == 0) {
>-		return -1;
>-	}
>-
>-	if (name2[0] == '@') {
>-		return 1;
>-	}
>-	if (search_name && strcasecmp(name2, search_name) == 0) {
>-		return 1;
>-	}
>-
> 	/* Compare the last components of names.
> 	 * If search_name is not NULL, compare the second last components of names */
> 	ptr1 = strrchr(name1, '.');
>diff --git a/source4/rpc_server/dnsserver/dnsserver.h b/source4/rpc_server/dnsserver/dnsserver.h
>index a8307ef836a..2e46e7c66a4 100644
>--- a/source4/rpc_server/dnsserver/dnsserver.h
>+++ b/source4/rpc_server/dnsserver/dnsserver.h
>@@ -188,8 +188,8 @@ struct DNS_ADDR_ARRAY *dns_addr_array_copy(TALLOC_CTX *mem_ctx, struct DNS_ADDR_
> int dns_split_name_components(TALLOC_CTX *mem_ctx, const char *name, char ***components);
> char *dns_split_node_name(TALLOC_CTX *mem_ctx, const char *node_name, const char *zone_name);
> 
>-int dns_name_compare(const struct ldb_message **m1, const struct ldb_message **m2,
>-			char *search_name);
>+int dns_name_compare(struct ldb_message * const *m1, struct ldb_message * const *m2,
>+		     const char *search_name);
> bool dns_record_match(struct dnsp_DnssrvRpcRecord *rec1, struct dnsp_DnssrvRpcRecord *rec2);
> 
> void dnsp_to_dns_copy(TALLOC_CTX *mem_ctx, struct dnsp_DnssrvRpcRecord *dnsp,
>-- 
>2.17.1
>
>
>From 29873a9e21f64d7d661aad057176bebcddfb5a68 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 30 Oct 2019 11:50:57 +1300
>Subject: [PATCH 4/4] CVE-2019-14861: Test to demonstrate the bug
>
>This test does not fail every time, but when it does it casues a segfault which
>takes out the rpc_server master process, as this hosts the dnsserver pipe.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14138
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>---
> python/samba/tests/dcerpc/dnsserver.py | 47 ++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
>diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py
>index b2dffc924cb..c6a150c876f 100644
>--- a/python/samba/tests/dcerpc/dnsserver.py
>+++ b/python/samba/tests/dcerpc/dnsserver.py
>@@ -191,6 +191,53 @@ class DnsserverTests(RpcInterfaceTestCase):
>         self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
>         self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
> 
>+    def test_enum_is_sorted_with_zone_dup(self):
>+        """
>+        Confirm the zone is sorted
>+        """
>+
>+        record_str = "192.168.50.50"
>+        record_type_str = "A"
>+        self.add_record(self.custom_zone, "atestrecord-1", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-2", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-3", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-4", record_type_str, record_str)
>+        self.add_record(self.custom_zone, "atestrecord-0", record_type_str, record_str)
>+
>+        # This triggers a bug in old Samba
>+        self.add_record(self.custom_zone, self.custom_zone + "1", record_type_str, record_str)
>+
>+        dn, record = self.get_record_from_db(self.custom_zone, self.custom_zone + "1")
>+
>+        new_dn = ldb.Dn(self.samdb, str(dn))
>+        new_dn.set_component(0, "dc", self.custom_zone)
>+        self.samdb.rename(dn, new_dn)
>+
>+        _, result = self.conn.DnssrvEnumRecords2(dnsserver.DNS_CLIENT_VERSION_LONGHORN,
>+                                                 0,
>+                                                 self.server,
>+                                                 self.custom_zone,
>+                                                 "@",
>+                                                 None,
>+                                                 self.record_type_int(record_type_str),
>+                                                 dnsserver.DNS_RPC_VIEW_AUTHORITY_DATA,
>+                                                 None,
>+                                                 None)
>+
>+        self.assertEqual(len(result.rec), 7)
>+        self.assertEqual(result.rec[0].dnsNodeName.str, "")
>+        self.assertEqual(result.rec[1].dnsNodeName.str, "atestrecord-0")
>+        self.assertEqual(result.rec[2].dnsNodeName.str, "atestrecord-1")
>+        self.assertEqual(result.rec[3].dnsNodeName.str, "atestrecord-2")
>+        self.assertEqual(result.rec[4].dnsNodeName.str, "atestrecord-3")
>+        self.assertEqual(result.rec[5].dnsNodeName.str, "atestrecord-4")
>+
>+        # Windows doesn't reload the zone fast enough, but doesn't
>+        # have the bug anyway, it will sort last on both names (where
>+        # it should)
>+        if result.rec[6].dnsNodeName.str != (self.custom_zone + "1"):
>+            self.assertEqual(result.rec[6].dnsNodeName.str, self.custom_zone)
>+
>     def test_enum_is_sorted_children_prefix_first(self):
>         """
>         Confirm the zone returns the selected prefix first but no more
>-- 
>2.17.1
>

Flags:

dbagnall: review+
abartlet: ci-passed+

Actions: View

Attachments on bug 14138: 15560 | 15574 | 15579 | 15580 | 15581 | 15582 | 15583 | 15584 | 15590 | 15591 | 15595 | 15596 | 15597 | 15598 | 15647 | 15649 | 15674