From: Michał Kępień <michal@isc.org>
Date: Wed, 13 Jul 2022 08:31:16 +0000 (+0200)
Subject: Stop resolving invalid names in resume_dslookup()
X-Git-Tag: v9.19.4~35^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1a79aeab444d45236f71d6a21229fa121df188e3;p=thirdparty%2Fbind9.git

Stop resolving invalid names in resume_dslookup()

Commit 7b2ea97e46034ec3db4c950100708297798826af introduced a logic bug
in resume_dslookup(): that function now only conditionally checks
whether DS chasing can still make progress.  Specifically, that check is
only performed when the previous resume_dslookup() call invokes
dns_resolver_createfetch() with the 'nameservers' argument set to
something else than NULL, which may not always be the case.  Failing to
perform that check may trigger assertion failures as a result of
dns_resolver_createfetch() attempting to resolve an invalid name.

Example scenario that leads to such outcome:

 1. A validating resolver is configured to forward all queries to
    another resolver.  The latter returns broken DS responses that
    trigger DS chasing.

 2. rctx_chaseds() calls dns_resolver_createfetch() with the
    'nameservers' argument set to NULL.

 3. The fetch fails, so resume_dslookup() is called.  Due to
    fevent->result being set to e.g. DNS_R_SERVFAIL, the default branch
    is taken in the switch statement.

 4. Since 'nameservers' was set to NULL for the fetch which caused the
    resume_dslookup() callback to be invoked
    (fctx->nsfetch->private->nameservers), resume_dslookup() chops off
    one label off fctx->nsname and calls dns_resolver_createfetch()
    again, for a name containing one label less than before.

 5. Steps 3-4 are repeated (i.e. all attempts to find the name servers
    authoritative for the DS RRset being chased fail) until fctx->nsname
    becomes stripped down the the root name.

 6. Since resume_dslookup() does not check whether DS chasing can still
    make progress, it strips off a label off the root name and continues
    its attempts at finding the name servers authoritative for the DS
    RRset being chased, passing an invalid name to
    dns_resolver_createfetch().

Fix by ensuring resume_dslookup() always checks whether DS chasing can
still make progress when a name server fetch fails.  Update code
comments to ensure the purpose of the relevant dns_name_equal() check is
clear.
---

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index e10404859d9..ddc0ed50419 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -7343,22 +7343,34 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) {
 		}
 
 		/*
-		 * Get domain and nameservers from fctx->nsfetch
-		 * before we destroy it.
+		 * Get domain from fctx->nsfetch before we destroy it.
+		 */
+		domain = dns_fixedname_initname(&fixed);
+		dns_name_copy(fctx->nsfetch->private->domain, domain);
+
+		/*
+		 * If the chain of resume_dslookup() invocations managed to
+		 * chop off enough labels from the original DS owner name to
+		 * reach the top of the namespace, no further progress can be
+		 * made.  Interrupt the DS chasing process, returning SERVFAIL.
+		 */
+		if (dns_name_equal(fctx->nsname, domain)) {
+			dns_resolver_destroyfetch(&fctx->nsfetch);
+			fctx_done_detach(&fctx, DNS_R_SERVFAIL);
+			return;
+		}
+
+		/*
+		 * Get nameservers from fctx->nsfetch before we destroy it.
 		 */
 		dns_rdataset_init(&nameservers);
 		if (dns_rdataset_isassociated(
 			    &fctx->nsfetch->private->nameservers)) {
-			domain = dns_fixedname_initname(&fixed);
-			dns_name_copy(fctx->nsfetch->private->domain, domain);
-			if (dns_name_equal(fctx->nsname, domain)) {
-				dns_resolver_destroyfetch(&fctx->nsfetch);
-				fctx_done_detach(&fctx, DNS_R_SERVFAIL);
-				return;
-			}
 			dns_rdataset_clone(&fctx->nsfetch->private->nameservers,
 					   &nameservers);
 			nsrdataset = &nameservers;
+		} else {
+			domain = NULL;
 		}
 
 		dns_resolver_destroyfetch(&fctx->nsfetch);