Attachment 15598 Details for Bug 14138 – patch for Samba 4.9 (cherry-picked from master patch) v6

[patch] patch for Samba 4.9 (cherry-picked from master patch) v6

CVE-2019-14861-dnsserver-4.09-06.patch (text/plain), 18.75 KB, created by Andrew Bartlett on 2019-11-03 21:39:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2019-11-03 21:39:23 UTC

Size: 18.75 KB

patch

obsolete

>From 3c9c2c53ebaad0057fb5c346452cda9ce174185b 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/5] 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 7264a290ef2..14ce308e38f 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 2a6747c7237d176b1566f3f8e68f93e14b3c3b90 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/5] 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 59e29f029a6..f991f4042e3 100644
>--- a/source4/rpc_server/dnsserver/dnsdata.c
>+++ b/source4/rpc_server/dnsserver/dnsdata.c
>@@ -795,10 +795,11 @@ struct dns_tree *dns_build_tree(TALLOC_CTX *mem_ctx, const char *name, struct ld
> 	for (i=0; i<res->count; i++) {
> 		ptr = ldb_msg_find_attr_as_string(res->msgs[i], "name", NULL);
> 
>-		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 3ca03e7d1a1239a191efcc9aefa6d1955037c46e 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/5] 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 353754f9261..c0bf3425dae 100644
>--- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
>+++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
>@@ -1733,6 +1733,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;
>@@ -1749,6 +1750,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);
>@@ -1756,6 +1758,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);
>@@ -1769,16 +1772,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 f991f4042e3..acdb87752f8 100644
>--- a/source4/rpc_server/dnsserver/dnsdata.c
>+++ b/source4/rpc_server/dnsserver/dnsdata.c
>@@ -1052,8 +1052,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;
>@@ -1064,21 +1064,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 93f1d72f2ef..690ab87ed10 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 54bac874e108bb4b396f6dc0cd022124573513f2 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/5] 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 14ce308e38f..a9b8a4ace91 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
>
>
>From a87c95902e1e00eb1bbceef2fc21d65b146d2b77 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Fri, 1 Nov 2019 06:53:56 +1300
>Subject: [PATCH 5/5] s4-torture: Reduce flapping in
> SambaToolDrsTests.test_samba_tool_replicate_local
>
>This test often flaps in Samba 4.9 (where more tests and DCs run in the environment)
>with obj_1 being 3.  This is quite OK, we just need to see some changes get
>replicated, not 0 changes.
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>(cherry picked from commit 4ae0f9ce0f5ada99cf1d236377e5a1234c879ae3)
>---
> source4/torture/drs/python/samba_tool_drs.py | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/source4/torture/drs/python/samba_tool_drs.py b/source4/torture/drs/python/samba_tool_drs.py
>index 29c6016471c..74fa433ce59 100644
>--- a/source4/torture/drs/python/samba_tool_drs.py
>+++ b/source4/torture/drs/python/samba_tool_drs.py
>@@ -209,6 +209,7 @@ class SambaToolDrsTests(drs_base.DrsBaseTestCase):
>         self._disable_inbound_repl(self.dnsname_dc1)
>         self._disable_inbound_repl(self.dnsname_dc2)
> 
>+        self._net_drs_replicate(DC=self.dnsname_dc2, fromDC=self.dnsname_dc1)
>         self._net_drs_replicate(DC=self.dnsname_dc1, fromDC=self.dnsname_dc2)
> 
>         # add an object with link on dc1
>@@ -231,7 +232,7 @@ class SambaToolDrsTests(drs_base.DrsBaseTestCase):
> 
>         (obj_1, link_1) = get_num_obj_links(out)
> 
>-        self.assertEqual(obj_1, 2)
>+        self.assertGreaterEqual(obj_1, 2)
>         self.assertEqual(link_1, 1)
> 
>         # pull that change with --local into local db from dc2: shouldn't send link or object
>-- 
>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