diff --git a/CHANGES b/CHANGES index 4b069a046a..5ddf3d2146 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +4727. [bug] Retransferring an inline-signed slave using NSEC3 + around the time its NSEC3 salt was changed could result + in an infinite signing loop. [RT #45080] + 4726. [port] Prevent setsockopt() errors related to TCP_FASTOPEN from being logged on FreeBSD if the kernel does not support it. Notify the user when the kernel does diff --git a/bin/named/server.c b/bin/named/server.c index 4ef0b519ab..162ff90d2d 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -12790,7 +12790,7 @@ typedef struct { } ns_dzctx_t; /* - * Carry out a zone deletion scheduled by named_server_addzone(). + * Carry out a zone deletion scheduled by named_server_delzone(). */ static void rmzone(isc_task_t *task, isc_event_t *event) { diff --git a/bin/tests/system/inline/clean.sh b/bin/tests/system/inline/clean.sh index 575aa827f0..60e76777cd 100644 --- a/bin/tests/system/inline/clean.sh +++ b/bin/tests/system/inline/clean.sh @@ -68,6 +68,10 @@ rm -f ns5/bits.bk rm -f ns5/bits.bk.jnl rm -f ns5/bits.bk.signed rm -f ns5/bits.bk.signed.jnl +rm -f ns7/K* +rm -f ns7/nsec3-loop.db +rm -f ns7/nsec3-loop.db.signed +rm -f ns7/nsec3-loop.db.signed.jnl rm -f */*.jbk rm -f dig.out.ns* rm -f signing.out* diff --git a/bin/tests/system/inline/ns2/named.conf b/bin/tests/system/inline/ns2/named.conf index 77f7f76a7c..752ed9a30e 100644 --- a/bin/tests/system/inline/ns2/named.conf +++ b/bin/tests/system/inline/ns2/named.conf @@ -47,3 +47,9 @@ zone "retransfer3" { allow-update { any; }; notify no; }; + +zone "nsec3-loop" { + type master; + file "nsec3-loop.db"; + notify no; +}; diff --git a/bin/tests/system/inline/ns2/nsec3-loop.db b/bin/tests/system/inline/ns2/nsec3-loop.db new file mode 100644 index 0000000000..4929c21e81 --- /dev/null +++ b/bin/tests/system/inline/ns2/nsec3-loop.db @@ -0,0 +1,20 @@ +; Copyright (C) 2017 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/. + +; NOTE: This zone's data has been crafted in order to reproduce a very specific +; scenario (see ns7/named.conf for more details). Please do not modify this +; file. + +$TTL 300 ; 5 minutes +@ IN SOA ns2 . ( + 1 ; serial + 20 ; refresh (20 seconds) + 20 ; retry (20 seconds) + 1814400 ; expire (3 weeks) + 3600 ; minimum (1 hour) + ) + NS ns2 +ns2 A 10.53.0.2 diff --git a/bin/tests/system/inline/ns7/named.conf b/bin/tests/system/inline/ns7/named.conf new file mode 100644 index 0000000000..c92e9843ff --- /dev/null +++ b/bin/tests/system/inline/ns7/named.conf @@ -0,0 +1,43 @@ +/* + * Copyright (C) 2017 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/. + */ + +/* + * NS7 + * + * NOTE: This named instance is used to reproduce a scenario which involves a + * number of functions getting called in a very specific order which results in + * an infinite loop while iterating over NSEC3 red-black tree. Ensuring this + * happens requires carefully setting the number of signing keys, NSEC3 + * parameters (number of iterations and salt value), zone data and named + * configuration. Changing any of these and/or influencing this instance's + * behavior (e.g. by sending extra queries to it) might render this test moot + * as it will no longer be able to reproduce the exact scenario it attempts to. + * + * Given the above, please do not use this instance for any other test than the + * one it was meant for. + */ + +include "../../common/rndc.key"; + +controls { inet 10.53.0.7 port 9953 allow { any; } keys { rndc_key; }; }; + +options { + query-source address 10.53.0.7; + notify-source 10.53.0.7; + transfer-source 10.53.0.7; + port 5300; + pid-file "named.pid"; + listen-on { 10.53.0.7; }; + listen-on-v6 { none; }; + recursion no; + notify no; + try-tcp-refresh no; + allow-new-zones yes; + sig-signing-nodes 100; + sig-signing-signatures 10; +}; diff --git a/bin/tests/system/inline/ns7/sign.sh b/bin/tests/system/inline/ns7/sign.sh new file mode 100755 index 0000000000..07a1f29a77 --- /dev/null +++ b/bin/tests/system/inline/ns7/sign.sh @@ -0,0 +1,20 @@ +#!/bin/sh -e +# +# Copyright (C) 2017 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/. + +SYSTEMTESTTOP=../.. +. $SYSTEMTESTTOP/conf.sh + +# NOTE: The number of signing keys generated below is not coincidental. More +# details can be found in the comment inside ns7/named.conf. + +zone=nsec3-loop +rm -f K${zone}.+*+*.key +rm -f K${zone}.+*+*.private +keyname=`$KEYGEN -q -r $RANDFILE -a NSEC3RSASHA1 -b 1024 -n zone $zone` +keyname=`$KEYGEN -q -r $RANDFILE -a NSEC3RSASHA1 -b 1024 -n zone $zone` +keyname=`$KEYGEN -q -r $RANDFILE -a NSEC3RSASHA1 -b 1024 -n zone -f KSK $zone` diff --git a/bin/tests/system/inline/setup.sh b/bin/tests/system/inline/setup.sh index ec9ff3a8d1..7a0f38624c 100644 --- a/bin/tests/system/inline/setup.sh +++ b/bin/tests/system/inline/setup.sh @@ -35,3 +35,4 @@ cp ns5/named.conf.pre ns5/named.conf (cd ns3; $SHELL -e sign.sh) (cd ns1; $SHELL -e sign.sh) +(cd ns7; $SHELL -e sign.sh) diff --git a/bin/tests/system/inline/tests.sh b/bin/tests/system/inline/tests.sh index ddbc75ff80..b14ea9d10c 100755 --- a/bin/tests/system/inline/tests.sh +++ b/bin/tests/system/inline/tests.sh @@ -41,7 +41,7 @@ do done n=`expr $n + 1` -echo "I:checking that rrsigs are replaced with ksk only" +echo "I:checking that rrsigs are replaced with ksk only ($n)" ret=0 $DIG @10.53.0.3 -p 5300 axfr nsec3. | awk '/RRSIG NSEC3/ {a[$1]++} END { for (i in a) {if (a[i] != 1) exit (1)}}' || ret=1 @@ -198,8 +198,8 @@ do done if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` -n=`expr $n + 1` +n=`expr $n + 1` echo "I:checking that the zone is signed on initial transfer, noixfr ($n)" ret=0 for i in 1 2 3 4 5 6 7 8 9 10 1 2 3 4 5 6 7 8 9 10 1 2 3 4 5 6 7 8 9 10 @@ -290,6 +290,7 @@ do sleep 1 done if [ $ret != 0 ]; then echo "I:failed"; fi +status=`expr $status + $ret` n=`expr $n + 1` echo "I:checking removal of private type record via 'rndc signing -clear' (master) ($n)" @@ -412,6 +413,7 @@ do sleep 1 done if [ $ret != 0 ]; then echo "I:failed"; fi +status=`expr $status + $ret` n=`expr $n + 1` echo "I:checking master zone that was updated while offline is correct ($n)" @@ -749,8 +751,8 @@ do done if [ $ans != 1 ]; then echo "I:failed"; ret=1; fi status=`expr $status + $ret` -n=`expr $n + 1` +n=`expr $n + 1` echo "I:check rndc retransfer of a inline slave zone works ($n)" ret=0 $RNDC -c ../common/rndc.conf -s 10.53.0.3 -p 9953 retransfer retransfer 2>&1 || ret=1 @@ -764,10 +766,10 @@ do sleep 1 done [ $ans = 1 ] && ret=1 -n=`expr $n + 1` if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` echo "I:check rndc retransfer of a inline nsec3 slave retains nsec3 ($n)" ret=0 for i in 0 1 2 3 4 5 6 7 8 9 @@ -790,7 +792,53 @@ do sleep 1 done [ $ans = 1 ] && ret=1 +if [ $ret != 0 ]; then echo "I:failed"; fi +status=`expr $status + $ret` + +# NOTE: The test below should be considered fragile. More details can be found +# in the comment inside ns7/named.conf. n=`expr $n + 1` +echo "I:check rndc retransfer of a inline nsec3 slave does not trigger an infinite loop ($n)" +ret=0 +zone=nsec3-loop +# Add slave zone using rndc +$RNDC -c ../common/rndc.conf -s 10.53.0.7 -p 9953 addzone $zone \ + '{ type slave; masters { 10.53.0.2; }; file "'$zone'.db"; inline-signing yes; auto-dnssec maintain; };' +# Wait until slave zone is fully signed using NSEC +for i in 1 2 3 4 5 6 7 8 9 0 +do + ret=1 + $RNDC -c ../common/rndc.conf -s 10.53.0.7 -p 9953 signing -list $zone > signing.out.test$n 2>&1 + keys=`grep '^Done signing' signing.out.test$n | wc -l` + [ $keys -eq 3 ] && ret=0 && break + sleep 1 +done +# Switch slave zone to NSEC3 +$RNDC -c ../common/rndc.conf -s 10.53.0.7 -p 9953 signing -nsec3param 1 0 2 12345678 $zone > /dev/null 2>&1 +# Wait until slave zone is fully signed using NSEC3 +for i in 1 2 3 4 5 6 7 8 9 0 +do + ret=1 + nsec3param=`$DIG +short @10.53.0.7 -p 5300 nsec3param $zone` + test "$nsec3param" = "1 0 2 12345678" && ret=0 && break + sleep 1 +done +# Attempt to retransfer the slave zone from master +$RNDC -c ../common/rndc.conf -s 10.53.0.7 -p 9953 retransfer $zone +# Check whether the signer managed to fully sign the retransferred zone by +# waiting for a specific SOA serial number to appear in the logs; if this +# specific SOA serial number does not appear in the logs, it means the signer +# has either ran into an infinite loop or crashed; note that we check the logs +# instead of sending SOA queries to the signer as these may influence its +# behavior in a way which may prevent the desired scenario from being +# reproduced (see comment in ns7/named.conf) +for i in 1 2 3 4 5 6 7 8 9 0 +do + ret=1 + grep "ns2.$zone. . 10 20 20 1814400 3600" ns7/named.run > /dev/null 2>&1 + [ $? -eq 0 ] && ret=0 && break + sleep 1 +done if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` @@ -841,6 +889,8 @@ $RNDC -c ../common/rndc.conf -s 10.53.0.3 -p 9953 addzone test-$zone \ '{ type slave; masters { 10.53.0.2; }; file "'test-$zone.bk'"; inline-signing yes; auto-dnssec maintain; allow-transfer { any; }; };' $RNDC -c ../common/rndc.conf -s 10.53.0.3 -p 9953 delzone test-$zone > /dev/null 2>&1 done +if [ $ret != 0 ]; then echo "I:failed"; fi +status=`expr $status + $ret` n=`expr $n + 1` echo "I:testing adding external keys to a inline zone ($n)" @@ -865,6 +915,7 @@ do test ${dnskeys:-0} -eq 3 || { echo "I: failed $alg (dnskeys ${dnskeys:-0})"; ret=1; } test ${rrsigs:-0} -eq 2 || { echo "I: failed $alg (rrsigs ${rrsigs:-0})"; ret=1; } done +if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` n=`expr $n + 1` @@ -927,8 +978,8 @@ serial=`awk '$4 == "SOA" { print $7 }' dig.out.ns3.post.test$n` [ ${newserial:-0} -eq ${serial:-1} ] || ret=1 if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` -n=`expr $n + 1` +n=`expr $n + 1` echo "I:testing updating dynamic serial via 'rndc signing -serial' ($n)" ret=0 $DIG bits. SOA -p 5300 @10.53.0.2 > dig.out.ns2.pre.test$n diff --git a/lib/dns/include/dns/rbt.h b/lib/dns/include/dns/rbt.h index a4d8ddae50..1490f3bd90 100644 --- a/lib/dns/include/dns/rbt.h +++ b/lib/dns/include/dns/rbt.h @@ -194,6 +194,13 @@ typedef void (*dns_rbtdeleter_t)(void *, void *); * pointers, chains might be going away in a future release, though the * movement functionality would remain. * + * Chains may be used to iterate over a tree of trees. After setting up the + * chain's structure using dns_rbtnodechain_init(), it needs to be initialized + * to point to the lexically first or lexically last node in the tree of trees + * using dns_rbtnodechain_first() or dns_rbtnodechain_last(), respectively. + * Calling dns_rbtnodechain_next() or dns_rbtnodechain_prev() then moves the + * chain over to the next or previous node, respectively. + * * In any event, parent information, whether via parent pointers or chains, is * necessary information for iterating through the tree or for basic internal * tree maintenance issues (ie, the rotations that are done to rebalance the diff --git a/lib/dns/rbt.c b/lib/dns/rbt.c index 83163db3a4..4a02f3924c 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -3495,8 +3495,22 @@ dns_rbtnodechain_next(dns_rbtnodechain_t *chain, dns_name_t *name, * Reached the root without having traversed * any left pointers, so this level is done. */ - if (chain->level_count == 0) + if (chain->level_count == 0) { + /* + * If the tree we are iterating over + * was modified since this chain was + * initialized in a way that caused + * node splits to occur, "current" may + * now be pointing to a root node which + * appears to be at level 0, but still + * has a parent. If that happens, + * abort. Otherwise, we are done + * looking for a successor as we really + * reached the root node on level 0. + */ + INSIST(PARENT(current) == NULL); break; + } current = chain->levels[--chain->level_count]; new_origin = ISC_TRUE; @@ -3517,6 +3531,12 @@ dns_rbtnodechain_next(dns_rbtnodechain_t *chain, dns_name_t *name, } if (successor != NULL) { + /* + * If we determine that the current node is the successor to + * itself, we will run into an infinite loop, so abort instead. + */ + INSIST(chain->end != successor); + chain->end = successor; /* diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 3ec6241d4a..d14f27b658 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -641,6 +641,7 @@ struct dns_rbtdb { unsigned int node_lock_count; rbtdb_nodelock_t * node_locks; dns_rbtnode_t * origin_node; + dns_rbtnode_t * nsec3_origin_node; dns_stats_t * rrsetstats; /* cache DB only */ isc_stats_t * cachestats; /* cache DB only */ /* Locked by lock. */ @@ -829,6 +830,15 @@ typedef struct rbtdb_rdatasetiter { rdatasetheader_t * current; } rbtdb_rdatasetiter_t; +/* + * Note that these iterators, unless created with either DNS_DB_NSEC3ONLY or + * DNS_DB_NONSEC3, will transparently move between the last node of the + * "regular" RBT ("chain" field) and the root node of the NSEC3 RBT + * ("nsec3chain" field) of the database in question, as if the latter was a + * successor to the former in lexical order. The "current" field always holds + * the address of either "chain" or "nsec3chain", depending on which RBT is + * being traversed at given time. + */ static void dbiterator_destroy(dns_dbiterator_t **iteratorp); static isc_result_t dbiterator_first(dns_dbiterator_t *iterator); static isc_result_t dbiterator_last(dns_dbiterator_t *iterator); @@ -2194,8 +2204,12 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * have to be protected, but we must avoid a race condition where multiple * threads are decreasing the reference to zero simultaneously and at least * one of them is going to free the node. + * * This function returns ISC_TRUE if and only if the node reference decreases * to zero. + * + * NOTE: Decrementing the reference count of a node to zero does not mean it + * will be immediately freed. */ static isc_boolean_t decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, @@ -2213,7 +2227,8 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, nodelock = &rbtdb->node_locks[bucket]; #define KEEP_NODE(n, r) \ - ((n)->data != NULL || (n)->down != NULL || (n) == (r)->origin_node) + ((n)->data != NULL || (n)->down != NULL || \ + (n) == (r)->origin_node || (n) == (r)->nsec3_origin_node) /* Handle easy and typical case first. */ if (!node->dirty && KEEP_NODE(node, rbtdb)) { @@ -8486,8 +8501,6 @@ dns_rbtdb_create * change. */ if (!IS_CACHE(rbtdb)) { - dns_rbtnode_t *nsec3node; - rbtdb->origin_node = NULL; result = dns_rbt_addnode(rbtdb->tree, &rbtdb->common.origin, &rbtdb->origin_node); @@ -8516,25 +8529,27 @@ dns_rbtdb_create * return partial matches when there is only a single NSEC3 * record in the tree. */ - nsec3node = NULL; + rbtdb->nsec3_origin_node = NULL; result = dns_rbt_addnode(rbtdb->nsec3, &rbtdb->common.origin, - &nsec3node); + &rbtdb->nsec3_origin_node); if (result != ISC_R_SUCCESS) { INSIST(result != ISC_R_EXISTS); free_rbtdb(rbtdb, ISC_FALSE, NULL); return (result); } - nsec3node->nsec = DNS_RBT_NSEC_NSEC3; + rbtdb->nsec3_origin_node->nsec = DNS_RBT_NSEC_NSEC3; /* * We need to give the nsec3 origin node the right locknum. */ dns_name_init(&name, NULL); - dns_rbt_namefromnode(nsec3node, &name); + dns_rbt_namefromnode(rbtdb->nsec3_origin_node, &name); #ifdef DNS_RBT_USEHASH - nsec3node->locknum = nsec3node->hashval % + rbtdb->nsec3_origin_node->locknum = + rbtdb->nsec3_origin_node->hashval % rbtdb->node_lock_count; #else - nsec3node->locknum = dns_name_hash(&name, ISC_TRUE) % + rbtdb->nsec3_origin_node->locknum = + dns_name_hash(&name, ISC_TRUE) % rbtdb->node_lock_count; #endif } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 514c597472..a56e6b2591 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -7250,8 +7250,22 @@ zone_nsec3chain(dns_zone_t *zone) { } ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read); - dns_db_attach(zone->db, &db); + /* + * This function is called when zone timer fires, after the latter gets + * set by zone_addnsec3chain(). If the action triggering the call to + * zone_addnsec3chain() is closely followed by a zone deletion request, + * it might turn out that the timer thread will not be woken up until + * after the zone is deleted by rmzone(), which calls dns_db_detach() + * for zone->db, causing the latter to become NULL. Return immediately + * if that happens. + */ + if (zone->db != NULL) { + dns_db_attach(zone->db, &db); + } ZONEDB_UNLOCK(&zone->dblock, isc_rwlocktype_read); + if (db == NULL) { + return; + } result = dns_db_newversion(db, &version); if (result != ISC_R_SUCCESS) { @@ -14309,9 +14323,7 @@ save_nsec3param(dns_zone_t *zone, nsec3paramlist_t *nsec3list) { } /* - * Walk the list of the nsec3 chains desired for the zone, converting - * parameters to private type records using dns_nsec3param_toprivate(), - * and insert them into the new zone db. + * Populate new zone db with private type records found by save_nsec3param(). */ static isc_result_t restore_nsec3param(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *version, @@ -14344,20 +14356,11 @@ restore_nsec3param(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *version, rdata.data = nsec3p->data; rdata.type = zone->privatetype; rdata.rdclass = zone->rdclass; - CHECK(update_one_rr(db, version, &diff, DNS_DIFFOP_ADD, - &zone->origin, 0, &rdata)); - } - - result = ISC_R_SUCCESS; - -failure: - for (nsec3p = ISC_LIST_HEAD(*nsec3list); - nsec3p != NULL; - nsec3p = next) - { - next = ISC_LIST_NEXT(nsec3p, link); - ISC_LIST_UNLINK(*nsec3list, nsec3p, link); - isc_mem_put(zone->mctx, nsec3p, sizeof(nsec3param_t)); + result = update_one_rr(db, version, &diff, DNS_DIFFOP_ADD, + &zone->origin, 0, &rdata); + if (result != ISC_R_SUCCESS) { + break; + } } dns_diff_clear(&diff); @@ -14481,8 +14484,12 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { * Call restore_nsec3param() to create private-type records from * the old nsec3 parameters and insert them into db */ - if (!ISC_LIST_EMPTY(nsec3list)) - restore_nsec3param(zone, db, version, &nsec3list); + if (!ISC_LIST_EMPTY(nsec3list)) { + result = restore_nsec3param(zone, db, version, &nsec3list); + if (result != ISC_R_SUCCESS) { + goto failure; + } + } dns_db_closeversion(db, &version, ISC_TRUE);