From 62b9b3106519ad459d59627323edb83324019d94 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 11:52:07 +1300 Subject: [PATCH 1/2] dns: prevent self-referencing CNAME Stops the user from adding a self-referencing CNAME over RPC, which is an easy mistake to make with samba-tool. 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 | 44 +++++++++++++++++++++++++ selftest/knownfail.d/dns | 1 + source4/rpc_server/dnsserver/dcerpc_dnsserver.c | 39 ++++++++++++++++++++++ 3 files changed, 84 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 12cfb86c254..56609769a4f 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -1481,6 +1481,50 @@ class TestRPCRoundtrip(DNSTest): def tearDown(self): super(TestRPCRoundtrip, self).tearDown() + def rpc_update(self, fqn=None, data=None, wType=None, delete=False): + fqn = fqn or ("rpctestrec." + self.get_dns_domain()) + + rec = data_to_dns_record(wType, data) + add_rec_buf = dnsserver.DNS_RPC_RECORD_BUF() + add_rec_buf.rec = rec + + add_arg = add_rec_buf + del_arg = None + if delete: + add_arg = None + del_arg = add_rec_buf + + self.rpc_conn.DnssrvUpdateRecord2( + dnsserver.DNS_CLIENT_VERSION_LONGHORN, + 0, + self.server_ip, + self.get_dns_domain(), + fqn, + add_arg, + del_arg) + + def test_rpc_self_referencing_cname(self): + cname = "cnametest2_unqual_rec_loop" + cname_fqn = "%s.%s" % (cname, self.get_dns_domain()) + + try: + self.rpc_update(fqn=cname, data=cname_fqn, + wType=dnsp.DNS_TYPE_CNAME, delete=True) + except WERRORError as e: + if e.args[0] != werror.WERR_DNS_ERROR_RECORD_DOES_NOT_EXIST: + self.fail("RPC DNS gaven wrong error on pre-test cleanup " + "for self referencing CNAME: %s" % e.args[0]) + + try: + self.rpc_update(fqn=cname, wType=dnsp.DNS_TYPE_CNAME, data=cname_fqn) + except WERRORError as e: + if e.args[0] != werror.WERR_DNS_ERROR_CNAME_LOOP: + self.fail("RPC DNS gaven wrong error on insertion of " + "self referencing CNAME: %s" % e.args[0]) + return + + self.fail("RPC DNS allowed insertion of self referencing CNAME") + def test_update_add_txt_rpc_to_dns(self): prefix, txt = 'rpctextrec', ['"This is a test"'] diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index ca18b4334c1..0f6caba8916 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -13,6 +13,7 @@ samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_null_char_txt_record\( samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_null_padded_txt_record\(rodc:local\) samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_slash_txt_record\(rodc:local\) samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_two_txt_records\(rodc:local\) +samba.tests.dns.__main__.TestRPCRoundtrip.test_rpc_self_referencing_cname\(rodc:local\) samba.tests.dns.__main__.TestDNSUpdates.test_delete_record\(vampire_dc:local\) samba.tests.dns.__main__.TestDNSUpdates.test_readd_record\(vampire_dc:local\) samba.tests.dns.__main__.TestDNSUpdates.test_update_add_mx_record\(vampire_dc:local\) diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c index b42d7c549d1..edea2b33f2d 100644 --- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c +++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c @@ -25,6 +25,7 @@ #include "dsdb/samdb/samdb.h" #include "lib/util/dlinklist.h" #include "librpc/gen_ndr/ndr_dnsserver.h" +#include "dns_server/dnsserver_common.h" #include "dnsserver.h" #define DCESRV_INTERFACE_DNSSERVER_BIND(call, iface) \ @@ -1868,6 +1869,37 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate, return WERR_OK; } +/* + * Check str1 + '.' + str2 = name, for example: + * ("dc0", "example.com", "dc0.example.com") = true + */ +static bool cname_self_reference(const char* node_name, + const char* zone_name, + struct DNS_RPC_NAME name) { + size_t node_len, zone_len; + + if (node_name == NULL || zone_name == NULL) { + return false; + } + + node_len = strlen(node_name); + zone_len = strlen(zone_name); + + if (node_len == 0 || + zone_len == 0 || + (name.len != node_len + zone_len + 1)) { + return false; + } + + if (strncmp(node_name, name.str, node_len) == 0 && + name.str[node_len] == '.' && + strncmp(zone_name, name.str + node_len + 1, zone_len) == 0) { + return true; + } + + return false; +} + /* dnsserver update function */ static WERROR dnsserver_update_record(struct dnsserver_state *dsstate, @@ -1895,6 +1927,13 @@ static WERROR dnsserver_update_record(struct dnsserver_state *dsstate, } W_ERROR_HAVE_NO_MEMORY_AND_FREE(name, tmp_ctx); + /* CNAMEs can't point to themselves */ + if (add_buf != NULL && add_buf->rec.wType == DNS_TYPE_CNAME) { + if (cname_self_reference(node_name, z->name, add_buf->rec.data.name)) { + return WERR_DNS_ERROR_CNAME_LOOP; + } + } + if (add_buf != NULL) { if (del_buf == NULL) { /* Add record */ -- 2.11.0 From 085fc7e01edc27ccfd623e03cf0bab0fc1a68c2a Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 17:25:51 +1300 Subject: [PATCH 2/2] 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. 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 | 22 ++++++++++++++++++++++ selftest/knownfail.d/dns | 6 ++++++ source4/dns_server/dns_query.c | 6 ++++++ 3 files changed, 34 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 56609769a4f..e35cded7b8c 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -846,6 +846,28 @@ class TestComplexQueries(DNSTest): self.assertEquals(response.answers[1].name, name2) self.assertEquals(response.answers[1].rdata, name0) + 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, response_packet) =\ + 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 setUp(self): diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index 0f6caba8916..39b337ed6bb 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -71,3 +71,9 @@ samba.tests.dns.__main__.TestSimpleQueries.test_qtype_all_query\(rodc:local\) # The SOA override should not pass against the RODC, it must not overstamp 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 +^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 923f7233eb9..65faeac3b6a 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; @@ -419,6 +420,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