From 01115b5414e1629c3b5788954de6a44e63d5af66 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Fri, 30 Nov 2018 18:37:27 +1300 Subject: [PATCH 3/4] 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 | 100 ++++++++++++++++++++++++++++++++++++++ selftest/knownfail.d/dns | 20 +++++++- 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 3390a3990c9..eea3f720a1a 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -821,6 +821,106 @@ 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): diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index 8c79b3abe00..bd12d439c11 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -47,7 +47,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 d63701b0ac2787e7e698fcf17743c52c07e1e32f Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 28 Nov 2018 15:21:56 +0100 Subject: [PATCH 4/4] 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 bd12d439c11..c26a31aeb4e 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -61,9 +61,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 fafadb6ac6f..cb9afd099c2 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)) { @@ -646,7 +653,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); } @@ -678,7 +686,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; } @@ -1050,7 +1059,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); } @@ -1152,7 +1162,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