From c6bf43a8219f5fea873d48454fd97bcc68f12e6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 24 Apr 2019 11:17:15 +0200 Subject: [PATCH 1/2] Make NTAs work with validating forwarders If named is configured to perform DNSSEC validation and also forwards all queries ("forward only;") to validating resolvers, negative trust anchors do not work properly because the CD bit is not set in queries sent to the forwarders. As a result, instead of retrieving bogus DNSSEC material and making validation decisions based on its configuration, named is only receiving SERVFAIL responses to queries for bogus data. Fix by ensuring the CD bit is always set in queries sent to forwarders if the query name is covered by an NTA. (cherry picked from commit 5e8048827015f4a04e61ae5f3c92758755fee6c3) --- bin/tests/system/dnssec/ns1/sign.sh | 1 + bin/tests/system/dnssec/ns9/named.conf.in | 38 +++++++++++++++++++++++ bin/tests/system/dnssec/setup.sh | 2 ++ bin/tests/system/dnssec/tests.sh | 22 +++++++++++++ lib/dns/include/dns/view.h | 6 ++-- lib/dns/resolver.c | 34 +++++++++++--------- lib/dns/tests/keytable_test.c | 18 +++++++---- lib/dns/view.c | 16 ++++++++-- 8 files changed, 112 insertions(+), 25 deletions(-) create mode 100644 bin/tests/system/dnssec/ns9/named.conf.in diff --git a/bin/tests/system/dnssec/ns1/sign.sh b/bin/tests/system/dnssec/ns1/sign.sh index 4df96bf1e0..effebd6f55 100644 --- a/bin/tests/system/dnssec/ns1/sign.sh +++ b/bin/tests/system/dnssec/ns1/sign.sh @@ -44,6 +44,7 @@ cp trusted.conf ../ns3/trusted.conf cp trusted.conf ../ns4/trusted.conf cp trusted.conf ../ns6/trusted.conf cp trusted.conf ../ns7/trusted.conf +cp trusted.conf ../ns9/trusted.conf # ...or with a managed key. keyfile_to_managed_keys "$keyname" > managed.conf diff --git a/bin/tests/system/dnssec/ns9/named.conf.in b/bin/tests/system/dnssec/ns9/named.conf.in new file mode 100644 index 0000000000..d655112a70 --- /dev/null +++ b/bin/tests/system/dnssec/ns9/named.conf.in @@ -0,0 +1,38 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +// NS9 + +options { + query-source address 10.53.0.9; + notify-source 10.53.0.9; + transfer-source 10.53.0.9; + port @PORT@; + pid-file "named.pid"; + listen-on { 10.53.0.9; }; + listen-on-v6 { none; }; + recursion yes; + dnssec-enable yes; + dnssec-validation yes; + forward only; + forwarders { 10.53.0.4; }; +}; + +key rndc_key { + secret "1234abcd8765"; + algorithm hmac-sha256; +}; + +controls { + inet 10.53.0.9 port @CONTROLPORT@ allow { any; } keys { rndc_key; }; +}; + +include "trusted.conf"; diff --git a/bin/tests/system/dnssec/setup.sh b/bin/tests/system/dnssec/setup.sh index 2238153204..9798e34be7 100644 --- a/bin/tests/system/dnssec/setup.sh +++ b/bin/tests/system/dnssec/setup.sh @@ -27,6 +27,8 @@ copy_setports ns6/named.conf.in ns6/named.conf copy_setports ns7/named.conf.in ns7/named.conf copy_setports ns8/named.conf.in ns8/named.conf +copy_setports ns9/named.conf.in ns9/named.conf + ( cd ns1 $SHELL sign.sh diff --git a/bin/tests/system/dnssec/tests.sh b/bin/tests/system/dnssec/tests.sh index e764449c1d..4ff5d6eae0 100644 --- a/bin/tests/system/dnssec/tests.sh +++ b/bin/tests/system/dnssec/tests.sh @@ -2300,9 +2300,31 @@ fi # cleanup rndccmd 10.53.0.4 nta -remove secure.example > rndc.out.ns4.test$n.3 2>/dev/null +n=$((n+1)) if [ "$ret" -ne 0 ]; then echo_i "failed - NTA lifetime clamping failed"; fi status=$((status+ret)) + +echo_i "checking that NTAs work with 'forward only;' to a validating resolver ($n)" ret=0 +# Sanity check behavior without an NTA in place. +dig_with_opts @10.53.0.9 badds.example. SOA > dig.out.ns9.test$n.1 || ret=1 +grep "SERVFAIL" dig.out.ns9.test$n.1 > /dev/null || ret=1 +grep "ANSWER: 0" dig.out.ns9.test$n.1 > /dev/null || ret=1 +grep "flags:[^;]* ad[ ;].*QUERY" dig.out.ns9.test$n.1 > /dev/null && ret=1 +# Add an NTA, expecting that to cause resolution to succeed. +rndccmd 10.53.0.9 nta badds.example > rndc.out.ns9.test$n.1 2>&1 || ret=1 +dig_with_opts @10.53.0.9 badds.example. SOA > dig.out.ns9.test$n.2 || ret=1 +grep "NOERROR" dig.out.ns9.test$n.2 > /dev/null || ret=1 +grep "ANSWER: 2" dig.out.ns9.test$n.2 > /dev/null || ret=1 +grep "flags:[^;]* ad[ ;].*QUERY" dig.out.ns9.test$n.2 > /dev/null && ret=1 +# Remove the NTA, expecting that to cause resolution to fail again. +rndccmd 10.53.0.9 nta -remove badds.example > rndc.out.ns9.test$n.2 2>&1 || ret=1 +dig_with_opts @10.53.0.9 badds.example. SOA > dig.out.ns9.test$n.3 || ret=1 +grep "SERVFAIL" dig.out.ns9.test$n.3 > /dev/null || ret=1 +grep "ANSWER: 0" dig.out.ns9.test$n.3 > /dev/null || ret=1 +grep "flags:[^;]* ad[ ;].*QUERY" dig.out.ns9.test$n.3 > /dev/null && ret=1 +if [ "$ret" -ne 0 ]; then echo_i "failed"; fi +status=$((status+ret)) echo_i "completed NTA tests" diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 43a3e9e016..8015b916be 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -1190,14 +1190,16 @@ dns_view_getsecroots(dns_view_t *view, dns_keytable_t **ktp); isc_result_t dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name, - isc_stdtime_t now, bool checknta, + isc_stdtime_t now, bool checknta, bool *ntap, bool *secure_domain); /*%< * Is 'name' at or beneath a trusted key, and not covered by a valid * negative trust anchor? Put answer in '*secure_domain'. * * If 'checknta' is false, ignore the NTA table in determining - * whether this is a secure domain. + * whether this is a secure domain. If 'checknta' is not false, and if + * 'ntap' is non-NULL, then '*ntap' will be updated with true if the + * name is covered by an NTA. * * Requires: * \li 'view' is valid. diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 99676882df..a8ed58b8ec 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -2328,8 +2328,7 @@ compute_cc(resquery_t *query, unsigned char *cookie, size_t len) { static isc_result_t issecuredomain(dns_view_t *view, const dns_name_t *name, dns_rdatatype_t type, - isc_stdtime_t now, bool checknta, - bool *issecure) + isc_stdtime_t now, bool checknta, bool *ntap, bool *issecure) { dns_name_t suffix; unsigned int labels; @@ -2347,7 +2346,8 @@ issecuredomain(dns_view_t *view, const dns_name_t *name, dns_rdatatype_t type, name = &suffix; } - return (dns_view_issecuredomain(view, name, now, checknta, issecure)); + return (dns_view_issecuredomain(view, name, now, checknta, + ntap, issecure)); } static isc_result_t @@ -2451,24 +2451,30 @@ resquery_send(resquery_t *query) { * question is under a secure entry point and this is a * recursive/forward query -- unless the client said not to. */ - if ((query->options & DNS_FETCHOPT_NOCDFLAG) != 0) + if ((query->options & DNS_FETCHOPT_NOCDFLAG) != 0) { /* Do nothing */ - ; - else if ((query->options & DNS_FETCHOPT_NOVALIDATE) != 0) + } else if ((query->options & DNS_FETCHOPT_NOVALIDATE) != 0) { fctx->qmessage->flags |= DNS_MESSAGEFLAG_CD; - else if (res->view->enablevalidation && - ((fctx->qmessage->flags & DNS_MESSAGEFLAG_RD) != 0)) + } else if (res->view->enablevalidation && + ((fctx->qmessage->flags & DNS_MESSAGEFLAG_RD) != 0)) { bool checknta = ((query->options & DNS_FETCHOPT_NONTA) == 0); + bool ntacovered = false; result = issecuredomain(res->view, &fctx->name, fctx->type, isc_time_seconds(&query->start), - checknta, &secure_domain); - if (result != ISC_R_SUCCESS) + checknta, &ntacovered, &secure_domain); + if (result != ISC_R_SUCCESS) { secure_domain = false; - if (res->view->dlv != NULL) + } + if (res->view->dlv != NULL) { secure_domain = true; - if (secure_domain) + } + + if (secure_domain || + (ISFORWARDER(query->addrinfo) && ntacovered)) + { fctx->qmessage->flags |= DNS_MESSAGEFLAG_CD; + } } /* @@ -5923,7 +5929,7 @@ cache_name(fetchctx_t *fctx, dns_name_t *name, dns_adbaddrinfo_t *addrinfo, if (res->view->enablevalidation) { result = issecuredomain(res->view, name, fctx->type, - now, checknta, &secure_domain); + now, checknta, NULL, &secure_domain); if (result != ISC_R_SUCCESS) { return (result); } @@ -6518,7 +6524,7 @@ ncache_message(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, if (fctx->res->view->enablevalidation) { result = issecuredomain(res->view, name, fctx->type, - now, checknta, &secure_domain); + now, checknta, NULL, &secure_domain); if (result != ISC_R_SUCCESS) return (result); diff --git a/lib/dns/tests/keytable_test.c b/lib/dns/tests/keytable_test.c index 1a6c7100f2..d2fdb7f0c1 100644 --- a/lib/dns/tests/keytable_test.c +++ b/lib/dns/tests/keytable_test.c @@ -676,15 +676,17 @@ nta_test(void **state) { /* Should be secure */ result = dns_view_issecuredomain(myview, str2name("test.secure.example"), - now, true, &issecure); + now, true, &covered, &issecure); assert_int_equal(result, ISC_R_SUCCESS); + assert_false(covered); assert_true(issecure); /* Should not be secure */ result = dns_view_issecuredomain(myview, str2name("test.insecure.example"), - now, true, &issecure); + now, true, &covered, &issecure); assert_int_equal(result, ISC_R_SUCCESS); + assert_true(covered); assert_false(issecure); /* NTA covered */ @@ -700,14 +702,16 @@ nta_test(void **state) { /* As of now + 2, the NTA should be clear */ result = dns_view_issecuredomain(myview, str2name("test.insecure.example"), - now + 2, true, &issecure); + now + 2, true, &covered, &issecure); assert_int_equal(result, ISC_R_SUCCESS); + assert_false(covered); assert_true(issecure); /* Now check deletion */ result = dns_view_issecuredomain(myview, str2name("test.new.example"), - now, true, &issecure); + now, true, &covered, &issecure); assert_int_equal(result, ISC_R_SUCCESS); + assert_false(covered); assert_true(issecure); result = dns_ntatable_add(ntatable, str2name("new.example"), @@ -715,16 +719,18 @@ nta_test(void **state) { assert_int_equal(result, ISC_R_SUCCESS); result = dns_view_issecuredomain(myview, str2name("test.new.example"), - now, true, &issecure); + now, true, &covered, &issecure); assert_int_equal(result, ISC_R_SUCCESS); + assert_true(covered); assert_false(issecure); result = dns_ntatable_delete(ntatable, str2name("new.example")); assert_int_equal(result, ISC_R_SUCCESS); result = dns_view_issecuredomain(myview, str2name("test.new.example"), - now, true, &issecure); + now, true, &covered, &issecure); assert_int_equal(result, ISC_R_SUCCESS); + assert_false(covered); assert_true(issecure); /* Clean up */ diff --git a/lib/dns/view.c b/lib/dns/view.c index 46edf41478..46f26e925f 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -1911,7 +1911,7 @@ dns_view_ntacovers(dns_view_t *view, isc_stdtime_t now, isc_result_t dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name, - isc_stdtime_t now, bool checknta, + isc_stdtime_t now, bool checknta, bool *ntap, bool *secure_domain) { isc_result_t result; @@ -1921,19 +1921,29 @@ dns_view_issecuredomain(dns_view_t *view, const dns_name_t *name, REQUIRE(DNS_VIEW_VALID(view)); - if (view->secroots_priv == NULL) + if (view->secroots_priv == NULL) { return (ISC_R_NOTFOUND); + } anchor = dns_fixedname_initname(&fn); result = dns_keytable_issecuredomain(view->secroots_priv, name, anchor, &secure); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { return (result); + } + if (ntap != NULL) { + *ntap = false; + } if (checknta && secure && view->ntatable_priv != NULL && dns_ntatable_covered(view->ntatable_priv, now, name, anchor)) + { + if (ntap != NULL) { + *ntap = true; + } secure = false; + } *secure_domain = secure; return (ISC_R_SUCCESS); From 9ca0c63f1f9fb5687581d15c5c09e0a8e71cbe92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 24 Apr 2019 11:17:15 +0200 Subject: [PATCH 2/2] Add CHANGES entry 5219. [bug] Negative trust anchors did not work with "forward only;" to validating resolvers. [GL #997] (cherry picked from commit 5be7c6f4b3d5967167d3c95e3b2bfe6e237814c9) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 1ee12a8191..cf2dfd3e34 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5232. [bug] Negative trust anchors did not work with "forward only;" + to validating resolvers. [GL #997] + 5231. [protocol] Add support for displaying CLIENT-TAG and SERVER-TAG. [GL #960]