From 3e74c7e5ffc455051549b1199328c76ddc5c72cc Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 25 Feb 2019 12:55:27 -0800 Subject: [PATCH 1/3] fix crash in query_respond_any() from all records being hidden in query_respond_any(), the assumption had previously been made that it was impossible to get past iterating the node with a return value of ISC_R_NOMORE but not have found any records, unless we were searching for RRSIG or SIG. however, it is possible for other types to exist but be hidden, such as when the zone is transitioning from insecure to secure and DNSSEC types are encountered, and this situation could trigger an assertion. removed the assertion and reorganized the code. --- lib/ns/query.c | 91 ++++++++++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index f1c640b43f..0efcafd578 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6727,7 +6727,7 @@ query_addnoqnameproof(query_ctx_t *qctx) { */ static isc_result_t query_respond_any(query_ctx_t *qctx) { - bool found = false; + bool found = false, hidden = false; dns_rdatasetiter_t *rdsiter = NULL; isc_result_t result; dns_rdatatype_t onetype = 0; /* type to use for minimal-any */ @@ -6782,11 +6782,11 @@ query_respond_any(query_ctx_t *qctx) { dns_rdatatype_isdnssec(qctx->rdataset->type)) { /* - * The zone is transitioning from insecure - * to secure. Hide the dnssec records from - * ANY queries. + * The zone may be transitioning from insecure + * to secure. Hide DNSSEC records from ANY queries. */ dns_rdataset_disassociate(qctx->rdataset); + hidden = true; } else if (qctx->view->minimal_any && !TCP(qctx->client) && !WANTDNSSEC(qctx->client) && qctx->qtype == dns_rdatatype_any && @@ -6808,7 +6808,8 @@ query_respond_any(query_ctx_t *qctx) { qctx->rdataset->type == qctx->qtype) && qctx->rdataset->type != 0) { - if (NOQNAME(qctx->rdataset) && WANTDNSSEC(qctx->client)) + if (NOQNAME(qctx->rdataset) && + WANTDNSSEC(qctx->client)) { qctx->noqname = qctx->rdataset; } else { @@ -6864,8 +6865,9 @@ query_respond_any(query_ctx_t *qctx) { } qctx->rdataset = ns_client_newrdataset(qctx->client); - if (qctx->rdataset == NULL) + if (qctx->rdataset == NULL) { break; + } } else { /* * We're not interested in this rdataset. @@ -6886,48 +6888,59 @@ query_respond_any(query_ctx_t *qctx) { } if (found) { + /* + * Call hook if any answers were found. + * Do this before releasing qctx->fname, in case + * the hook function needs it. + */ CALL_HOOK(NS_QUERY_RESPOND_ANY_FOUND, qctx); - - if (qctx->fname != NULL) { - dns_message_puttempname(qctx->client->message, - &qctx->fname); - } - - query_addauth(qctx); - return (ns_query_done(qctx)); } - /* - * If we're here, no matching rdatasets were found. If we were - * searching for RRSIG/SIG, that may be okay, but otherwise - * something's gone wrong. - */ - INSIST(qctx->qtype == dns_rdatatype_rrsig || - qctx->qtype == dns_rdatatype_sig); - if (qctx->fname != NULL) { - dns_message_puttempname(qctx->client->message, - &qctx->fname); + dns_message_puttempname(qctx->client->message, &qctx->fname); } - if (!qctx->is_zone) { - qctx->authoritative = false; - qctx->client->attributes &= ~NS_CLIENTATTR_RA; + if (found) { + /* + * At least one matching rdataset was found + */ query_addauth(qctx); - return (ns_query_done(qctx)); + } else if (qctx->qtype == dns_rdatatype_rrsig || + qctx->qtype == dns_rdatatype_sig) + { + /* + * No matching rdatasets were found, but we got + * here on a search for RRSIG/SIG, so that's okay. + */ + if (!qctx->is_zone) { + qctx->authoritative = false; + qctx->client->attributes &= ~NS_CLIENTATTR_RA; + query_addauth(qctx); + return (ns_query_done(qctx)); + } + + if (qctx->qtype == dns_rdatatype_rrsig && + dns_db_issecure(qctx->db)) + { + char namebuf[DNS_NAME_FORMATSIZE]; + dns_name_format(qctx->client->query.qname, + namebuf, sizeof(namebuf)); + ns_client_log(qctx->client, DNS_LOGCATEGORY_DNSSEC, + NS_LOGMODULE_QUERY, ISC_LOG_WARNING, + "missing signature for %s", namebuf); + } + + qctx->fname = ns_client_newname(qctx->client, qctx->dbuf, &b); + return (query_sign_nodata(qctx)); + } else if (!hidden) { + /* + * No matching rdatasets were found and nothing was + * deliberately hidden: something must have gone wrong. + */ + QUERY_ERROR(qctx, DNS_R_SERVFAIL); } - if (qctx->qtype == dns_rdatatype_rrsig && dns_db_issecure(qctx->db)) { - char namebuf[DNS_NAME_FORMATSIZE]; - dns_name_format(qctx->client->query.qname, - namebuf, sizeof(namebuf)); - ns_client_log(qctx->client, DNS_LOGCATEGORY_DNSSEC, - NS_LOGMODULE_QUERY, ISC_LOG_WARNING, - "missing signature for %s", namebuf); - } - - qctx->fname = ns_client_newname(qctx->client, qctx->dbuf, &b); - return (query_sign_nodata(qctx)); + return (ns_query_done(qctx)); cleanup: return (result); From c6939f0bd469120de53d4a7d8b6ba1cfd960a86b Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 28 Feb 2019 14:28:26 -0800 Subject: [PATCH 2/3] test correct occlusion of DNSSEC records --- bin/tests/system/dnssec/ns3/insecure.example.db | 1 + bin/tests/system/dnssec/ns3/secure.example.db.in | 1 + bin/tests/system/dnssec/tests.sh | 13 +++++++++++++ 3 files changed, 15 insertions(+) diff --git a/bin/tests/system/dnssec/ns3/insecure.example.db b/bin/tests/system/dnssec/ns3/insecure.example.db index 86552149e1..98777d674f 100644 --- a/bin/tests/system/dnssec/ns3/insecure.example.db +++ b/bin/tests/system/dnssec/ns3/insecure.example.db @@ -21,4 +21,5 @@ ns A 10.53.0.3 a A 10.0.0.1 b A 10.0.0.2 d A 10.0.0.4 +x DNSKEY 258 3 5 Cg== z A 10.0.0.26 diff --git a/bin/tests/system/dnssec/ns3/secure.example.db.in b/bin/tests/system/dnssec/ns3/secure.example.db.in index 9d310d8cb2..27f2b2401c 100644 --- a/bin/tests/system/dnssec/ns3/secure.example.db.in +++ b/bin/tests/system/dnssec/ns3/secure.example.db.in @@ -28,6 +28,7 @@ g A 10.0.0.7 z A 10.0.0.26 a.a.a.a.a.a.a.a.a.a.e A 10.0.0.27 x CNAME a +zz DNSKEY 258 3 5 Cg== private NS ns.private ns.private A 10.53.0.2 diff --git a/bin/tests/system/dnssec/tests.sh b/bin/tests/system/dnssec/tests.sh index 1f39bd535b..81865b626e 100644 --- a/bin/tests/system/dnssec/tests.sh +++ b/bin/tests/system/dnssec/tests.sh @@ -3635,6 +3635,19 @@ n=$((n+1)) test "$ret" -eq 0 || echo_i "failed" status=$((status+ret)) +echo_i "checking DNSSEC records are occluded from ANY in an insecure zone ($n)" +ret=0 +dig_with_opts any x.insecure.example. @10.53.0.3 > dig.out.ns3.1.test$n || ret=1 +grep "status: NOERROR" dig.out.ns3.1.test$n > /dev/null || ret=1 +grep "ANSWER: 0," dig.out.ns3.1.test$n > /dev/null || ret=1 +dig_with_opts any zz.secure.example. @10.53.0.3 > dig.out.ns3.2.test$n || ret=1 +grep "status: NOERROR" dig.out.ns3.2.test$n > /dev/null || ret=1 +# DNSKEY+RRSIG, NSEC+RRSIG +grep "ANSWER: 4," dig.out.ns3.2.test$n > /dev/null || ret=1 +n=$((n+1)) +test "$ret" -eq 0 || echo_i "failed" +status=$((status+ret)) + # Note: after this check, ns4 will not be validating any more; do not add any # further validation tests employing ns4 below this check. echo_i "check that validation defaults to off when dnssec-enable is off ($n)" From 4ad0bc38e973a3be97adeb1a06f3833af394297a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 28 Feb 2019 14:06:23 -0800 Subject: [PATCH 3/3] CHANGES, release notes --- CHANGES | 4 ++++ doc/arm/notes.xml | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 3ec56b9c9e..339a5046f1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5169. [bug] The presence of certain types in an otherwise + empty node could cause a crash while processing a + type ANY query. [GL #901] + 5168. [bug] Do not crash on shutdown when RPZ fails to load. Also, keep previous version of the database if RPZ fails to load. [GL #813] diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml index 8ae3174604..3dc8716a07 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -127,7 +127,9 @@ - None. + The presence of certain record types in an otherwise empty + node of a zone could trigger an assertion failure while processing + a query of type ANY. [GL #901]