From 946931ccb710b45c538a57f9c9e404ea87591ed9 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 | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 132a151bed..866ea9d509 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -11910,7 +11910,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"); @@ -11944,13 +11944,36 @@ 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) { - qctx->client->query.restarts++; - return (ns__query_start(qctx)); + if (qctx->want_restart) { + if (qctx->client->query.restarts < MAX_RESTARTS) { + qctx->client->query.restarts++; + return (ns__query_start(qctx)); + } 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 27f6fc915c584c860273489e54fac628b599bbbd 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 893c9ed8d5..880848e9c2 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 0296ad6413..83e0ed404c 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -222,6 +222,17 @@ if [ -x "${RESOLVE}" ]; then status=$((status + ret)) fi +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 6bad06ea2eaddbb0c73af067f932e382347674f9 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 4957fe769b..6330dafdc8 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))