fix: usr: Fix recursion loop in case of badly behaving forwarders

When forwarding DNS queries, the CD bit is cleared on the first query, and the CD bit is only used as a fallback if the first query fails. However, due to a logic bug this could lead to an unbounded loop re-sending the same message, until the maximum query count is hit. This has been fixed.

Closes #5804

Merge branch '5804-resend-loop-forwarder-cd' into 'main'

See merge request isc-projects/bind9!12133

bin/tests/system/resend_loop/ans3/ans.py

@@ -22,6 +22,7 @@ from isctest.asyncserver import (
     AsyncDnsServer,
     DnsResponseSend,
     DomainHandler,
+    QnameHandler,
     QnameQtypeHandler,
     QueryContext,
     StaticResponseHandler,
@@ -78,12 +79,19 @@ class ExampleCookieHandler(DomainHandler):
             yield DnsResponseSend(qctx.response)
 
 
+class TestDotComServFailHandler(QnameHandler, StaticResponseHandler):
+    qnames = ["test.com."]
+    authoritative = False
+    rcode = dns.rcode.SERVFAIL
+
+
 def main() -> None:
     server = AsyncDnsServer(default_aa=True, default_rcode=dns.rcode.NOERROR)
     server.install_response_handlers(
         RootNsHandler(),
         ExampleNsHandler(),
         ExampleCookieHandler(),
+        TestDotComServFailHandler(),
     )
     server.run()
 

bin/tests/system/resend_loop/ns1/named.conf.j2

@@ -0,0 +1,13 @@
+options {
+	query-source address 10.53.0.1;
+	notify-source 10.53.0.1;
+	transfer-source 10.53.0.1;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.1; };
+	listen-on-v6 { none; };
+	recursion yes;
+	allow-query { any; };
+	forwarders { 10.53.0.3 port @PORT@; };
+	forward only;
+};

bin/tests/system/resend_loop/tests_resend_loop.py

@@ -83,3 +83,39 @@ def test_resend_loop_badcookie(ns4):
 
     prohibited_log = "query failed (timed out) for test.example/IN/A"
     assert prohibited_log not in ns4.log
+
+
+def test_resend_loop_forward(ns1):
+    sending_packet = Re("sending packet from 10.53.0.1#[0-9]+ to 10.53.0.3#[0-9]+")
+    received_packet = Re("received packet from 10.53.0.3#[0-9]+ to 10.53.0.1#[0-9]+")
+
+    # Make sure the server is done with priming and ./DNSKEY refresh (RFC5011)
+    msg = dns.message.make_query(".", "NS")
+    isctest.query.udp(msg, ns1.ip)
+    query_count_start = len(ns1.log.grep(sending_packet))
+
+    log_sequence = [
+        sending_packet,
+        "flags: rd; QUESTION: 1",
+        received_packet,
+        "opcode: QUERY, status: SERVFAIL",
+        "flags: qr rd; QUESTION: 1",
+        sending_packet,
+        "flags: rd cd; QUESTION: 1",
+        received_packet,
+        "opcode: QUERY, status: SERVFAIL",
+        "flags: qr rd; QUESTION: 1",
+    ]
+
+    msg = dns.message.make_query("test.com.", "A")
+    with ns1.watch_log_from_here() as watcher:
+        res = isctest.query.udp(msg, ns1.ip)
+        watcher.wait_for_sequence(log_sequence)
+
+    isctest.check.servfail(res)
+
+    query_count_end = len(ns1.log.grep(sending_packet))
+    assert query_count_end - query_count_start == 2
+
+    prohibited_log = "query failed (timed out) for test.com/IN/A"
+    assert prohibited_log not in ns1.log

lib/dns/include/dns/resolver.h

@@ -133,16 +133,19 @@ enum {
 						 * possible. */
 	DNS_FETCHOPT_QMINFETCH = 1 << 16,	/*%< Qmin fetch */
 	DNS_FETCHOPT_WANTZONEVERSION = 1 << 17, /*%< Request ZONEVERSION */
-	DNS_FETCHOPT_TRYCD = 1 << 18,		/*%< Send the first query
-						 * to a forwader with
-						 * CD=0, but retry with CD=1
-						 * if it returns SERVFAIL.
-						 */
-	DNS_FETCHOPT_PRIMING = 1 << 19,		/*%< Root priming fetch.
-						 * Copies the '.' NS answer
-						 * and root-server glue from
-						 * the response into
-						 * view->rootdb. */
+
+	/*
+	 * Send the first query to a forwarder with CD=0 (TRYNOCD), but retry
+	 * with CD=1 if it returns SERVFAIL (TRYCD).
+	 */
+	DNS_FETCHOPT_TRYNOCD = 1 << 18,
+	DNS_FETCHOPT_TRYCD = 1 << 19,
+
+	DNS_FETCHOPT_PRIMING = 1 << 20, /*%< Root priming fetch.
+					 * Copies the '.' NS answer
+					 * and root-server glue from
+					 * the response into
+					 * view->rootdb. */
 
 	/*% EDNS version bits: */
 	DNS_FETCHOPT_EDNSVERSIONSET = 1 << 23,

lib/dns/resolver.c

@@ -2533,11 +2533,31 @@ resquery_send(resquery_t *query) {
 	} else if ((query->options & DNS_FETCHOPT_NOVALIDATE) != 0 ||
 		   (query->options & DNS_FETCHOPT_TRYCD) != 0)
 	{
+		/*
+		 * `DNS_FETCHOPT_TRYCD` has been set by the code processing a
+		 * SERVFAIL response from a forwarder with CD=0. This is a
+		 * second (and last) attempt to resend the query with CD=1 to
+		 * the same server.
+		 */
 		fctx->qmessage->flags |= DNS_MESSAGEFLAG_CD;
 	} else if (res->view->enablevalidation &&
 		   ((fctx->qmessage->flags & DNS_MESSAGEFLAG_RD) != 0))
 	{
-		query->options |= DNS_FETCHOPT_TRYCD;
+		/*
+		 * RD flag is set, meaning either that the client requests
+		 * recursion or (non exclusive) we are asking a forwarder. The
+		 * logic about CD doesn't matter in the non-forwarder case, but
+		 * doesn't harm either (as an authoritative server will ignore
+		 * this). However, for the forwarder case, it means the
+		 * forwarder will attempt the validation.
+		 *
+		 * The query is sent with CD=0, and `DNS_FETCHOPT_TRYNOCD`
+		 * enables the code processing the response to know (if this is
+		 * a forwarder, which is re-checked in the code processing the
+		 * response) that we can re-send the query again, one more time,
+		 * with CD=1 (leaving the resolver doing the validation itself).
+		 */
+		query->options |= DNS_FETCHOPT_TRYNOCD;
 	}
 
 	/*
@@ -9993,7 +10013,8 @@ rctx_badserver(respctx_t *rctx, isc_result_t result) {
 		rctx->resend = true;
 	} else if (ISFORWARDER(query->addrinfo) &&
 		   query->rmessage->rcode == dns_rcode_servfail &&
-		   (query->options & DNS_FETCHOPT_TRYCD) != 0)
+		   (query->options & DNS_FETCHOPT_TRYNOCD) != 0 &&
+		   (query->options & DNS_FETCHOPT_TRYCD) == 0)
 	{
 		/*
 		 * We got a SERVFAIL from a forwarder with