[9.18] new: usr: Tighten 'max-recursion-queries' and add 'max-query-restarts' option

There were cases in resolver.c when the `max-recursion-queries` quota was ineffective. It was possible to craft zones that would cause a resolver to waste resources by sending excessive queries while attempting to resolve a name. This has been addressed by correcting errors in the implementation of `max-recursion-queries`, and by reducing the default value from 100 to 32.

In addition, a new `max-query-restarts` option has been added which limits the number of times a recursive server will follow CNAME or DNAME records before terminating resolution. This was previously a hard-coded limit of 16, and now defaults to 11.
 
Closes #4741

Backport of MR !9281

Merge branch 'backport-4741-reclimit-restarts-9.18' into 'bind-9.18'

See merge request isc-projects/bind9!9283
This commit is contained in:
Evan Hunt 2024-08-07 23:22:22 +00:00
commit fe3ae71e90
23 changed files with 188 additions and 52 deletions

View file

@ -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);

View file

@ -174,7 +174,8 @@ 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-query-restarts 11;\n\
max-stale-ttl 86400; /* 1 day */\n\
message-compression yes;\n\
min-ncache-ttl 0; /* 0 hours */\n\

View file

@ -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);

View file

@ -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";
};
};

View file

@ -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)"
@ -454,7 +456,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))

View file

@ -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" {

View file

@ -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;

View file

@ -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 {

View file

@ -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))

View file

@ -4690,9 +4690,20 @@ 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:: 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

View file

@ -180,6 +180,7 @@ options {
max-ixfr-ratio ( unlimited | <percentage> );
max-journal-size ( default | unlimited | <sizeval> );
max-ncache-ttl <duration>;
max-query-restarts <integer>;
max-records <integer>;
max-records-per-type <integer>;
max-recursion-depth <integer>;
@ -472,6 +473,7 @@ view <string> [ <class> ] {
max-ixfr-ratio ( unlimited | <percentage> );
max-journal-size ( default | unlimited | <sizeval> );
max-ncache-ttl <duration>;
max-query-restarts <integer>;
max-records <integer>;
max-records-per-type <integer>;
max-recursion-depth <integer>;

View file

@ -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);
}

View file

@ -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 16
#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;

View file

@ -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,

View file

@ -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.
*

View file

@ -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

View file

@ -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 */
/*
@ -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);
@ -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);
@ -7567,8 +7575,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 +9943,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) {

View file

@ -15,6 +15,7 @@
#include <stdbool.h>
#include <isc/base32.h>
#include <isc/counter.h>
#include <isc/md.h>
#include <isc/mem.h>
#include <isc/print.h>
@ -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));

View file

@ -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;
}

View file

@ -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 },

View file

@ -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;

View file

@ -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 16
#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 {

View file

@ -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);