From be86f4bb790d5912b83f943be0839f3701e411d9 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Fri, 30 Nov 2018 18:37:27 +1300 Subject: [PATCH 1/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 Reviewed-by: Douglas Bagnall Reviewed-by: Stefan Metzmacher (cherry picked from commit 14399fd818b130a6347eec860460929c292d5996) --- python/samba/tests/dns.py | 101 ++++++++++++++++++++++++++++++++++++++ selftest/knownfail.d/dns | 20 +++++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 3e6306e2be8..515fd92a55e 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -867,6 +867,107 @@ 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, response_packet) =\ + 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, response_packet) =\ + 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, response_packet) =\ + 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): def setUp(self): super(TestInvalidQueries, self).setUp() diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index a248432aafa..e27b93aad86 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -71,7 +71,23 @@ samba.tests.dns.__main__.TestSimpleQueries.test_qtype_all_query\(rodc:local\) samba.tests.dns.__main__.TestSimpleQueries.test_one_SOA_query\(rodc:local\) # -# 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\) + +# These all fail until the next patch +^samba.tests.dns.__main__.TestComplexQueries.test_cname_limit +^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_SRV +^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_AAAA +^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_A -- 2.18.1 From 407e55aa12b3c6dc84aa8242765289996b12bf02 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 28 Nov 2018 15:21:56 +0100 Subject: [PATCH 2/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 Reviewed-by: Douglas Bagnall Autobuild-User(master): Andrew Bartlett Autobuild-Date(master): Tue Dec 4 08:52:29 CET 2018 on sn-devel-144 (cherry picked from commit 34f4491d79b47b2fe2457b8882f11644cf773bc4) --- selftest/knownfail.d/dns | 6 ------ source4/dns_server/dns_query.c | 29 ++++++++++++++++++++--------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index e27b93aad86..8f1b0b1341b 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -85,9 +85,3 @@ samba.tests.dns.__main__.TestSimpleQueries.test_one_SOA_query\(rodc: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\) - -# These all fail until the next patch -^samba.tests.dns.__main__.TestComplexQueries.test_cname_limit -^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_SRV -^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_AAAA -^samba.tests.dns.__main__.TestComplexQueries.test_record_limit_A diff --git a/source4/dns_server/dns_query.c b/source4/dns_server/dns_query.c index 65faeac3b6a..b75fabe7e82 100644 --- a/source4/dns_server/dns_query.c +++ b/source4/dns_server/dns_query.c @@ -388,7 +388,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 { @@ -404,7 +405,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; @@ -420,7 +422,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); } @@ -465,7 +467,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); } @@ -549,6 +552,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); @@ -557,7 +562,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; @@ -575,6 +581,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)) { @@ -595,7 +602,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); } @@ -627,7 +635,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; } @@ -1000,7 +1009,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); } @@ -1102,7 +1112,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.18.1