From 2b3ce5e51400d84d5e6e06704f7de4ab21fa0653 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 6 Jun 2024 12:06:59 +0000 Subject: [PATCH 1/3] Return SERVFAIL for a too long CNAME chain Due to the maximum query restart limitation a long CNAME chain it is cut after 16 queries but named still returns NOERROR. Return SERVFAIL instead and the partial answer. (cherry picked from commit b621f1d88e2a8a4e22d871e90e58ef4bd4b6050f) --- lib/ns/query.c | 49 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 1a0e74918c..d957826fc2 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -11546,7 +11546,7 @@ isc_result_t ns_query_done(query_ctx_t *qctx) { isc_result_t result = ISC_R_UNSET; const dns_namelist_t *secs = qctx->client->message->sections; - bool nodetach; + bool nodetach, partial_result_with_servfail = false; CCTRACE(ISC_LOG_DEBUG(3), "ns_query_done"); @@ -11580,21 +11580,44 @@ ns_query_done(query_ctx_t *qctx) { /* * Do we need to restart the query (e.g. for CNAME chaining)? */ - if (qctx->want_restart && qctx->client->query.restarts < MAX_RESTARTS) { - query_ctx_t *saved_qctx = NULL; - qctx->client->query.restarts++; - saved_qctx = isc_mem_get(qctx->client->manager->mctx, - sizeof(*saved_qctx)); - qctx_save(qctx, saved_qctx); - isc_nmhandle_attach(qctx->client->handle, - &qctx->client->restarthandle); - isc_async_run(qctx->client->manager->loop, async_restart, - saved_qctx); - return (DNS_R_CONTINUE); + if (qctx->want_restart) { + if (qctx->client->query.restarts < MAX_RESTARTS) { + query_ctx_t *saved_qctx = NULL; + qctx->client->query.restarts++; + saved_qctx = isc_mem_get(qctx->client->manager->mctx, + sizeof(*saved_qctx)); + qctx_save(qctx, saved_qctx); + isc_nmhandle_attach(qctx->client->handle, + &qctx->client->restarthandle); + isc_async_run(qctx->client->manager->loop, + async_restart, saved_qctx); + return (DNS_R_CONTINUE); + } else { + /* + * This is e.g. a long CNAME chain which we cut short. + */ + qctx->client->query.attributes |= + NS_QUERYATTR_PARTIALANSWER; + qctx->client->message->rcode = dns_rcode_servfail; + qctx->result = DNS_R_SERVFAIL; + + /* + * Send the answer back with a SERVFAIL result even + * if recursion was requested. + */ + partial_result_with_servfail = true; + + ns_client_extendederror(qctx->client, 0, + "max. restarts reached"); + ns_client_log(qctx->client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_QUERY, ISC_LOG_INFO, + "query iterations limit reached"); + } } if (qctx->result != ISC_R_SUCCESS && - (!PARTIALANSWER(qctx->client) || WANTRECURSION(qctx->client) || + (!PARTIALANSWER(qctx->client) || + (WANTRECURSION(qctx->client) && !partial_result_with_servfail) || qctx->result == DNS_R_DROP)) { if (qctx->result == DNS_R_DUPLICATE || From 21cdd8ed5bdccbec85b476bcfc501391da88a24a Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 6 Jun 2024 12:10:19 +0000 Subject: [PATCH 2/3] Test that a long CNAME chain causes SERVFAIL Also check that the expected partial answer in returned too. (cherry picked from commit 580f872fe13a85b71c5f2483aa297445759583c2) --- bin/tests/system/resolver/ans3/ans.pl | 3 +++ bin/tests/system/resolver/tests.sh | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/bin/tests/system/resolver/ans3/ans.pl b/bin/tests/system/resolver/ans3/ans.pl index 98f4ec04b5..85d46cd4eb 100644 --- a/bin/tests/system/resolver/ans3/ans.pl +++ b/bin/tests/system/resolver/ans3/ans.pl @@ -102,6 +102,9 @@ sub handleQuery { $packet->push("answer", new Net::DNS::RR($qname . " 300 CNAME goodcname.example.org")); + } elsif ($qname =~ /^longcname/) { + $cname = $qname =~ s/longcname/longcnamex/r; + $packet->push("answer", new Net::DNS::RR($qname . " 300 CNAME " . $cname)); } elsif ($qname =~ /^nodata\.example\.net$/i) { $packet->header->aa(1); } elsif ($qname =~ /^nxdomain\.example\.net$/i) { diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 69a0850e8d..fff57498c7 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -120,6 +120,17 @@ grep "status: NOERROR" dig.out.ns1.test${n} >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) +n=$((n + 1)) +echo_i "checking long CNAME chain target filtering (deny) ($n)" +ret=0 +dig_with_opts +tcp longcname1.example.net @10.53.0.1 a >dig.out.ns1.test${n} || ret=1 +grep -F "status: SERVFAIL" dig.out.ns1.test${n} >/dev/null || ret=1 +grep -F "max. restarts reached" dig.out.ns1.test${n} >/dev/null || ret=1 +lines=$(grep -F "CNAME" dig.out.ns1.test${n} | wc -l) +test ${lines:-1} -eq 17 || ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + n=$((n + 1)) echo_i "checking DNAME target filtering (deny) ($n)" ret=0 From b6372216ba363fb6c43529fc4f787010060ef4db Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 6 Jun 2024 20:49:34 +0000 Subject: [PATCH 3/3] Update the chain test Update the CNAME chain test to correspond to the changed behavior, because now named returns SERVFAIL when hitting the maximum query restarts limit (e.g. happening when following a long CNAME chain). In the current test auth will hit the limit and return partial data with a SERVFAIL code, while the resolver will return no data with a SERVFAIL code after auth returns SERVFAIL to it. (cherry picked from commit 7751c7eca6ad543106e2517e5e172ad30aeb6c76) --- bin/tests/system/chain/tests.sh | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/bin/tests/system/chain/tests.sh b/bin/tests/system/chain/tests.sh index dc49ff5141..2c87e123e4 100644 --- a/bin/tests/system/chain/tests.sh +++ b/bin/tests/system/chain/tests.sh @@ -439,11 +439,21 @@ if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) n=$((n + 1)) -echo_i "checking CNAME loops are detected ($n)" +echo_i "checking CNAME loops are detected (resolver) ($n)" ret=0 $RNDCCMD 10.53.0.7 null --- start test$n --- 2>&1 | sed 's/^/ns7 /' | cat_i $DIG $DIGOPTS @10.53.0.7 loop.example >dig.out.test$n -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 +grep "status: SERVFAIL" dig.out.test$n >/dev/null || ret=1 +grep "ANSWER: 0" dig.out.test$n >/dev/null || ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + +n=$((n + 1)) +echo_i "checking CNAME loops are detected (auth) ($n)" +ret=0 +$DIG $DIGOPTS @10.53.0.2 loop.example >dig.out.test$n +grep "status: SERVFAIL" dig.out.test$n >/dev/null || ret=1 +grep "max. restarts reached" dig.out.test$n >/dev/null || ret=1 grep "ANSWER: 17" dig.out.test$n >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret))