From 6d3da1eb00a31e7b0a30415d81c1ac4508da2aad Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 17:25:51 +1300 Subject: [PATCH 1/4] 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 Reviewed-by: Andrew Bartlett Reviewed-by: Garming Sam --- 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 Date: Tue, 6 Sep 2016 10:48:57 +1200 Subject: [PATCH 2/4] tests/dns_forwarder: Wait for port for 15 seconds Signed-off-by: Garming Sam Reviewed-by: Andrew Bartlett (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 Date: Thu, 7 Jul 2016 16:58:27 +1200 Subject: [PATCH 3/4] tests/dns_forwarder: Check that the subprocess is still living Signed-off-by: Garming Sam Reviewed-by: Andrew Bartlett (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 Date: Thu, 9 Jun 2016 03:52:38 +0200 Subject: [PATCH 4/4] tests/dns_forwarder: Fail out with an assertion instead OOB error Signed-off-by: Garming Sam Reviewed-by: Andrew Bartlett Autobuild-User(master): Garming Sam 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 6c73a2b3d77115d69f99baa2452d6539c697fc3b Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 28 Nov 2018 15:21:56 +0100 Subject: [PATCH 1/2] 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 --- 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.19.2 From 77fac10d0171b731bce38e5596928b6d618ed4d8 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Fri, 30 Nov 2018 18:37:27 +1300 Subject: [PATCH 2/2] 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 --- 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.19.2