The Samba-Bugzilla – Attachment 15582 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) v4
CVE-2019-14861-dnsserver-4.10-04.patch (text/plain), 17.07 KB, created by
Andrew Bartlett
on 2019-10-30 02:19:44 UTC
(
hide
)
Description:
patch for Samba 4.10 (cherry-picked from master patch) v4
Filename:
MIME Type:
Creator:
Andrew Bartlett
Created:
2019-10-30 02:19:44 UTC
Size:
17.07 KB
patch
obsolete
>From 8421f69ef18ff60c07f60678bd1fade974dc9008 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 116dde04b80ac739a5b8d6c6698e57509cf029fa 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 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 0f29e883291731cedcea3c852f991812413c447d 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 | 20 +++++++++++-------- > source4/rpc_server/dnsserver/dnsdata.c | 19 ++---------------- > source4/rpc_server/dnsserver/dnsserver.h | 4 ++-- > 3 files changed, 16 insertions(+), 27 deletions(-) > >diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c >index f8a8f0bae61..663b8a3f768 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,17 @@ 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 componet 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 503b5678ee7b118ecda09de182ec5631cab66424 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 >
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
:
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