From 27f868f34c79cca80a1c3fd46353258be26c3a7f Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 6 Feb 2020 17:19:10 +1100 Subject: [PATCH 1/6] Reduce the number of fetches we make when looking up addresses If there are more that 5 NS record for a zone only perform a maximum of 4 address lookups for all the name servers. This limits the amount of remote lookup performed for server addresses at each level for a given query. --- lib/dns/adb.c | 10 +++++++--- lib/dns/include/dns/adb.h | 4 ++++ lib/dns/resolver.c | 38 +++++++++++++++++++++++++++++--------- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/lib/dns/adb.c b/lib/dns/adb.c index b41a19d94a..251fb592fb 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -414,6 +414,7 @@ static void log_quota(dns_adbentry_t *entry, const char *fmt, ...) #define FIND_GLUEOK(fn) (((fn)->options & DNS_ADBFIND_GLUEOK) != 0) #define FIND_HAS_ADDRS(fn) (!ISC_LIST_EMPTY((fn)->list)) #define FIND_RETURNLAME(fn) (((fn)->options & DNS_ADBFIND_RETURNLAME) != 0) +#define FIND_NOFETCH(fn) (((fn)->options & DNS_ADBFIND_NOFETCH) != 0) /* * These are currently used on simple unsigned ints, so they are @@ -3117,11 +3118,14 @@ dns_adb_createfind(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, fetch: if ((WANT_INET(wanted_addresses) && NAME_HAS_V4(adbname)) || (WANT_INET6(wanted_addresses) && NAME_HAS_V6(adbname))) + { have_address = true; - else + } else { have_address = false; - if (wanted_fetches != 0 && - ! (FIND_AVOIDFETCHES(find) && have_address)) { + } + if (wanted_fetches != 0 && !(FIND_AVOIDFETCHES(find) && have_address) && + !FIND_NOFETCH(find)) + { /* * We're missing at least one address family. Either the * caller hasn't instructed us to avoid fetches, or we don't diff --git a/lib/dns/include/dns/adb.h b/lib/dns/include/dns/adb.h index 5ba920c853..768668182f 100644 --- a/lib/dns/include/dns/adb.h +++ b/lib/dns/include/dns/adb.h @@ -207,6 +207,10 @@ struct dns_adbfind { * lame for this query. */ #define DNS_ADBFIND_OVERQUOTA 0x00000400 +/*% + * Don't perform a fetch even if there are no address records available. + */ +#define DNS_ADBFIND_NOFETCH 0x00000800 /*% * The answers to queries come back as a list of these. diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 73fc5763dc..4fdcbd821d 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -173,6 +173,14 @@ #define DEFAULT_MAX_QUERIES 75 #endif +/* + * After NS_FAIL_LIMIT attempts to fetch a name server address, + * if the number of addresses in the NS RRset exceeds NS_RR_LIMIT, + * stop trying to fetch, in order to avoid wasting resources. + */ +#define NS_FAIL_LIMIT 4 +#define NS_RR_LIMIT 5 + /* Number of hash buckets for zone counters */ #ifndef RES_DOMAIN_BUCKETS #define RES_DOMAIN_BUCKETS 523 @@ -3371,8 +3379,7 @@ sort_finds(dns_adbfindlist_t *findlist, unsigned int bias) { static void findname(fetchctx_t *fctx, const dns_name_t *name, in_port_t port, unsigned int options, unsigned int flags, isc_stdtime_t now, - bool *overquota, bool *need_alternate) -{ + bool *overquota, bool *need_alternate, unsigned int *no_addresses) { dns_adbaddrinfo_t *ai; dns_adbfind_t *find; dns_resolver_t *res; @@ -3465,8 +3472,12 @@ findname(fetchctx_t *fctx, const dns_name_t *name, in_port_t port, ((res->dispatches4 == NULL && find->result_v6 != DNS_R_NXDOMAIN) || (res->dispatches6 == NULL && - find->result_v4 != DNS_R_NXDOMAIN))) + find->result_v4 != DNS_R_NXDOMAIN))) { *need_alternate = true; + } + if (no_addresses != NULL) { + (*no_addresses)++; + } } else { if ((find->options & DNS_ADBFIND_OVERQUOTA) != 0) { if (overquota != NULL) @@ -3517,6 +3528,7 @@ fctx_getaddresses(fetchctx_t *fctx, bool badcache) { dns_rdata_ns_t ns; bool need_alternate = false; bool all_spilled = true; + unsigned int no_addresses = 0; FCTXTRACE5("getaddresses", "fctx->depth=", fctx->depth); @@ -3684,20 +3696,28 @@ fctx_getaddresses(fetchctx_t *fctx, bool badcache) { * Extract the name from the NS record. */ result = dns_rdata_tostruct(&rdata, &ns, NULL); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { continue; + } - findname(fctx, &ns.name, 0, stdoptions, 0, now, - &overquota, &need_alternate); + if (no_addresses > NS_FAIL_LIMIT && + dns_rdataset_count(&fctx->nameservers) > NS_RR_LIMIT) + { + stdoptions |= DNS_ADBFIND_NOFETCH; + } + findname(fctx, &ns.name, 0, stdoptions, 0, now, &overquota, + &need_alternate, &no_addresses); - if (!overquota) + if (!overquota) { all_spilled = false; + } dns_rdata_reset(&rdata); dns_rdata_freestruct(&ns); } - if (result != ISC_R_NOMORE) + if (result != ISC_R_NOMORE) { return (result); + } /* * Do we need to use 6 to 4? @@ -3712,7 +3732,7 @@ fctx_getaddresses(fetchctx_t *fctx, bool badcache) { if (!a->isaddress) { findname(fctx, &a->_u._n.name, a->_u._n.port, stdoptions, FCTX_ADDRINFO_FORWARDER, - now, NULL, NULL); + now, NULL, NULL, NULL); continue; } if (isc_sockaddr_pf(&a->_u.addr) != family) From b06df900b8504502bde4abc414bb6b309a86efb6 Mon Sep 17 00:00:00 2001 From: Stephen Morris Date: Thu, 5 Mar 2020 18:46:46 +0000 Subject: [PATCH 2/6] Add test for reduction in number of fetches Add a system test that counts how many address fetches are made for different numbers of NS records and checks that the number are successfully limited. --- bin/tests/system/resolver/clean.sh | 4 +- bin/tests/system/resolver/ns4/named.conf.in | 5 ++ bin/tests/system/resolver/ns4/root.db | 4 + bin/tests/system/resolver/ns4/sourcens.db | 89 +++++++++++++++++++++ bin/tests/system/resolver/ns5/named.conf.in | 9 ++- bin/tests/system/resolver/ns6/named.conf.in | 15 ++++ bin/tests/system/resolver/ns6/targetns.db | 23 ++++++ bin/tests/system/resolver/tests.sh | 34 ++++++++ 8 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 bin/tests/system/resolver/ns4/sourcens.db create mode 100644 bin/tests/system/resolver/ns6/targetns.db diff --git a/bin/tests/system/resolver/clean.sh b/bin/tests/system/resolver/clean.sh index 78c95aeb40..9f86cb14fc 100644 --- a/bin/tests/system/resolver/clean.sh +++ b/bin/tests/system/resolver/clean.sh @@ -17,8 +17,7 @@ rm -f */named.memstats rm -f */named.run rm -f */ans.run rm -f */*.jdb -rm -f dig.out dig.out.* -rm -f dig.*.out.* +rm -f dig.out dig.out.* dig.*.out.* rm -f dig.*.foo.* rm -f dig.*.bar.* rm -f dig.*.prime.* @@ -28,6 +27,7 @@ rm -f ns6/example.net.db.signed ns6/example.net.db rm -f ns6/ds.example.net.db.signed ns6/ds.example.net.db rm -f ns6/dsset-ds.example.net* rm -f ns6/dsset-example.net* ns6/example.net.db.signed.jnl +rm -f ns6/named.stats* rm -f ns6/to-be-removed.tld.db ns6/to-be-removed.tld.db.jnl rm -f ns7/server.db ns7/server.db.jnl rm -f resolve.out.*.test* diff --git a/bin/tests/system/resolver/ns4/named.conf.in b/bin/tests/system/resolver/ns4/named.conf.in index 2a6d90960f..7095346ed2 100644 --- a/bin/tests/system/resolver/ns4/named.conf.in +++ b/bin/tests/system/resolver/ns4/named.conf.in @@ -50,6 +50,11 @@ zone "broken" { file "broken.db"; }; +zone "sourcens" { + type master; + file "sourcens.db"; +}; + key rndc_key { secret "1234abcd8765"; algorithm hmac-sha256; diff --git a/bin/tests/system/resolver/ns4/root.db b/bin/tests/system/resolver/ns4/root.db index 43e4bea4a6..f289e73ddb 100644 --- a/bin/tests/system/resolver/ns4/root.db +++ b/bin/tests/system/resolver/ns4/root.db @@ -26,3 +26,7 @@ no-questions. NS ns.no-questions. ns.no-questions. A 10.53.0.8 formerr-to-all. NS ns.formerr-to-all. ns.formerr-to-all. A 10.53.0.8 +sourcens. NS ns.sourcens. +ns.sourcens. A 10.53.0.4 +targetns. NS ns.targetns. +ns.targetns. A 10.53.0.6 diff --git a/bin/tests/system/resolver/ns4/sourcens.db b/bin/tests/system/resolver/ns4/sourcens.db new file mode 100644 index 0000000000..b02cc6e835 --- /dev/null +++ b/bin/tests/system/resolver/ns4/sourcens.db @@ -0,0 +1,89 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; 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 http://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +; This zone contains a set of delegations with varying numbers of NS +; records. This is used to check that BIND is limiting the number of +; NS records it follows when resolving a delegation. It tests all +; numbers of NS records up to twice the number followed. + +$TTL 60 +@ IN SOA marka.isc.org. ns.server. ( + 2010 ; serial + 600 ; refresh + 600 ; retry + 1200 ; expire + 600 ; minimum + ) +@ NS ns +ns A 10.53.0.4 + +target1 NS ns.fake11.targetns. + +target2 NS ns.fake21.targetns. + NS ns.fake22.targetns. + +target3 NS ns.fake31.targetns. + NS ns.fake32.targetns. + NS ns.fake33.targetns. + +target4 NS ns.fake41.targetns. + NS ns.fake42.targetns. + NS ns.fake43.targetns. + NS ns.fake44.targetns. + +target5 NS ns.fake51.targetns. + NS ns.fake52.targetns. + NS ns.fake53.targetns. + NS ns.fake54.targetns. + NS ns.fake55.targetns. + +target6 NS ns.fake61.targetns. + NS ns.fake62.targetns. + NS ns.fake63.targetns. + NS ns.fake64.targetns. + NS ns.fake65.targetns. + NS ns.fake66.targetns. + +target7 NS ns.fake71.targetns. + NS ns.fake72.targetns. + NS ns.fake73.targetns. + NS ns.fake74.targetns. + NS ns.fake75.targetns. + NS ns.fake76.targetns. + NS ns.fake77.targetns. + +target8 NS ns.fake81.targetns. + NS ns.fake82.targetns. + NS ns.fake83.targetns. + NS ns.fake84.targetns. + NS ns.fake85.targetns. + NS ns.fake86.targetns. + NS ns.fake87.targetns. + NS ns.fake88.targetns. + +target9 NS ns.fake91.targetns. + NS ns.fake92.targetns. + NS ns.fake93.targetns. + NS ns.fake94.targetns. + NS ns.fake95.targetns. + NS ns.fake96.targetns. + NS ns.fake97.targetns. + NS ns.fake98.targetns. + NS ns.fake99.targetns. + +target10 NS ns.fake101.targetns. + NS ns.fake102.targetns. + NS ns.fake103.targetns. + NS ns.fake104.targetns. + NS ns.fake105.targetns. + NS ns.fake106.targetns. + NS ns.fake107.targetns. + NS ns.fake108.targetns. + NS ns.fake109.targetns. + NS ns.fake1010.targetns. diff --git a/bin/tests/system/resolver/ns5/named.conf.in b/bin/tests/system/resolver/ns5/named.conf.in index c81a3ba5de..b8fdd5e508 100644 --- a/bin/tests/system/resolver/ns5/named.conf.in +++ b/bin/tests/system/resolver/ns5/named.conf.in @@ -48,4 +48,11 @@ zone "delegation-only" { type delegation-only; }; -include "trusted.conf"; +key rndc_key { + secret "1234abcd8765"; + algorithm hmac-sha256; +}; + +controls { + inet 10.53.0.5 port @CONTROLPORT@ allow { any; } keys { rndc_key; }; +}; diff --git a/bin/tests/system/resolver/ns6/named.conf.in b/bin/tests/system/resolver/ns6/named.conf.in index 6661620e13..3046e62965 100644 --- a/bin/tests/system/resolver/ns6/named.conf.in +++ b/bin/tests/system/resolver/ns6/named.conf.in @@ -22,6 +22,7 @@ options { recursion no; dnssec-validation no; querylog yes; + statistics-file "named.stats"; /* * test that named loads with root-delegation-only that * has a exclude list. @@ -72,3 +73,17 @@ zone "fetch.tld" { type master; file "fetch.tld.db"; }; + +zone "targetns" { + type master; + file "targetns.db"; +}; + +key rndc_key { + secret "1234abcd8765"; + algorithm hmac-sha256; +}; + +controls { + inet 10.53.0.6 port @CONTROLPORT@ allow { any; } keys { rndc_key; }; +}; diff --git a/bin/tests/system/resolver/ns6/targetns.db b/bin/tests/system/resolver/ns6/targetns.db new file mode 100644 index 0000000000..036e64580b --- /dev/null +++ b/bin/tests/system/resolver/ns6/targetns.db @@ -0,0 +1,23 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; 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 http://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +; In the test for checking how many NS records BIND will follow, this +; zone marks the server as the one to which the NS lookups will be +; directed. + +$TTL 300 +@ IN SOA marka.isc.org. ns.server. ( + 2010 ; serial + 600 ; refresh + 600 ; retry + 1200 ; expire + 600 ; minimum + ) + NS ns +ns A 10.53.0.6 diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 54eee75dea..d34382572e 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -256,6 +256,40 @@ grep "foo.glue-in-answer.example.org.*192.0.2.1" dig.ns1.out.${n} > /dev/null || if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` +echo_i "check that the resolver limits the number of NS records it follows in a referral response ($n)" +# ns5 is the recusor being tested. ns4 holds the sourcens zone containing names with varying numbers of NS +# records pointing to non-existent nameservers in the targetns zone on ns6. +ret=0 +$RNDCCMD 10.53.0.5 flush || ret=1 # Ensure cache is empty before doing this test +for nscount in 1 2 3 4 5 6 7 8 9 10 +do + # Verify number of NS records at source server + $DIG $DIGOPTS +norecurse @10.53.0.4 target${nscount}.sourcens ns > dig.ns4.out.${nscount}.${n} + sourcerecs=`grep NS dig.ns4.out.${nscount}.${n} | grep -v ';' | wc -l` + test $sourcerecs -eq $nscount || ret=1 + test $sourcerecs -eq $nscount || echo_i "NS count incorrect for target${nscount}.sourcens" + # Expected queries = 2 * number of NS records, up to a maximum of 10. + expected=`expr 2 \* $nscount` + if [ $expected -gt 10 ]; then expected=10; fi + # Work out the queries made by checking statistics on the target before and after the test + $RNDCCMD 10.53.0.6 stats || ret=1 + initial_count=`awk '/responses sent/ {print $1}' ns6/named.stats` + mv ns6/named.stats ns6/named.stats.initial.${nscount}.${n} + $DIG $DIGOPTS @10.53.0.5 target${nscount}.sourcens A > dig.ns5.out.${nscount}.${n} || ret=1 + $RNDCCMD 10.53.0.6 stats || ret=1 + final_count=`awk '/responses sent/ {print $1}' ns6/named.stats` + mv ns6/named.stats ns6/named.stats.final.${nscount}.${n} + # Check number of queries during the test is as expected + actual=`expr $final_count - $initial_count` + if [ $actual -ne $expected ]; then + echo_i "query count error: $nscount NS records: expected queries $expected, actual $actual" + ret=1 + fi +done +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` + n=`expr $n + 1` echo_i "RT21594 regression test check setup ($n)" ret=0 From 034b7e34525ecd9d2b4e5a9d38c2ebbabd471204 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 30 Mar 2020 14:28:58 +1100 Subject: [PATCH 3/6] Count queries to the root and TLD servers as well --- lib/dns/resolver.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 4fdcbd821d..07276adbdb 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -4164,16 +4164,14 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) { return; } - if (dns_name_countlabels(&fctx->domain) > 2) { - result = isc_counter_increment(fctx->qc); - if (result != ISC_R_SUCCESS) { - isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, - DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), - "exceeded max queries resolving '%s'", - fctx->info); - fctx_done(fctx, DNS_R_SERVFAIL, __LINE__); - return; - } + result = isc_counter_increment(fctx->qc); + if (result != ISC_R_SUCCESS) { + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), + "exceeded max queries resolving '%s'", + fctx->info); + fctx_done(fctx, DNS_R_SERVFAIL, __LINE__); + return; } fctx_increference(fctx); From 943b274e2c953a4d0bf59ecc8432f2475eab069d Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 31 Mar 2020 13:58:16 +1100 Subject: [PATCH 4/6] Update the ARM to reflect that TLD and root servers are no longer exempt from max-recursion-queries limits. --- doc/arm/Bv9ARM-book.xml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/doc/arm/Bv9ARM-book.xml b/doc/arm/Bv9ARM-book.xml index a856458d7b..6dce2721ec 100644 --- a/doc/arm/Bv9ARM-book.xml +++ b/doc/arm/Bv9ARM-book.xml @@ -9211,10 +9211,7 @@ avoid-v6-udp-ports { 40000; range 50000 60000; }; 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. Queries to - look up top level domains such as "com" and "net" - and the DNS root zone are exempt from this limitation. - The default is 75. + is terminated and returns SERVFAIL. The default is 75. From 50405828436d8a61e253f1c6cd38271c7e097292 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 31 Mar 2020 14:02:22 +1100 Subject: [PATCH 5/6] Add CHANGES entry --- CHANGES | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES b/CHANGES index 7819b610f5..66fcf12ea0 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,10 @@ +5395. [security] Further limit the number of queries that can be + triggered from a request. Root and TLD servers + are no longer exempt from max-recursion-queries. + Fetches for missing name server address records + are limited to 4 for any domain. (CVE-2020-8616) + [GL #1388] + 5390. [security] Replaying a TSIG BADTIME response as a request could trigger an assertion failure. (CVE-2020-8617) [GL #1703] From c0970157328239c515dac200d8c4840c557b8060 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 31 Mar 2020 17:22:15 +1100 Subject: [PATCH 6/6] Add release notes entry --- doc/arm/notes-9.14.12.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/arm/notes-9.14.12.xml b/doc/arm/notes-9.14.12.xml index 47b919e0ef..42761216b8 100644 --- a/doc/arm/notes-9.14.12.xml +++ b/doc/arm/notes-9.14.12.xml @@ -13,6 +13,17 @@
Security Fixes + + + To prevent exhaustion of server resources by a maliciously configured + domain, the number of recursive queries that can be triggered by a + request before aborting recursion has been further limited. Root and + top-level domain servers are no longer exempt from the + max-recursion-queries limit. Fetches for missing + name server address records are limited to 4 for any domain. This + issue was disclosed in CVE-2020-8616. [GL #1388] + + Replaying a TSIG BADTIME response as a request could