From 3c9c2c53ebaad0057fb5c346452cda9ce174185b Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 --- 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 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 --- 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; icount; 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 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 --- .../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 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 --- 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 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 Reviewed-by: Douglas Bagnall (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