From 9ae83a0e4ee0ac9362ad633a6e521c21b53dc492 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 9 Apr 2026 11:32:07 +0200 Subject: [PATCH 1/3] Add reproducer for BADCOOKIE resend loop Run malicious server: resend_loop/ans3/ans.py Start BIND: ns4 Send single query to test.example The resolver will repeatedly resend queries until the fetch timeout expires, resulting in resulting in thousands of qrysent while the quota counter remains 0. --- bin/tests/system/resend_loop/ans3/ans.py | 126 ++++++++++++++++++ .../system/resend_loop/ns4/named.conf.j2 | 16 +++ bin/tests/system/resend_loop/ns4/root.hint | 14 ++ .../system/resend_loop/tests_resend_loop.py | 28 ++++ 4 files changed, 184 insertions(+) create mode 100644 bin/tests/system/resend_loop/ans3/ans.py create mode 100644 bin/tests/system/resend_loop/ns4/named.conf.j2 create mode 100644 bin/tests/system/resend_loop/ns4/root.hint create mode 100644 bin/tests/system/resend_loop/tests_resend_loop.py diff --git a/bin/tests/system/resend_loop/ans3/ans.py b/bin/tests/system/resend_loop/ans3/ans.py new file mode 100644 index 0000000000..217bae0301 --- /dev/null +++ b/bin/tests/system/resend_loop/ans3/ans.py @@ -0,0 +1,126 @@ +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + +from collections.abc import AsyncGenerator + +import dns.edns +import dns.name +import dns.rcode +import dns.rdatatype +import dns.rrset + +from isctest.asyncserver import ( + AsyncDnsServer, + DnsResponseSend, + QueryContext, + ResponseHandler, +) + + +def _get_cookie(qctx: QueryContext): + for o in qctx.query.options: + if o.otype == dns.edns.OptionType.COOKIE: + cookie = o + try: + if len(cookie.server) == 0: + cookie.server = b"\x11\x22\x33\x44\x55\x66\x77\x88" + except AttributeError: # dnspython<2.7.0 compat + if len(o.data) == 8: + cookie.data *= 2 + + return cookie + + return None + + +class PrimeHandler(ResponseHandler): + """ + Specifically handle priming query for "." NS (type 2) + """ + + def match(self, qctx: QueryContext) -> bool: + return len(qctx.qname.labels) == 0 and qctx.qtype == dns.rdatatype.NS + + async def get_responses( + self, qctx: QueryContext + ) -> AsyncGenerator[DnsResponseSend, None]: + + ns_rrset = dns.rrset.from_text( + ".", dns.rdatatype.NS, qctx.qclass, "a.root-servers.nil." + ) + a_rrset = dns.rrset.from_text( + "a.root-servers.nil.", dns.rdatatype.A, qctx.qclass, "10.53.0.3" + ) + + response = qctx.prepare_new_response(with_zone_data=False) + response.set_rcode(dns.rcode.NOERROR) + response.answer.append(ns_rrset) + response.additional.append(a_rrset) + + yield DnsResponseSend(response, authoritative=True) + + +class CookieHandler(ResponseHandler): + def match(self, qctx: QueryContext) -> bool: + example = dns.name.from_text("example") + return qctx.qname.is_subdomain(example) + + async def get_responses( + self, qctx: QueryContext + ) -> AsyncGenerator[DnsResponseSend, None]: + + qctx.prepare_new_response() + + # Check for client cookie + cookie = _get_cookie(qctx) + + # If missing cookie entirely, just return SERVFAIL + if cookie is None: + qctx.response.set_rcode(dns.rcode.SERVFAIL) + yield DnsResponseSend(qctx.response, authoritative=True) + + # If there is a client cookie, mock BADCOOKIE to trigger + # the resend loop logic. + qctx.response.use_edns(options=[cookie]) + qctx.response.set_rcode(dns.rcode.BADCOOKIE) + yield DnsResponseSend(qctx.response, authoritative=True) + + +class NoErrorHandler(ResponseHandler): + """ + If the query is NOT a subdomain of example, respond with standard NOERROR empty answer + """ + + async def get_responses( + self, qctx: QueryContext + ) -> AsyncGenerator[DnsResponseSend, None]: + + qctx.prepare_new_response() + qctx.response.set_rcode(dns.rcode.NOERROR) + yield DnsResponseSend(qctx.response, authoritative=True) + + +def resend_server() -> AsyncDnsServer: + server = AsyncDnsServer(default_aa=True, default_rcode=dns.rcode.NOERROR) + server.install_response_handlers( + PrimeHandler(), + CookieHandler(), + NoErrorHandler(), + ) + return server + + +def main() -> None: + resend_server().run() + + +if __name__ == "__main__": + main() diff --git a/bin/tests/system/resend_loop/ns4/named.conf.j2 b/bin/tests/system/resend_loop/ns4/named.conf.j2 new file mode 100644 index 0000000000..360bc12e17 --- /dev/null +++ b/bin/tests/system/resend_loop/ns4/named.conf.j2 @@ -0,0 +1,16 @@ +options { + query-source address 10.53.0.4; + notify-source 10.53.0.4; + transfer-source 10.53.0.4; + port @PORT@; + pid-file "named.pid"; + listen-on { 10.53.0.4; }; + listen-on-v6 { none; }; + recursion yes; + dnssec-validation no; +}; + +zone "." IN { + type hint; + file "root.hint"; +}; diff --git a/bin/tests/system/resend_loop/ns4/root.hint b/bin/tests/system/resend_loop/ns4/root.hint new file mode 100644 index 0000000000..3889a8b353 --- /dev/null +++ b/bin/tests/system/resend_loop/ns4/root.hint @@ -0,0 +1,14 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; SPDX-License-Identifier: MPL-2.0 +; +; This Source Code Form is subject to the terms of the Mozilla Public +; License, v. 2.0. If a copy of the MPL was not distributed with this +; file, you can obtain one at https://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +$TTL 999999 +. IN NS a.root-servers.nil. +a.root-servers.nil. IN A 10.53.0.3 diff --git a/bin/tests/system/resend_loop/tests_resend_loop.py b/bin/tests/system/resend_loop/tests_resend_loop.py new file mode 100644 index 0000000000..f7ed4d3da6 --- /dev/null +++ b/bin/tests/system/resend_loop/tests_resend_loop.py @@ -0,0 +1,28 @@ +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + +import dns.message + +import isctest + + +def test_resend_loop_badcookie(ns4): + expected_log = "exceeded max queries resolving 'test.example/A'" + + msg = dns.message.make_query("test.example", "A") + with ns4.watch_log_from_here() as watcher: + res = isctest.query.udp(msg, ns4.ip) + watcher.wait_for_line(expected_log) + + isctest.check.servfail(res) + + prohibited_log = "query failed (timed out) for test.example/IN/A" + assert prohibited_log not in ns4.log From 11aae777a7da87974951ca75e58ddd49d72ed076 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Tue, 7 Apr 2026 22:18:10 +0200 Subject: [PATCH 2/3] Refactor incrementing query counters Move the logic incrementing the query counter and the global query counter into a dedicated helper function. --- lib/dns/resolver.c | 60 ++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 19c4c29f0d..5fed2760de 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -4285,6 +4285,39 @@ fctx_nextaddress(fetchctx_t *fctx) { return addrinfo; } +static isc_result_t +incr_query_counters(fetchctx_t *fctx) { + isc_result_t result; + + result = isc_counter_increment(fctx->qc); +#if WANT_QUERYTRACE + FCTXTRACE5("query", "max-recursion-queries, querycount=", + isc_counter_used(fctx->qc)); +#endif + if (result != ISC_R_SUCCESS) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, + ISC_LOG_DEBUG(3), + "exceeded max queries resolving '%s' " + "(max-recursion-queries, querycount=%u)", + fctx->info, isc_counter_used(fctx->qc)); + } else if (fctx->gqc != NULL) { + result = isc_counter_increment(fctx->gqc); +#if WANT_QUERYTRACE + FCTXTRACE5("query", "max-query-count, querycount=", + isc_counter_used(fctx->gqc)); +#endif + if (result != ISC_R_SUCCESS) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), + "exceeded global max queries resolving " + "'%s' (max-query-count, querycount=%u)", + fctx->info, isc_counter_used(fctx->gqc)); + } + } + + return result; +} + static void fctx_try(fetchctx_t *fctx, bool retrying) { isc_result_t result; @@ -4425,36 +4458,11 @@ fctx_try(fetchctx_t *fctx, bool retrying) { return; } - result = isc_counter_increment(fctx->qc); -#if WANT_QUERYTRACE - FCTXTRACE5("query", "max-recursion-queries, querycount=", - isc_counter_used(fctx->qc)); -#endif + result = incr_query_counters(fctx); if (result != ISC_R_SUCCESS) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, - ISC_LOG_DEBUG(3), - "exceeded max queries resolving '%s' " - "(max-recursion-queries, querycount=%u)", - fctx->info, isc_counter_used(fctx->qc)); goto done; } - if (fctx->gqc != NULL) { - result = isc_counter_increment(fctx->gqc); -#if WANT_QUERYTRACE - FCTXTRACE5("query", "max-query-count, querycount=", - isc_counter_used(fctx->gqc)); -#endif - if (result != ISC_R_SUCCESS) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, - DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), - "exceeded global max queries resolving " - "'%s' (max-query-count, querycount=%u)", - fctx->info, isc_counter_used(fctx->gqc)); - goto done; - } - } - result = fctx_query(fctx, addrinfo, fctx->options); if (result != ISC_R_SUCCESS) { goto done; From d9ee3b1de087ce07c768e339f52c645c1b981740 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Tue, 7 Apr 2026 22:18:58 +0200 Subject: [PATCH 3/3] rctx_resend() increment query counters Calls to `rctx_resend()` are done internally within the resolver, in flow which are not supposed to happens more than once. For instance, if some query fails, and a specific flag "F" wasn't set, then set the flag and try again. This wouldn't occur more than once because if the query fails the next attempt, the flag "F" would be set already, so the resolver would move to the next server (or give up). However, a subtle bug missing checking a flag, for instance, could lead to an unbounded loop re-trying to query the same server. This is now impossible as `rctx_resend()` also increment the query counters (so if such case occurs, it would stop once the maximum limit is reached). The dns_resstatscounter_retry are also only incremented if the `fctx_query()` succeeds, similar to as is done in `fctx_try()`. --- lib/dns/resolver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 5fed2760de..d7418d494a 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -9467,9 +9467,9 @@ rctx_nextserver(respctx_t *rctx, dns_message_t *message, * rctx_resend(): * * Resend the query, probably with the options changed. Calls - * fctx_query(), passing rctx->retryopts (which is based on - * query->options, but may have been updated since the last time - * fctx_query() was called). + * fctx_query(), unless query counter limits are hit, passing + * rctx->retryopts (which is based on query->options, but may have + * been updated since the last time fctx_query() was called). */ static void rctx_resend(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo) { @@ -9477,8 +9477,15 @@ rctx_resend(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo) { isc_result_t result; FCTXTRACE("resend"); - inc_stats(fctx->res, dns_resstatscounter_retry); + + CHECK(incr_query_counters(fctx)); + result = fctx_query(fctx, addrinfo, rctx->retryopts); + if (result == ISC_R_SUCCESS) { + inc_stats(fctx->res, dns_resstatscounter_retry); + } + +cleanup: if (result != ISC_R_SUCCESS) { fctx_failure_detach(&rctx->fctx, result); }