From 5ab4cae4ed5c2f7b8509a0dfcbadbc727abecf36 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 22 May 2024 13:02:16 -0700 Subject: [PATCH 1/7] attach query counter to NS fetches there were cases in resolver.c when queries for NS records were started without passing a pointer to the parent fetch's query counter; as a result, the max-recursion-queries quota for those queries started counting from zero, instead of sharing the limit for the parent fetch, making the quota ineffective in some cases. (cherry picked from commit d3b7e92783754e9a4ce93046fadcb96c5439a0d7) --- lib/dns/resolver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 70ede1d515..0b22397ad5 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -7567,8 +7567,9 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) { options = fctx->options & ~DNS_FETCHOPT_TRYSTALE_ONTIMEOUT; result = dns_resolver_createfetch( res, fctx->nsname, dns_rdatatype_ns, domain, nsrdataset, - NULL, NULL, 0, options, 0, NULL, task, resume_dslookup, - fctx, &fctx->nsrrset, NULL, &fctx->nsfetch); + NULL, NULL, 0, options, 0, fctx->qc, task, + resume_dslookup, fctx, &fctx->nsrrset, NULL, + &fctx->nsfetch); if (result != ISC_R_SUCCESS) { if (result == DNS_R_DUPLICATE) { result = DNS_R_SERVFAIL; @@ -9934,7 +9935,7 @@ rctx_chaseds(respctx_t *rctx, dns_message_t *message, options = fctx->options & ~DNS_FETCHOPT_TRYSTALE_ONTIMEOUT; result = dns_resolver_createfetch( fctx->res, fctx->nsname, dns_rdatatype_ns, NULL, NULL, NULL, - NULL, 0, options, 0, NULL, task, resume_dslookup, fctx, + NULL, 0, options, 0, fctx->qc, task, resume_dslookup, fctx, &fctx->nsrrset, NULL, &fctx->nsfetch); if (result != ISC_R_SUCCESS) { if (result == DNS_R_DUPLICATE) { From 18e39d989f5a716045cd6d99b3bdb7a2633a2db8 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 22 May 2024 15:17:47 -0700 Subject: [PATCH 2/7] apply max-recursion-queries quota to validator queries previously, validator queries for DNSKEY and DS records were not counted toward the quota for max-recursion-queries; they are now. (cherry picked from commit af7db8951364a89c468eda1535efb3f53adc2c1f) --- lib/dns/include/dns/validator.h | 15 ++++++++------- lib/dns/resolver.c | 2 +- lib/dns/validator.c | 12 ++++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 352a60a6a0..2c758833f0 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -144,12 +144,13 @@ struct dns_validator { dns_fixedname_t wild; dns_fixedname_t closest; ISC_LINK(dns_validator_t) link; - bool mustbesecure; - unsigned int depth; - unsigned int authcount; - unsigned int authfail; - bool failed; - isc_stdtime_t start; + bool mustbesecure; + unsigned int depth; + unsigned int authcount; + unsigned int authfail; + bool failed; + isc_stdtime_t start; + isc_counter_t *qc; }; /*% @@ -167,7 +168,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, dns_message_t *message, unsigned int options, isc_task_t *task, isc_taskaction_t action, void *arg, - dns_validator_t **validatorp); + isc_counter_t *qc, dns_validator_t **validatorp); /*%< * Start a DNSSEC validation. * diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 0b22397ad5..83ae50b782 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -974,7 +974,7 @@ valcreate(fetchctx_t *fctx, dns_message_t *message, dns_adbaddrinfo_t *addrinfo, result = dns_validator_create(fctx->res->view, name, type, rdataset, sigrdataset, message, valoptions, task, - validated, valarg, &validator); + validated, valarg, fctx->qc, &validator); RUNTIME_CHECK(result == ISC_R_SUCCESS); if (result == ISC_R_SUCCESS) { inc_stats(fctx->res, dns_resstatscounter_val); diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 62647270a0..696a464ec1 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -1091,7 +1092,7 @@ create_validator(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, validator_logcreate(val, name, type, caller, "validator"); result = dns_validator_create(val->view, name, type, rdataset, sig, NULL, vopts, val->task, action, val, - &val->subvalidator); + val->qc, &val->subvalidator); if (result == ISC_R_SUCCESS) { val->subvalidator->parent = val; val->subvalidator->depth = val->depth + 1; @@ -3152,7 +3153,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, dns_message_t *message, unsigned int options, isc_task_t *task, isc_taskaction_t action, void *arg, - dns_validator_t **validatorp) { + isc_counter_t *qc, dns_validator_t **validatorp) { isc_result_t result = ISC_R_FAILURE; dns_validator_t *val; isc_task_t *tclone = NULL; @@ -3193,6 +3194,10 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, goto cleanup; } + if (qc != NULL) { + isc_counter_attach(qc, &val->qc); + } + val->mustbesecure = dns_resolver_getmustbesecure(view->resolver, name); dns_rdataset_init(&val->fdsset); dns_rdataset_init(&val->frdataset); @@ -3297,6 +3302,9 @@ destroy(dns_validator_t *val) { if (val->siginfo != NULL) { isc_mem_put(mctx, val->siginfo, sizeof(*val->siginfo)); } + if (val->qc != NULL) { + isc_counter_detach(&val->qc); + } isc_mutex_destroy(&val->lock); dns_view_weakdetach(&val->view); isc_mem_put(mctx, val, sizeof(*val)); From 14bce7e27582f97d2e5be43a8e88df84e241c5cb Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 25 Jun 2024 11:02:49 -0700 Subject: [PATCH 3/7] add debug logging when creating or attaching to a query counter fctx_create() now logs at debug level 9 when the fctx attaches to an existing counter or creates a new one. (cherry picked from commit 825f3d68c5b041b53f13a8c5b4dc431ca9b5887f) --- lib/dns/resolver.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 83ae50b782..cdb3c6e785 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -4809,16 +4809,6 @@ fctx_create(dns_resolver_t *res, isc_task_t *task, const dns_name_t *name, dns_resolver_attach(res, &fctx->res); - if (qc != NULL) { - isc_counter_attach(qc, &fctx->qc); - } else { - result = isc_counter_create(res->mctx, res->maxqueries, - &fctx->qc); - if (result != ISC_R_SUCCESS) { - goto cleanup_fetch; - } - } - /* * Make fctx->info point to a copy of a formatted string * "name/type". FCTXTRACE won't work until this is done. @@ -4831,6 +4821,24 @@ fctx_create(dns_resolver_t *res, isc_task_t *task, const dns_name_t *name, FCTXTRACE("create"); + if (qc != NULL) { + isc_counter_attach(qc, &fctx->qc); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(9), + "fctx %p(%s): attached to counter %p (%d)", fctx, + fctx->info, fctx->qc, isc_counter_used(fctx->qc)); + } else { + result = isc_counter_create(res->mctx, res->maxqueries, + &fctx->qc); + if (result != ISC_R_SUCCESS) { + goto cleanup_fetch; + } + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(9), + "fctx %p(%s): created counter %p", fctx, + fctx->info, fctx->qc); + } + isc_refcount_init(&fctx->references, 1); ISC_LIST_INIT(fctx->queries); From dd88a4cdfc8042e2cd2fdae93b803ba8490b88dc Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 25 Jun 2024 12:28:23 -0700 Subject: [PATCH 4/7] reduce MAX_RESTARTS to 11 the number of steps that can be followed in a CNAME chain before terminating the lookup has been reduced from 16 to 11. (this is a hard-coded value, but will be made configurable later.) (cherry picked from commit 05d78671bb6a5ba63d78d77339e17cbc73f18188) --- bin/tests/system/chain/tests.sh | 2 +- bin/tests/system/resolver/tests.sh | 2 +- lib/dns/client.c | 2 +- lib/ns/query.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bin/tests/system/chain/tests.sh b/bin/tests/system/chain/tests.sh index 6330dafdc8..49a18356ca 100644 --- a/bin/tests/system/chain/tests.sh +++ b/bin/tests/system/chain/tests.sh @@ -454,7 +454,7 @@ 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 +grep "ANSWER: 12" dig.out.test$n >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 2a3774675b..d74b1be6d5 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -229,7 +229,7 @@ dig_with_opts +tcp longcname1.example.net @10.53.0.1 a >dig.out.ns1.test${n} || 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 +test ${lines:-1} -eq 12 || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) diff --git a/lib/dns/client.c b/lib/dns/client.c index 9d8eb87e00..7ac358df02 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -60,7 +60,7 @@ #define UCTX_MAGIC ISC_MAGIC('U', 'c', 't', 'x') #define UCTX_VALID(c) ISC_MAGIC_VALID(c, UCTX_MAGIC) -#define MAX_RESTARTS 16 +#define MAX_RESTARTS 11 #ifdef TUNE_LARGE #define RESOLVER_NTASKS 523 diff --git a/lib/ns/query.c b/lib/ns/query.c index 866ea9d509..b712fe2fb2 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -89,7 +89,7 @@ * Maximum number of chained queries before we give up * to prevent CNAME loops. */ -#define MAX_RESTARTS 16 +#define MAX_RESTARTS 11 #define QUERY_ERROR(qctx, r) \ do { \ From bfbc6a6c840461a530077f2d5b02f9a53500f8ce Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 25 Jun 2024 14:30:20 -0700 Subject: [PATCH 5/7] make "max_restarts" a configurable value MAX_RESTARTS is no longer hard-coded; ns_server_setmaxrestarts() and dns_client_setmaxrestarts() can now be used to modify the max-restarts value at runtime. in both cases, the default is 11. (cherry picked from commit c5588babaf89f3e3ad2edccaada716e55c135dd3) --- bin/delv/delv.c | 8 ++++++++ lib/dns/client.c | 22 ++++++++++++++++++---- lib/dns/include/dns/client.h | 13 +++++++++++++ lib/dns/include/dns/view.h | 13 +++++++++++++ lib/dns/view.c | 16 ++++++++++++++++ lib/ns/include/ns/server.h | 1 + lib/ns/query.c | 14 +++++--------- lib/ns/server.c | 2 +- 8 files changed, 75 insertions(+), 14 deletions(-) diff --git a/bin/delv/delv.c b/bin/delv/delv.c index c750629517..650d8cb61b 100644 --- a/bin/delv/delv.c +++ b/bin/delv/delv.c @@ -76,6 +76,12 @@ #define MAXNAME (DNS_NAME_MAXTEXT + 1) +/* + * Default maximum number of chained queries before we give up + * to prevent CNAME loops. + */ +#define MAX_RESTARTS 11 + /* Variables used internally by delv. */ char *progname; static isc_mem_t *mctx = NULL; @@ -1776,6 +1782,8 @@ main(int argc, char *argv[]) { goto cleanup; } + dns_client_setmaxrestarts(client, MAX_RESTARTS); + /* Set the nameserver */ if (server != NULL) { addserver(client); diff --git a/lib/dns/client.c b/lib/dns/client.c index 7ac358df02..16f1facb91 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -60,8 +60,6 @@ #define UCTX_MAGIC ISC_MAGIC('U', 'c', 't', 'x') #define UCTX_VALID(c) ISC_MAGIC_VALID(c, UCTX_MAGIC) -#define MAX_RESTARTS 11 - #ifdef TUNE_LARGE #define RESOLVER_NTASKS 523 #else /* ifdef TUNE_LARGE */ @@ -95,6 +93,7 @@ struct dns_client { unsigned int find_timeout; unsigned int find_udpretries; + uint8_t max_restarts; isc_refcount_t references; @@ -105,6 +104,7 @@ struct dns_client { #define DEF_FIND_TIMEOUT 5 #define DEF_FIND_UDPRETRIES 3 +#define DEF_MAX_RESTARTS 11 /*% * Internal state for a single name resolution procedure @@ -281,7 +281,11 @@ dns_client_create(isc_mem_t *mctx, isc_appctx_t *actx, isc_taskmgr_t *taskmgr, client = isc_mem_get(mctx, sizeof(*client)); *client = (dns_client_t){ - .actx = actx, .taskmgr = taskmgr, .timermgr = timermgr, .nm = nm + .actx = actx, + .taskmgr = taskmgr, + .timermgr = timermgr, + .nm = nm, + .max_restarts = DEF_MAX_RESTARTS, }; isc_mutex_init(&client->lock); @@ -474,6 +478,14 @@ dns_client_clearservers(dns_client_t *client, dns_rdataclass_t rdclass, return (result); } +void +dns_client_setmaxrestarts(dns_client_t *client, uint8_t max_restarts) { + REQUIRE(DNS_CLIENT_VALID(client)); + REQUIRE(max_restarts > 0); + + client->max_restarts = max_restarts; +} + static isc_result_t getrdataset(isc_mem_t *mctx, dns_rdataset_t **rdatasetp) { dns_rdataset_t *rdataset; @@ -886,7 +898,9 @@ client_resfind(resctx_t *rctx, dns_fetchevent_t *event) { /* * Limit the number of restarts. */ - if (want_restart && rctx->restarts == MAX_RESTARTS) { + if (want_restart && + rctx->restarts == rctx->client->max_restarts) + { want_restart = false; result = ISC_R_QUOTA; send_event = true; diff --git a/lib/dns/include/dns/client.h b/lib/dns/include/dns/client.h index ec70f92d88..e5908973bd 100644 --- a/lib/dns/include/dns/client.h +++ b/lib/dns/include/dns/client.h @@ -189,6 +189,19 @@ dns_client_clearservers(dns_client_t *client, dns_rdataclass_t rdclass, *\li Anything else Failure. */ +void +dns_client_setmaxrestarts(dns_client_t *client, uint8_t max_restarts); +/*%< + * Set the number of permissible chained queries before we give up, + * to prevent CNAME loops. This defaults to 11. + * + * Requires: + * + *\li 'client' is a valid client. + + *\li 'max_restarts' is greater than 0. + */ + isc_result_t dns_client_resolve(dns_client_t *client, const dns_name_t *name, dns_rdataclass_t rdclass, dns_rdatatype_t type, diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 516c209b92..f5e274ef3d 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -193,6 +193,7 @@ struct dns_view { dns_badcache_t *failcache; uint32_t maxrrperset; uint32_t maxtypepername; + uint8_t max_restarts; /* * Configurable data for server use only, @@ -1427,4 +1428,16 @@ dns_view_setmaxtypepername(dns_view_t *view, uint32_t value); * Set the maximum resource record types per owner name that can be cached. */ +void +dns_view_setmaxrestarts(dns_view_t *view, uint8_t max_restarts); +/*%< + * Set the number of permissible chained queries before we give up, + * to prevent CNAME loops. This defaults to 11. + * + * Requires: + * + *\li 'view' is valid; + *\li 'max_restarts' is greater than 0. + */ + ISC_LANG_ENDDECLS diff --git a/lib/dns/view.c b/lib/dns/view.c index 231041efab..7b8520acd7 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -88,6 +88,12 @@ adb_shutdown(isc_task_t *task, isc_event_t *event); static void req_shutdown(isc_task_t *task, isc_event_t *event); +/*% + * Default maximum number of chained queries before we give up + * to prevent CNAME loops. + */ +#define DEFAULT_MAX_RESTARTS 11 + isc_result_t dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp) { @@ -266,6 +272,8 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, view->hooktable = NULL; view->hooktable_free = NULL; + view->max_restarts = DEFAULT_MAX_RESTARTS; + isc_mutex_init(&view->new_zone_lock); result = dns_order_create(view->mctx, &view->order); @@ -2780,3 +2788,11 @@ dns_view_setmaxtypepername(dns_view_t *view, uint32_t value) { dns_cache_setmaxtypepername(view->cache, value); } } + +void +dns_view_setmaxrestarts(dns_view_t *view, uint8_t max_restarts) { + REQUIRE(DNS_VIEW_VALID(view)); + REQUIRE(max_restarts > 0); + + view->max_restarts = max_restarts; +} diff --git a/lib/ns/include/ns/server.h b/lib/ns/include/ns/server.h index 206c495dc0..6e4309b000 100644 --- a/lib/ns/include/ns/server.h +++ b/lib/ns/include/ns/server.h @@ -100,6 +100,7 @@ struct ns_server { uint16_t transfer_tcp_message_size; bool interface_auto; dns_tkeyctx_t *tkeyctx; + uint8_t max_restarts; /*% Server id for NSID */ char *server_id; diff --git a/lib/ns/query.c b/lib/ns/query.c index b712fe2fb2..5549e202df 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -85,12 +85,6 @@ #define dns64_bis_return_excluded_addresses 1 #endif /* if 0 */ -/*% - * Maximum number of chained queries before we give up - * to prevent CNAME loops. - */ -#define MAX_RESTARTS 11 - #define QUERY_ERROR(qctx, r) \ do { \ (qctx)->result = r; \ @@ -2095,10 +2089,10 @@ addname: /* * In some cases, a record that has been added as additional * data may *also* trigger the addition of additional data. - * This cannot go more than MAX_RESTARTS levels deep. + * This cannot go more than 'max-restarts' levels deep. */ if (trdataset != NULL && dns_rdatatype_followadditional(type)) { - if (client->additionaldepth++ < MAX_RESTARTS) { + if (client->additionaldepth++ < client->view->max_restarts) { eresult = dns_rdataset_additionaldata( trdataset, fname, query_additional_cb, qctx); } @@ -11945,7 +11939,9 @@ ns_query_done(query_ctx_t *qctx) { * Do we need to restart the query (e.g. for CNAME chaining)? */ if (qctx->want_restart) { - if (qctx->client->query.restarts < MAX_RESTARTS) { + if (qctx->client->query.restarts < + qctx->client->view->max_restarts) + { qctx->client->query.restarts++; return (ns__query_start(qctx)); } else { diff --git a/lib/ns/server.c b/lib/ns/server.c index 7b245c39c7..bea4f36a47 100644 --- a/lib/ns/server.c +++ b/lib/ns/server.c @@ -38,7 +38,7 @@ isc_result_t ns_server_create(isc_mem_t *mctx, ns_matchview_t matchingview, ns_server_t **sctxp) { - ns_server_t *sctx; + ns_server_t *sctx = NULL; isc_result_t result; REQUIRE(sctxp != NULL && *sctxp == NULL); From a11367ade3f4ebd314c31a1ef45965e3859b5095 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 25 Jun 2024 14:39:58 -0700 Subject: [PATCH 6/7] reduce the max-recursion-queries default to 32 the number of iterative queries that can be sent to resolve a name now defaults to 32 rather than 100. (cherry picked from commit 7e3b425dc283df66df9c46002307ab676e10e4fd) --- bin/named/config.c | 2 +- bin/tests/system/reclimit/ns3/named1.conf.in | 1 + bin/tests/system/resolver/ns1/named.conf.in | 1 + doc/arm/reference.rst | 9 ++++++--- lib/dns/resolver.c | 2 +- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/bin/named/config.c b/bin/named/config.c index 47549c54f8..3640f714ac 100644 --- a/bin/named/config.c +++ b/bin/named/config.c @@ -174,7 +174,7 @@ options {\n\ max-clients-per-query 100;\n\ max-ncache-ttl 10800; /* 3 hours */\n\ max-recursion-depth 7;\n\ - max-recursion-queries 100;\n\ + max-recursion-queries 32;\n\ max-stale-ttl 86400; /* 1 day */\n\ message-compression yes;\n\ min-ncache-ttl 0; /* 0 hours */\n\ diff --git a/bin/tests/system/reclimit/ns3/named1.conf.in b/bin/tests/system/reclimit/ns3/named1.conf.in index 58b5d02514..36e5888f02 100644 --- a/bin/tests/system/reclimit/ns3/named1.conf.in +++ b/bin/tests/system/reclimit/ns3/named1.conf.in @@ -22,6 +22,7 @@ options { listen-on-v6 { none; }; servfail-ttl 0; qname-minimization disabled; + max-recursion-queries 50; max-recursion-depth 12; recursion yes; dnssec-validation yes; diff --git a/bin/tests/system/resolver/ns1/named.conf.in b/bin/tests/system/resolver/ns1/named.conf.in index f0f9571209..31321ad729 100644 --- a/bin/tests/system/resolver/ns1/named.conf.in +++ b/bin/tests/system/resolver/ns1/named.conf.in @@ -29,6 +29,7 @@ options { allow-query {!10.53.0.8; any; }; max-zone-ttl unlimited; attach-cache "globalcache"; + max-recursion-queries 50; }; server 10.53.0.3 { diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index a075e29bef..23422d1ba5 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4690,9 +4690,12 @@ Tuning :tags: server, query :short: Sets the maximum number of iterative queries while servicing a recursive query. - This sets the maximum number of iterative queries that may be sent while - servicing a recursive query. If more queries are sent, the recursive - query is terminated and returns SERVFAIL. The default is 100. + This sets the maximum number of iterative queries that may be sent + by a resolver while looking up a single name. If more queries than this + need to be sent before an answer is reached, then recursion is terminated + and a SERVFAIL response is returned to the client. (Note: if the answer + is a CNAME, then the subsequent lookup for the target of the CNAME is + counted separately.) The default is 32. .. namedconf:statement:: notify-delay :tags: transfer, zone diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index cdb3c6e785..f8f53d2650 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -199,7 +199,7 @@ /* The default maximum number of iterative queries to allow before giving up. */ #ifndef DEFAULT_MAX_QUERIES -#define DEFAULT_MAX_QUERIES 100 +#define DEFAULT_MAX_QUERIES 50 #endif /* ifndef DEFAULT_MAX_QUERIES */ /* From 2e04f0380c5af65661ee906ffc0730e6ea8040aa Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 25 Jun 2024 23:49:00 -0700 Subject: [PATCH 7/7] implement 'max-query-restarts' implement, document, and test the 'max-query-restarts' option which specifies the query restart limit - the number of times we can follow CNAMEs before terminating resolution. (cherry picked from commit 104f3b82fb7c7cd03edc36507b167cfc6e11d17c) --- bin/named/config.c | 1 + bin/named/server.c | 5 +++++ bin/tests/system/chain/ns7/named.conf.in | 23 ++++++++++++++++++++--- bin/tests/system/chain/tests.sh | 12 +++++++----- bin/tests/system/checkconf/good.conf | 2 ++ doc/arm/reference.rst | 8 ++++++++ doc/misc/options | 2 ++ lib/bind9/check.c | 14 ++++++++++++++ lib/isccfg/namedconf.c | 1 + 9 files changed, 60 insertions(+), 8 deletions(-) diff --git a/bin/named/config.c b/bin/named/config.c index 3640f714ac..1fe74d423f 100644 --- a/bin/named/config.c +++ b/bin/named/config.c @@ -175,6 +175,7 @@ options {\n\ max-ncache-ttl 10800; /* 3 hours */\n\ max-recursion-depth 7;\n\ max-recursion-queries 32;\n\ + max-query-restarts 11;\n\ max-stale-ttl 86400; /* 1 day */\n\ message-compression yes;\n\ min-ncache-ttl 0; /* 0 hours */\n\ diff --git a/bin/named/server.c b/bin/named/server.c index 9ad6d6046b..40b808a817 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -5585,6 +5585,11 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, INSIST(result == ISC_R_SUCCESS); dns_resolver_setmaxqueries(view->resolver, cfg_obj_asuint32(obj)); + obj = NULL; + result = named_config_get(maps, "max-query-restarts", &obj); + INSIST(result == ISC_R_SUCCESS); + dns_view_setmaxrestarts(view, cfg_obj_asuint32(obj)); + obj = NULL; result = named_config_get(maps, "fetches-per-zone", &obj); INSIST(result == ISC_R_SUCCESS); diff --git a/bin/tests/system/chain/ns7/named.conf.in b/bin/tests/system/chain/ns7/named.conf.in index 32c9b5f569..4cc6ac75c1 100644 --- a/bin/tests/system/chain/ns7/named.conf.in +++ b/bin/tests/system/chain/ns7/named.conf.in @@ -35,11 +35,28 @@ key rndc_key { algorithm @DEFAULT_HMAC@; }; +key restart16 { + secret "1234abcd8765"; + algorithm @DEFAULT_HMAC@; +}; + controls { inet 10.53.0.7 port @CONTROLPORT@ allow { any; } keys { rndc_key; }; }; -zone "." { - type hint; - file "root.hint"; +view restart16 { + match-clients { key restart16; none; }; + max-query-restarts 16; + + zone "." { + type hint; + file "root.hint"; + }; +}; + +view default { + zone "." { + type hint; + file "root.hint"; + }; }; diff --git a/bin/tests/system/chain/tests.sh b/bin/tests/system/chain/tests.sh index 49a18356ca..25e617bc5c 100644 --- a/bin/tests/system/chain/tests.sh +++ b/bin/tests/system/chain/tests.sh @@ -442,11 +442,13 @@ n=$((n + 1)) 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: 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)) +$DIG $DIGOPTS @10.53.0.7 loop.example >dig.out.1.test$n +grep "status: NOERROR" dig.out.1.test$n >/dev/null || ret=1 +grep "ANSWER: 12" dig.out.1.test$n >/dev/null || ret=1 +# also check with max-query-restarts 16: +$DIG $DIGOPTS @10.53.0.7 -y "${DEFAULT_HMAC}:restart16:1234abcd8765" loop.example >dig.out.2.test$n +grep "status: NOERROR" dig.out.2.test$n >/dev/null || ret=1 +grep "ANSWER: 17" dig.out.2.test$n >/dev/null || ret=1 n=$((n + 1)) echo_i "checking CNAME loops are detected (auth) ($n)" diff --git a/bin/tests/system/checkconf/good.conf b/bin/tests/system/checkconf/good.conf index 7e0f87901c..4b0518e457 100644 --- a/bin/tests/system/checkconf/good.conf +++ b/bin/tests/system/checkconf/good.conf @@ -77,6 +77,7 @@ options { check-names primary warn; check-names secondary ignore; max-cache-size 20000000000000; + max-query-restarts 10; nta-lifetime 604800; nta-recheck 604800; validate-except { @@ -109,6 +110,7 @@ view "first" { max-ixfr-ratio unlimited; }; dnssec-validation auto; + max-query-restarts 15; zone-statistics terse; }; view "second" { diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 23422d1ba5..3fc76fa735 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4697,6 +4697,14 @@ Tuning is a CNAME, then the subsequent lookup for the target of the CNAME is counted separately.) The default is 32. +.. namedconf:statement:: max-query-restarts + :tags: server, query + :short: Sets the maximum number of chained CNAMEs to follow + + This sets the maximum number of successive CNAME targets to follow + when resolving a client query, before terminating the query to avoid a + CNAME loop. Valid values are 1 to 255. The default is 11. + .. namedconf:statement:: notify-delay :tags: transfer, zone :short: Sets the delay (in seconds) between sending sets of NOTIFY messages for a zone. diff --git a/doc/misc/options b/doc/misc/options index a8cd16425c..8c01d57237 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -180,6 +180,7 @@ options { max-ixfr-ratio ( unlimited | ); max-journal-size ( default | unlimited | ); max-ncache-ttl ; + max-query-restarts ; max-records ; max-records-per-type ; max-recursion-depth ; @@ -472,6 +473,7 @@ view [ ] { max-ixfr-ratio ( unlimited | ); max-journal-size ( default | unlimited | ); max-ncache-ttl ; + max-query-restarts ; max-records ; max-records-per-type ; max-recursion-depth ; diff --git a/lib/bind9/check.c b/lib/bind9/check.c index 1c850d8e5a..962872bdc1 100644 --- a/lib/bind9/check.c +++ b/lib/bind9/check.c @@ -1943,6 +1943,20 @@ check_options(const cfg_obj_t *options, const cfg_obj_t *config, } } + obj = NULL; + (void)cfg_map_get(options, "max-query-restarts", &obj); + if (obj != NULL) { + uint32_t restarts = cfg_obj_asuint32(obj); + if (restarts == 0 || restarts > 255) { + cfg_obj_log(obj, logctx, ISC_LOG_ERROR, + "'max-query-restarts' is out of " + "range 1..255)"); + if (result == ISC_R_SUCCESS) { + result = ISC_R_RANGE; + } + } + } + if (actx != NULL) { cfg_aclconfctx_detach(&actx); } diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index 0b78e3c5d6..c1aa72c1e1 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -2105,6 +2105,7 @@ static cfg_clausedef_t view_clauses[] = { { "max-ncache-ttl", &cfg_type_duration, 0 }, { "max-recursion-depth", &cfg_type_uint32, 0 }, { "max-recursion-queries", &cfg_type_uint32, 0 }, + { "max-query-restarts", &cfg_type_uint32, 0 }, { "max-stale-ttl", &cfg_type_duration, 0 }, { "max-udp-size", &cfg_type_uint32, 0 }, { "message-compression", &cfg_type_boolean, 0 },