The Samba-Bugzilla – Attachment 14717 Details for
Bug 13600
[SECURITY] CVE-2018-14629 CNAME loops in Samba AD DC DNS server
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
combined fix, regression fix and test patch for 4.5
CVE-2018-14629-v4-5-v2.patch (text/plain), 17.62 KB, created by
Andrew Bartlett
on 2018-12-05 04:12:19 UTC
(
hide
)
Description:
combined fix, regression fix and test patch for 4.5
Filename:
MIME Type:
Creator:
Andrew Bartlett
Created:
2018-12-05 04:12:19 UTC
Size:
17.62 KB
patch
obsolete
>From 6d3da1eb00a31e7b0a30415d81c1ac4508da2aad Mon Sep 17 00:00:00 2001 >From: Aaron Haslett <aaronhaslett@catalyst.net.nz> >Date: Tue, 23 Oct 2018 17:25:51 +1300 >Subject: [PATCH 1/6] CVE-2018-14629 dns: CNAME loop prevention using counter > >Count number of answers generated by internal DNS query routine and stop at >20 to match Microsoft's loop prevention mechanism. > >(backport to Samba 4.5) > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13600 > >Signed-off-by: Aaron Haslett <aaronhaslett@catalyst.net.nz> >Reviewed-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Garming Sam <garming@catalyst.net.nz> >--- > python/samba/tests/dns.py | 23 +++++++++++++++++++++++ > selftest/knownfail.d/dns | 5 +++++ > source4/dns_server/dns_query.c | 6 ++++++ > 3 files changed, 34 insertions(+) > create mode 100644 selftest/knownfail.d/dns > >diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py >index babf898afd7..102269c7156 100644 >--- a/python/samba/tests/dns.py >+++ b/python/samba/tests/dns.py >@@ -896,6 +896,29 @@ class TestComplexQueries(DNSTest): > self.assertEquals(response.answers[1].name, name2) > self.assertEquals(response.answers[1].rdata, name3) > >+ def test_cname_loop(self): >+ cname1 = "cnamelooptestrec." + self.get_dns_domain() >+ cname2 = "cnamelooptestrec2." + self.get_dns_domain() >+ cname3 = "cnamelooptestrec3." + self.get_dns_domain() >+ self.make_dns_update(cname1, cname2, dnsp.DNS_TYPE_CNAME) >+ self.make_dns_update(cname2, cname3, dnsp.DNS_TYPE_CNAME) >+ self.make_dns_update(cname3, cname1, dnsp.DNS_TYPE_CNAME) >+ >+ p = self.make_name_packet(dns.DNS_OPCODE_QUERY) >+ questions = [] >+ >+ q = self.make_name_question(cname1, >+ dns.DNS_QTYPE_A, >+ dns.DNS_QCLASS_IN) >+ questions.append(q) >+ self.finish_name_packet(p, questions) >+ >+ response = self.dns_transaction_udp(p, host=self.server_ip) >+ >+ max_recursion_depth = 20 >+ self.assertEquals(len(response.answers), max_recursion_depth) >+ >+ > class TestInvalidQueries(DNSTest): > > def test_one_a_query(self): >diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns >new file mode 100644 >index 00000000000..916afc1af85 >--- /dev/null >+++ b/selftest/knownfail.d/dns >@@ -0,0 +1,5 @@ >+# >+# rodc and vampire_dc require signed dns updates, so the test setup >+# fails, but the test does run on fl2003dc >+^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(rodc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(vampire_dc:local\) >diff --git a/source4/dns_server/dns_query.c b/source4/dns_server/dns_query.c >index 3e9359ee666..0c26f9f8fb5 100644 >--- a/source4/dns_server/dns_query.c >+++ b/source4/dns_server/dns_query.c >@@ -40,6 +40,7 @@ > > #undef DBGC_CLASS > #define DBGC_CLASS DBGC_DNS >+#define MAX_Q_RECURSION_DEPTH 20 > > struct forwarder_string { > const char *forwarder; >@@ -470,6 +471,11 @@ static struct tevent_req *handle_dnsrpcrec_send( > state->answers = answers; > state->nsrecs = nsrecs; > >+ if (talloc_array_length(*answers) >= MAX_Q_RECURSION_DEPTH) { >+ tevent_req_done(req); >+ return tevent_req_post(req, ev); >+ } >+ > resolve_cname = ((rec->wType == DNS_TYPE_CNAME) && > ((question->question_type == DNS_QTYPE_A) || > (question->question_type == DNS_QTYPE_AAAA))); >-- >2.11.0 > > >From 5104d30c53be7161d03c853f480223b24115874a Mon Sep 17 00:00:00 2001 >From: Garming Sam <garming@catalyst.net.nz> >Date: Tue, 6 Sep 2016 10:48:57 +1200 >Subject: [PATCH 2/6] tests/dns_forwarder: Wait for port for 15 seconds > >Signed-off-by: Garming Sam <garming@catalyst.net.nz> >Reviewed-by: Andrew Bartlett <abartlet@samba.org> >(cherry picked from commit 668e4e4a436756d73d64790fd0a7e79fa4769ffe) >--- > python/samba/tests/dns_forwarder.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/python/samba/tests/dns_forwarder.py b/python/samba/tests/dns_forwarder.py >index de36de9c979..1d598662a83 100644 >--- a/python/samba/tests/dns_forwarder.py >+++ b/python/samba/tests/dns_forwarder.py >@@ -185,8 +185,8 @@ class TestDnsForwarding(DNSTest): > host, str(port), id]) > self.subprocesses.append(p) > s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, 0) >- for i in xrange(30): >- time.sleep(0.01) >+ for i in xrange(300): >+ time.sleep(0.05) > s.connect((host, port)) > try: > s.send('timeout 0', 0) >-- >2.11.0 > > >From c66c90469efe920d8273df041c1c46698ffb1a21 Mon Sep 17 00:00:00 2001 >From: Garming Sam <garming@catalyst.net.nz> >Date: Thu, 7 Jul 2016 16:58:27 +1200 >Subject: [PATCH 3/6] tests/dns_forwarder: Check that the subprocess is still > living > >Signed-off-by: Garming Sam <garming@catalyst.net.nz> >Reviewed-by: Andrew Bartlett <abartlet@samba.org> >(cherry picked from commit ad3b3e978ebf0692580166f9deba0368a922362d) >--- > python/samba/tests/dns_forwarder.py | 4 ++++ > 1 file changed, 4 insertions(+) > >diff --git a/python/samba/tests/dns_forwarder.py b/python/samba/tests/dns_forwarder.py >index 1d598662a83..bef21f6bdaf 100644 >--- a/python/samba/tests/dns_forwarder.py >+++ b/python/samba/tests/dns_forwarder.py >@@ -193,6 +193,10 @@ class TestDnsForwarding(DNSTest): > except socket.error, e: > if e.errno in (errno.ECONNREFUSED, errno.EHOSTUNREACH): > continue >+ >+ if p.returncode is not None: >+ self.fail("Toy server has managed to die already!") >+ > return s > > def tearDown(self): >-- >2.11.0 > > >From dec2a328b3262b131f147c339fa17112f998ca39 Mon Sep 17 00:00:00 2001 >From: Garming Sam <garming@samba.org> >Date: Thu, 9 Jun 2016 03:52:38 +0200 >Subject: [PATCH 4/6] tests/dns_forwarder: Fail out with an assertion instead > OOB error > >Signed-off-by: Garming Sam <garming@catalyst.net.nz> >Reviewed-by: Andrew Bartlett <abartlet@samba.org> > >Autobuild-User(master): Garming Sam <garming@samba.org> >Autobuild-Date(master): Tue Sep 6 15:41:54 CEST 2016 on sn-devel-144 > >(cherry picked from commit 451907739cc14717c12875b88fbbe63a53e9cbec) >--- > python/samba/tests/dns_forwarder.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/python/samba/tests/dns_forwarder.py b/python/samba/tests/dns_forwarder.py >index bef21f6bdaf..51a86198b54 100644 >--- a/python/samba/tests/dns_forwarder.py >+++ b/python/samba/tests/dns_forwarder.py >@@ -466,8 +466,9 @@ class TestDnsForwarding(DNSTest): > try: > data = ad.recv(0xffff + 2, 0) > data = ndr.ndr_unpack(dns.name_packet, data) >- self.assertEqual('forwarder1', data.answers[0].rdata) > self.assert_dns_rcode_equals(data, dns.DNS_RCODE_OK) >+ self.assertEqual(len(data.answers), 1) >+ self.assertEqual('forwarder1', data.answers[0].rdata) > except socket.timeout: > self.fail("DNS server is too slow (timeout %s)" % timeout) > >-- >2.11.0 > > >From 860852fb04d2ea7cf1bbd8d9225a421861e8da2b Mon Sep 17 00:00:00 2001 >From: Stefan Metzmacher <metze@samba.org> >Date: Wed, 28 Nov 2018 15:21:56 +0100 >Subject: [PATCH 5/6] CVE-2018-14629 dns: fix CNAME loop prevention using > counter regression > >The loop prevention should only be done for CNAME records! > >Otherwise we truncate the answer records for A, AAAA or >SRV queries, which is a bad idea if you have more than 20 DCs. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13600 > >Signed-off-by: Stefan Metzmacher <metze@samba.org> >--- > source4/dns_server/dns_query.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > >diff --git a/source4/dns_server/dns_query.c b/source4/dns_server/dns_query.c >index 0c26f9f8fb5..19c4dc32faa 100644 >--- a/source4/dns_server/dns_query.c >+++ b/source4/dns_server/dns_query.c >@@ -439,7 +439,8 @@ static struct tevent_req *handle_authoritative_send( > TALLOC_CTX *mem_ctx, struct tevent_context *ev, > struct dns_server *dns, const char *forwarder, > struct dns_name_question *question, >- struct dns_res_rec **answers, struct dns_res_rec **nsrecs); >+ struct dns_res_rec **answers, struct dns_res_rec **nsrecs, >+ size_t cname_depth); > static WERROR handle_authoritative_recv(struct tevent_req *req); > > struct handle_dnsrpcrec_state { >@@ -455,7 +456,8 @@ static struct tevent_req *handle_dnsrpcrec_send( > struct dns_server *dns, const char *forwarder, > const struct dns_name_question *question, > struct dnsp_DnssrvRpcRecord *rec, >- struct dns_res_rec **answers, struct dns_res_rec **nsrecs) >+ struct dns_res_rec **answers, struct dns_res_rec **nsrecs, >+ size_t cname_depth) > { > struct tevent_req *req, *subreq; > struct handle_dnsrpcrec_state *state; >@@ -471,7 +473,7 @@ static struct tevent_req *handle_dnsrpcrec_send( > state->answers = answers; > state->nsrecs = nsrecs; > >- if (talloc_array_length(*answers) >= MAX_Q_RECURSION_DEPTH) { >+ if (cname_depth >= MAX_Q_RECURSION_DEPTH) { > tevent_req_done(req); > return tevent_req_post(req, ev); > } >@@ -516,7 +518,8 @@ static struct tevent_req *handle_dnsrpcrec_send( > if (dns_authoritative_for_zone(dns, new_q->name)) { > subreq = handle_authoritative_send( > state, ev, dns, forwarder, new_q, >- state->answers, state->nsrecs); >+ state->answers, state->nsrecs, >+ cname_depth + 1); > if (tevent_req_nomem(subreq, req)) { > return tevent_req_post(req, ev); > } >@@ -600,6 +603,8 @@ struct handle_authoritative_state { > > struct dns_res_rec **answers; > struct dns_res_rec **nsrecs; >+ >+ size_t cname_depth; > }; > > static void handle_authoritative_done(struct tevent_req *subreq); >@@ -608,7 +613,8 @@ static struct tevent_req *handle_authoritative_send( > TALLOC_CTX *mem_ctx, struct tevent_context *ev, > struct dns_server *dns, const char *forwarder, > struct dns_name_question *question, >- struct dns_res_rec **answers, struct dns_res_rec **nsrecs) >+ struct dns_res_rec **answers, struct dns_res_rec **nsrecs, >+ size_t cname_depth) > { > struct tevent_req *req, *subreq; > struct handle_authoritative_state *state; >@@ -626,6 +632,7 @@ static struct tevent_req *handle_authoritative_send( > state->forwarder = forwarder; > state->answers = answers; > state->nsrecs = nsrecs; >+ state->cname_depth = cname_depth; > > werr = dns_name2dn(dns, state, question->name, &dn); > if (tevent_req_werror(req, werr)) { >@@ -647,7 +654,8 @@ static struct tevent_req *handle_authoritative_send( > subreq = handle_dnsrpcrec_send( > state, state->ev, state->dns, state->forwarder, > state->question, &state->recs[state->recs_done], >- state->answers, state->nsrecs); >+ state->answers, state->nsrecs, >+ state->cname_depth); > if (tevent_req_nomem(subreq, req)) { > return tevent_req_post(req, ev); > } >@@ -679,7 +687,8 @@ static void handle_authoritative_done(struct tevent_req *subreq) > subreq = handle_dnsrpcrec_send( > state, state->ev, state->dns, state->forwarder, > state->question, &state->recs[state->recs_done], >- state->answers, state->nsrecs); >+ state->answers, state->nsrecs, >+ state->cname_depth); > if (tevent_req_nomem(subreq, req)) { > return; > } >@@ -1010,7 +1019,8 @@ struct tevent_req *dns_server_process_query_send( > > subreq = handle_authoritative_send( > state, ev, dns, (forwarders == NULL ? NULL : forwarders[0]), >- &in->questions[0], &state->answers, &state->nsrecs); >+ &in->questions[0], &state->answers, &state->nsrecs, >+ 0); /* cname_depth */ > if (tevent_req_nomem(subreq, req)) { > return tevent_req_post(req, ev); > } >@@ -1112,7 +1122,8 @@ static void dns_server_process_query_got_auth(struct tevent_req *subreq) > subreq = handle_authoritative_send(state, state->ev, state->dns, > state->forwarders->forwarder, > state->question, &state->answers, >- &state->nsrecs); >+ &state->nsrecs, >+ 0); /* cname_depth */ > > if (tevent_req_nomem(subreq, req)) { > return; >-- >2.11.0 > > >From 37a9eb774c88d089d3c2be77824e650be9f2245e Mon Sep 17 00:00:00 2001 >From: Aaron Haslett <aaronhaslett@catalyst.net.nz> >Date: Fri, 30 Nov 2018 18:37:27 +1300 >Subject: [PATCH 6/6] CVE-2018-14629: Tests to expose regression from dns cname > loop fix > >These tests expose the regression described by Stefan Metzmacher in >discussion on the bugzilla paged linked below. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13600 >Signed-off-by: Aaron Haslett <aaronhaslett@catalyst.net.nz> >--- > python/samba/tests/dns.py | 97 +++++++++++++++++++++++++++++++++++++++++++++++ > selftest/knownfail.d/dns | 14 ++++++- > 2 files changed, 109 insertions(+), 2 deletions(-) > >diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py >index 102269c7156..65e4a3b0b3f 100644 >--- a/python/samba/tests/dns.py >+++ b/python/samba/tests/dns.py >@@ -918,6 +918,103 @@ class TestComplexQueries(DNSTest): > max_recursion_depth = 20 > self.assertEquals(len(response.answers), max_recursion_depth) > >+ # Make sure cname limit doesn't count other records. This is a generic >+ # test called in tests below >+ def max_rec_test(self, rtype, rec_gen): >+ name = "limittestrec{0}.{1}".format(rtype, self.get_dns_domain()) >+ limit = 20 >+ num_recs_to_enter = limit + 5 >+ >+ for i in range(1, num_recs_to_enter+1): >+ ip = rec_gen(i) >+ self.make_dns_update(name, ip, rtype) >+ >+ p = self.make_name_packet(dns.DNS_OPCODE_QUERY) >+ questions = [] >+ >+ q = self.make_name_question(name, >+ rtype, >+ dns.DNS_QCLASS_IN) >+ questions.append(q) >+ self.finish_name_packet(p, questions) >+ >+ response = self.dns_transaction_udp(p, host=self.server_ip) >+ >+ self.assertEqual(len(response.answers), num_recs_to_enter) >+ >+ def test_record_limit_A(self): >+ def ip4_gen(i): >+ return "127.0.0." + str(i) >+ self.max_rec_test(rtype=dns.DNS_QTYPE_A, rec_gen=ip4_gen) >+ >+ def test_record_limit_AAAA(self): >+ def ip6_gen(i): >+ return "AAAA:0:0:0:0:0:0:" + str(i) >+ self.max_rec_test(rtype=dns.DNS_QTYPE_AAAA, rec_gen=ip6_gen) >+ >+ def test_record_limit_SRV(self): >+ def srv_gen(i): >+ rec = dns.srv_record() >+ rec.priority = 1 >+ rec.weight = 1 >+ rec.port = 92 >+ rec.target = "srvtestrec" + str(i) >+ return rec >+ self.max_rec_test(rtype=dns.DNS_QTYPE_SRV, rec_gen=srv_gen) >+ >+ # Same as test_record_limit_A but with a preceding CNAME follow >+ def test_cname_limit(self): >+ cname1 = "cnamelimittestrec." + self.get_dns_domain() >+ cname2 = "cnamelimittestrec2." + self.get_dns_domain() >+ cname3 = "cnamelimittestrec3." + self.get_dns_domain() >+ ip_prefix = '127.0.0.' >+ limit = 20 >+ num_recs_to_enter = limit + 5 >+ >+ self.make_dns_update(cname1, cname2, dnsp.DNS_TYPE_CNAME) >+ self.make_dns_update(cname2, cname3, dnsp.DNS_TYPE_CNAME) >+ num_arecs_to_enter = num_recs_to_enter - 2 >+ for i in range(1, num_arecs_to_enter+1): >+ ip = ip_prefix + str(i) >+ self.make_dns_update(cname3, ip, dns.DNS_QTYPE_A) >+ >+ p = self.make_name_packet(dns.DNS_OPCODE_QUERY) >+ questions = [] >+ >+ q = self.make_name_question(cname1, >+ dns.DNS_QTYPE_A, >+ dns.DNS_QCLASS_IN) >+ questions.append(q) >+ self.finish_name_packet(p, questions) >+ >+ response = self.dns_transaction_udp(p, host=self.server_ip) >+ >+ self.assertEqual(len(response.answers), num_recs_to_enter) >+ >+ # ANY query on cname record shouldn't follow the link >+ def test_cname_any_query(self): >+ cname1 = "cnameanytestrec." + self.get_dns_domain() >+ cname2 = "cnameanytestrec2." + self.get_dns_domain() >+ cname3 = "cnameanytestrec3." + self.get_dns_domain() >+ >+ self.make_dns_update(cname1, cname2, dnsp.DNS_TYPE_CNAME) >+ self.make_dns_update(cname2, cname3, dnsp.DNS_TYPE_CNAME) >+ >+ p = self.make_name_packet(dns.DNS_OPCODE_QUERY) >+ questions = [] >+ >+ q = self.make_name_question(cname1, >+ dns.DNS_QTYPE_ALL, >+ dns.DNS_QCLASS_IN) >+ questions.append(q) >+ self.finish_name_packet(p, questions) >+ >+ response = self.dns_transaction_udp(p, host=self.server_ip) >+ >+ self.assertEqual(len(response.answers), 1) >+ self.assertEqual(response.answers[0].name, cname1) >+ self.assertEqual(response.answers[0].rdata, cname2) >+ > > class TestInvalidQueries(DNSTest): > >diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns >index 916afc1af85..a9b16eaac2a 100644 >--- a/selftest/knownfail.d/dns >+++ b/selftest/knownfail.d/dns >@@ -1,5 +1,15 @@ > # >-# rodc and vampire_dc require signed dns updates, so the test setup >-# fails, but the test does run on fl2003dc >+# rodc and vampire_dc require signed dns updates, so these tests' setups >+# fail, but they pass on fl2003dc > ^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(rodc:local\) > ^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(vampire_dc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_A\(rodc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_A\(vampire_dc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_AAAA\(rodc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_AAAA\(vampire_dc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_SRV\(rodc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_SRV\(vampire_dc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_cname_limit\(vampire_dc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_cname_limit\(rodc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_cname_any_query\(vampire_dc:local\) >+^samba.tests.dns.__main__.TestComplexQueries.test_cname_any_query\(rodc:local\) >-- >2.11.0 >
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
Actions:
View
Attachments on
bug 13600
:
14460
|
14537
|
14572
|
14573
|
14574
|
14575
|
14594
|
14631
|
14632
|
14635
|
14636
|
14649
|
14667
|
14668
|
14670
|
14685
|
14691
|
14692
|
14693
|
14694
|
14696
|
14697
|
14704
|
14711
|
14712
| 14717 |
14719
|
14720
|
14724
|
17140