Attachment 14724 Details for Bug 13600 – combined fix, regression fix and test patch for 4.8

[patch] combined fix, regression fix and test patch for 4.8

135.patch (text/plain), 12.62 KB, created by Andrew Bartlett on 2018-12-07 08:32:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2018-12-07 08:32:07 UTC

Size: 12.62 KB

patch

obsolete

>From b92430867c55273aa5940fc8a9ba4c9b9f02e7a3 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 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 <aaronhaslett@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(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 853bc12c79ebb8111432000cb9e129f6e6aeebbb Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>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 <metze@samba.org>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>
>Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>
>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 07cde80a258..ef3ece248fe 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;
> 	}
>@@ -999,7 +1008,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);
> 		}
>@@ -1101,7 +1111,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
>

Flags:

metze: review+

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