From 1a79aeab444d45236f71d6a21229fa121df188e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 13 Jul 2022 10:31:16 +0200 Subject: [PATCH 1/2] 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. --- lib/dns/resolver.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index e10404859d..ddc0ed5041 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); From cfa398ad373ca652402a330542dc9600ba1f29d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 13 Jul 2022 10:31:16 +0200 Subject: [PATCH 2/2] Add CHANGES entry and release note for GL #3439 --- CHANGES | 7 +++++++ doc/notes/notes-current.rst | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/CHANGES b/CHANGES index 96d37cabbd..49e43f3fa4 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,10 @@ +5925. [bug] With a forwarder configured for all queries, resolution + failures encountered during DS chasing could trigger + assertion failures due to a logic bug in + resume_dslookup() that caused it to call + dns_resolver_createfetch() with an invalid name. + [GL #3439] + 5924. [func] When it's necessary to use AXFR to respond to an IXFR request, a message explaining the reason is now logged at level info. [GL #2683] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index eecc87eb4c..b604010fcc 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -40,6 +40,12 @@ Feature Changes Bug Fixes ~~~~~~~~~ +- When running as a validating resolver forwarding all queries to + another resolver, :iscman:`named` could crash with an assertion + failure. These crashes occurred when the configured forwarder sent a + broken DS response and :iscman:`named` failed its attempts to find a + proper one instead. This has been fixed. :gl:`#3439` + - A DNS compression would be applied on the root zone name if it is repeatedly used in the same RRSet. :gl:`#3423`