The Samba-Bugzilla – Attachment 15597 Details for
Bug 14138
CVE-2019-14861 [SECURITY] DNSServer RPC server crash
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
patch for Samba 4.10 (cherry-picked from master patch) v6
CVE-2019-14861-dnsserver-4.10-06.patch (text/plain), 18.91 KB, created by
Andrew Bartlett
on 2019-11-03 21:38:33 UTC
(
hide
)
Description:
patch for Samba 4.10 (cherry-picked from master patch) v6
Filename:
MIME Type:
Creator:
Andrew Bartlett
Created:
2019-11-03 21:38:33 UTC
Size:
18.91 KB
patch
obsolete
>From a44a310ba990ccbaf70f2ee08ecb55c1045725a7 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. > >(patch differs from master patch due to addtional flapping entry >for python2) > >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 | 3 + > 2 files changed, 104 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..bf2dc99ce11 >--- /dev/null >+++ b/selftest/flapping.d/dnsserver >@@ -0,0 +1,3 @@ >+# This is not stable in samba due to a bug >+^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_enum_is_sorted_children >+^samba.tests.dcerpc.dnsserver.python2.samba.tests.dcerpc.dnsserver.DnsserverTests.test_enum_is_sorted_children >-- >2.17.1 > > >From af9133c9848c90b1070f2a56dd86cde9ef204e58 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 86b6ee3e42f5c88a9cb7058baf8a087ed8a66bb0 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 f8a8f0bae61..910de9a1efd 100644 >--- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c >+++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c >@@ -1758,6 +1758,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; >@@ -1774,6 +1775,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); >@@ -1781,6 +1783,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); >@@ -1794,16 +1797,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 747ab8b4f294286c58a090796549ec79b394c29f 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 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 > > >From 8677b1cb0a4383097ee95e778473529cfc064f5d 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 76cc86f832e..988f1dc7a3c 100644 >--- a/source4/torture/drs/python/samba_tool_drs.py >+++ b/source4/torture/drs/python/samba_tool_drs.py >@@ -210,6 +210,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 >@@ -232,7 +233,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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
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