Avoid resend loop when forwarder SERVFAILs with both CD=0 and CD=1

Commit `36cf1c6a5bf943ad718ddba9fbe6ea97810e3bc2` introduces the
`DNS_FETCHOPT_TRYCD` flag which enables, when sending a query to a
forwarder, the forwarder to validate the answer (CD=0). The crux is
that if for some reason the forwarder returns SERVFAIL, we can retry the
same query and disable the forwarder validation (CD=1) so the resolver
can attempt validation itself (or detect it's bogus).

The logic was to first set `DNS_FETCHOPT_TRYCD` to the query options but
not on the message (so CD=0), and, when getting a SERVFAIL answer, if
the option `DNS_FETCHOPT_TRYCD` was set, to also set it into the
message. However, there was no way to know if this was the first (or
second) query because the original message is discarded when getting the
answer. This can lead to an unbounded loop re-sending the same message
again and again (until the global query count stops it).

This is fixed by using two separate flags `DNS_FETCHOPT_TRYNOCD`, set on
the query options for the very first query, then, if it SERVFAIL,
check if `DNS_FETCHOPT_TRYNOCD` is set but `DNS_FETCHOPT_TRYCD` is not.
In this case, we know we're about to send the second query. If it also
fails, `DNS_FETCHOPT_TRYCD` will be set anyway, so there is no point
retrying. This breaks the unbounded loop.

diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h
index 8def29ed6b4eea7e7273483bddb955019032bea3..b5e964ec06b62b253d64f406e5a196a941afef48 100644 (file)
--- a/lib/dns/include/dns/resolver.h
+++ b/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,

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 699f833eb816cfcff8a306f19d5029c6bd34f6eb..4ad232a1838fcfd759959d1f584b4b590bb87e16 100644 (file)
--- a/lib/dns/resolver.c
+++ b/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