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 e10404859d90fc42048d3a082ec80a6b92e98eb5..ddc0ed50419b6e8ecf78bce547080c9807c43589 100644 (file)
--- 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);