From 08a9e7add10d53a34ddbbe2e9fe9a9efe3b7494b Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Wed, 2 Jun 2021 10:16:50 +0200 Subject: [PATCH 1/3] Add test for NSEC3PARAM not changed after restart Add a test case where 'named' is restarted and ensure that an already signed zone does not change its NSEC3 parameters. The test case first tests the current zone and saves the used salt value. Then after restart it checks if the salt (and other parameters) are the same as before the restart. This test case changes 'set_nsec3param'. This will now reset the salt value, and when checking for NSEC3PARAM we will store the salt and use it when testing the NXDOMAIN response. This does mean that for every test case we now have to call 'set_nsec3param' explicitly (and can not omit it because it is the same as the previous zone). Finally, slightly changed some echo output to make debugging friendlier. --- bin/tests/system/nsec3/tests.sh | 46 ++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/bin/tests/system/nsec3/tests.sh b/bin/tests/system/nsec3/tests.sh index 2982be9188..62c49e5904 100644 --- a/bin/tests/system/nsec3/tests.sh +++ b/bin/tests/system/nsec3/tests.sh @@ -41,8 +41,8 @@ set_nsec3param() { FLAGS=$1 ITERATIONS=$2 SALTLEN=$3 + # Reset salt. SALT="" - test "$SALTLEN" = "0" && SALT="-" } # The apex NSEC3PARAM record indicates that it is signed. @@ -115,6 +115,10 @@ _check_nsec3_nsec3param() { dig_with_opts +noquestion @$SERVER "${ZONE}" NSEC3PARAM > "dig.out.test$n.nsec3param.$ZONE" || return 1 grep "${ZONE}.*0.*IN.*NSEC3PARAM.*1.*0.*${ITERATIONS}.*${SALT}" "dig.out.test$n.nsec3param.$ZONE" > /dev/null || return 1 + + if [ -z "$SALT" ]; then + SALT=`awk '$4 == "NSEC3PARAM" { print $8 }' dig.out.test$n.nsec3param.$ZONE` + fi return 0 } @@ -128,14 +132,14 @@ _check_nsec3_nxdomain() check_nsec3() { n=$((n+1)) - echo_i "check NSEC3PARAM response for zone ${ZONE} ($n)" + echo_i "check that NSEC3PARAM 1 0 ${ITERATIONS} is published zone ${ZONE} ($n)" ret=0 retry_quiet 10 _check_nsec3_nsec3param || log_error "bad NSEC3PARAM response for ${ZONE}" test "$ret" -eq 0 || echo_i "failed" status=$((status+ret)) n=$((n+1)) - echo_i "check NSEC3 response for zone ${ZONE} ($n)" + echo_i "check NXDOMAIN response has correct NSEC3 1 ${FLAGS} ${ITERATIONS} ${SALT} for zone ${ZONE} ($n)" ret=0 retry_quiet 10 _check_nsec3_nxdomain || log_error "bad NXDOMAIN response for zone ${ZONE}" test "$ret" -eq 0 || echo_i "failed" @@ -169,24 +173,28 @@ dnssec_verify # Zone: nsec3-change.kasp. set_zone_policy "nsec3-change.kasp" "nsec3" +set_nsec3param "0" "5" "8" echo_i "initial check zone ${ZONE}" check_nsec3 dnssec_verify # Zone: nsec3-dynamic-change.kasp. set_zone_policy "nsec3-dynamic-change.kasp" "nsec3" +set_nsec3param "0" "5" "8" echo_i "initial check zone ${ZONE}" check_nsec3 dnssec_verify # Zone: nsec3-to-nsec.kasp. set_zone_policy "nsec3-to-nsec.kasp" "nsec3" +set_nsec3param "0" "5" "8" echo_i "initial check zone ${ZONE}" check_nsec3 dnssec_verify # Zone: nsec3-to-optout.kasp. set_zone_policy "nsec3-to-optout.kasp" "nsec3" +set_nsec3param "0" "5" "8" echo_i "initial check zone ${ZONE}" check_nsec3 dnssec_verify @@ -205,7 +213,6 @@ echo_i "initial check zone ${ZONE}" check_nsec3 dnssec_verify - # Reconfig named. echo_i "reconfig dnssec-policy to trigger nsec3 rollovers" copy_setports ns3/named2.conf.in ns3/named.conf @@ -221,12 +228,14 @@ dnssec_verify # Zone: nsec3.kasp. (same) set_zone_policy "nsec3.kasp" "nsec3" +set_nsec3param "0" "5" "8" echo_i "check zone ${ZONE} after reconfig" check_nsec3 dnssec_verify # Zone: nsec3-dyamic.kasp. (same) set_zone_policy "nsec3-dynamic.kasp" "nsec3" +set_nsec3param "0" "5" "8" echo_i "check zone ${ZONE} after reconfig" check_nsec3 dnssec_verify @@ -247,6 +256,7 @@ dnssec_verify # Zone: nsec3-to-nsec.kasp. (reconfigured) set_zone_policy "nsec3-to-nsec.kasp" "nsec" +set_nsec3param "1" "11" "0" echo_i "check zone ${ZONE} after reconfig" check_nsec dnssec_verify @@ -286,5 +296,33 @@ grep "zone uses dnssec-policy, use rndc dnssec command instead" rndc.signing.tes check_nsec3 dnssec_verify +# Test NSEC3 and NSEC3PARAM is the same after restart +set_zone_policy "nsec3.kasp" "nsec3" +set_nsec3param "0" "5" "8" +echo_i "check zone ${ZONE} before restart" +check_nsec3 +dnssec_verify + +# Restart named, NSEC3 should stay the same. +ret=0 +echo "stop ns3" +$PERL ../stop.pl --use-rndc --port ${CONTROLPORT} nsec3 ${DIR} || ret=1 +test "$ret" -eq 0 || echo_i "failed" +status=$((status+ret)) + +ret=0 +echo "start ns3" +start_server --noclean --restart --port ${PORT} nsec3 ${DIR} +test "$ret" -eq 0 || echo_i "failed" +status=$((status+ret)) + +prevsalt="${SALT}" +set_zone_policy "nsec3.kasp" "nsec3" +set_nsec3param "0" "5" "8" +SALT="${prevsalt}" +echo_i "check zone ${ZONE} after restart has salt ${SALT}" +check_nsec3 +dnssec_verify + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 From 0ae3ffdc1c1eb7dfe53ac42fafe4664581be2a05 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Wed, 2 Jun 2021 10:47:57 +0200 Subject: [PATCH 2/3] Fix NSEC3 resalting upon restart When named restarts, it will examine signed zones and checks if the current denial of existence strategy matches the dnssec-policy. If not, it will schedule to create a new NSEC(3) chain. However, on startup the zone database may not be read yet, fooling BIND that the denial of existence chain needs to be created. This results in a replacement of the previous NSEC(3) chain. Change the code such that if the NSEC3PARAM lookup failed (the result did not return in ISC_R_SUCCESS or ISC_R_NOTFOUND), we will try again later. The nsec3param structure has additional variables to signal if the lookup is postponed. We also need to save the signal if an explicit resalt was requested. In addition to the two added boolean variables, we add a variable to store the NSEC3PARAM rdata. This may have a yet to be determined salt value. We can't create the private data yet because there may be a mismatch in salt length and the NULL salt value. --- lib/dns/zone.c | 146 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 112 insertions(+), 34 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index c4788ea203..74dfa82c3c 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -959,10 +959,13 @@ static const char *dbargv_default[] = { "rbt" }; typedef struct nsec3param nsec3param_t; struct nsec3param { + dns_rdata_nsec3param_t rdata; unsigned char data[DNS_NSEC3PARAM_BUFFERSIZE + 1]; unsigned int length; bool nsec; bool replace; + bool resalt; + bool lookup; ISC_LINK(nsec3param_t) link; }; typedef ISC_LIST(nsec3param_t) nsec3paramlist_t; @@ -21250,6 +21253,25 @@ setnsec3param(isc_task_t *task, isc_event_t *event) { dns_zone_idetach(&zone); } +static void +salt2text(unsigned char *salt, uint8_t saltlen, unsigned char *text, + unsigned int textlen) { + isc_region_t r; + isc_buffer_t buf; + isc_result_t result; + + r.base = salt; + r.length = (unsigned int)saltlen; + + isc_buffer_init(&buf, text, textlen); + result = isc_hex_totext(&r, 2, "", &buf); + if (result == ISC_R_SUCCESS) { + text[saltlen * 2] = 0; + } else { + text[0] = 0; + } +} + /* * Check whether NSEC3 chain addition or removal specified by the private-type * record passed with the event was already queued (or even fully performed). @@ -21301,6 +21323,53 @@ rss_post(dns_zone_t *zone, isc_event_t *event) { CHECK(dns_db_getoriginnode(db, &node)); + /* + * Do we need to look up the NSEC3 parameters? + */ + if (np->lookup) { + dns_rdata_nsec3param_t param; + dns_rdata_t nrdata = DNS_RDATA_INIT; + dns_rdata_t prdata = DNS_RDATA_INIT; + unsigned char nbuf[DNS_NSEC3PARAM_BUFFERSIZE]; + unsigned char saltbuf[255]; + isc_buffer_t b; + + param.salt = NULL; + result = dns__zone_lookup_nsec3param(zone, &np->rdata, ¶m, + saltbuf, np->resalt); + if (result == ISC_R_SUCCESS) { + /* + * Success because the NSEC3PARAM already exists, but + * function returns void, so goto failure to clean up. + */ + goto failure; + } + if (result != DNS_R_NSEC3RESALT && result != ISC_R_NOTFOUND) { + dnssec_log(zone, ISC_LOG_DEBUG(3), + "setnsec3param:lookup nsec3param -> %s", + isc_result_totext(result)); + goto failure; + } + + INSIST(param.salt != NULL); + + /* Update NSEC3 parameters. */ + np->rdata.hash = param.hash; + np->rdata.flags = param.flags; + np->rdata.iterations = param.iterations; + np->rdata.salt_length = param.salt_length; + np->rdata.salt = param.salt; + + isc_buffer_init(&b, nbuf, sizeof(nbuf)); + CHECK(dns_rdata_fromstruct(&nrdata, zone->rdclass, + dns_rdatatype_nsec3param, &np->rdata, + &b)); + dns_nsec3param_toprivate(&nrdata, &prdata, zone->privatetype, + np->data, sizeof(np->data)); + np->length = prdata.length; + np->nsec = false; + } + /* * Does a private-type record already exist for this chain? */ @@ -21443,25 +21512,6 @@ failure: INSIST(newver == NULL); } -static void -salt2text(unsigned char *salt, uint8_t saltlen, unsigned char *text, - unsigned int textlen) { - isc_region_t r; - isc_buffer_t buf; - isc_result_t result; - - r.base = salt; - r.length = (unsigned int)saltlen; - - isc_buffer_init(&buf, text, textlen); - result = isc_hex_totext(&r, 2, "", &buf); - if (result == ISC_R_SUCCESS) { - text[saltlen * 2] = 0; - } else { - text[0] = 0; - } -} - /* * Check if zone has NSEC3PARAM (and thus a chain) with the right parameters. * @@ -21493,14 +21543,17 @@ dns__zone_lookup_nsec3param(dns_zone_t *zone, dns_rdata_nsec3param_t *lookup, } ZONEDB_UNLOCK(&zone->dblock, isc_rwlocktype_read); if (db == NULL) { + result = ISC_R_FAILURE; goto setparam; } result = dns_db_findnode(db, &zone->origin, false, &node); if (result != ISC_R_SUCCESS) { dns_zone_log(zone, ISC_LOG_ERROR, - "nsec3param lookup failure: %s", + "dns__zone_lookup_nsec3param:" + "dns_db_findnode -> %s", dns_result_totext(result)); + result = ISC_R_FAILURE; goto setparam; } dns_db_currentversion(db, &version); @@ -21512,7 +21565,8 @@ dns__zone_lookup_nsec3param(dns_zone_t *zone, dns_rdata_nsec3param_t *lookup, INSIST(!dns_rdataset_isassociated(&rdataset)); if (result != ISC_R_NOTFOUND) { dns_zone_log(zone, ISC_LOG_ERROR, - "nsec3param lookup failure: %s", + "dns__zone_lookup_nsec3param:" + "dns_db_findrdataset -> %s", dns_result_totext(result)); } goto setparam; @@ -21552,10 +21606,13 @@ dns__zone_lookup_nsec3param(dns_zone_t *zone, dns_rdata_nsec3param_t *lookup, break; } + if (result == ISC_R_NOMORE) { + result = ISC_R_NOTFOUND; + } + setparam: if (result != ISC_R_SUCCESS) { /* Found no match. */ - result = ISC_R_NOTFOUND; param->hash = lookup->hash; param->flags = lookup->flags; param->iterations = lookup->iterations; @@ -21563,6 +21620,10 @@ setparam: param->salt = lookup->salt; } + if (result != ISC_R_NOTFOUND && result != ISC_R_SUCCESS) { + goto failure; + } + if (param->salt_length == 0) { DE_CONST("-", param->salt); } else if (resalt || param->salt == NULL) { @@ -21595,6 +21656,7 @@ setparam: INSIST(result != ISC_R_SUCCESS); } +failure: if (dns_rdataset_isassociated(&rdataset)) { dns_rdataset_disassociate(&rdataset); } @@ -21646,6 +21708,7 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, dns_zone_t *dummy = NULL; isc_buffer_t b; isc_event_t *e = NULL; + bool do_lookup = false; REQUIRE(DNS_ZONE_VALID(zone)); @@ -21668,7 +21731,11 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, UNLOCK_ZONE(zone); return (ISC_R_SUCCESS); } - INSIST(param.salt != NULL); + /* + * Schedule lookup if lookup above failed (may happen if zone + * db is NULL for example). + */ + do_lookup = (param.salt == NULL) ? true : false; } e = isc_event_allocate(zone->mctx, zone, DNS_EVENT_SETNSEC3PARAM, @@ -21676,8 +21743,9 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, npe = (struct np3event *)e; np = &npe->params; - np->replace = replace; + np->resalt = resalt; + np->lookup = do_lookup; if (hash == 0) { np->length = 0; np->nsec = true; @@ -21689,22 +21757,32 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, param.mctx = NULL; /* nsec3 specific param set in dns__zone_lookup_nsec3param() */ isc_buffer_init(&b, nbuf, sizeof(nbuf)); - CHECK(dns_rdata_fromstruct(&nrdata, zone->rdclass, - dns_rdatatype_nsec3param, ¶m, - &b)); - dns_nsec3param_toprivate(&nrdata, &prdata, zone->privatetype, - np->data, sizeof(np->data)); - np->length = prdata.length; + + if (param.salt != NULL) { + CHECK(dns_rdata_fromstruct(&nrdata, zone->rdclass, + dns_rdatatype_nsec3param, + ¶m, &b)); + dns_nsec3param_toprivate(&nrdata, &prdata, + zone->privatetype, np->data, + sizeof(np->data)); + np->length = prdata.length; + } + + np->rdata = param; np->nsec = false; if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) { unsigned char salttext[255 * 2 + 1]; - salt2text(param.salt, param.salt_length, salttext, - sizeof(salttext)); + if (param.salt != NULL) { + salt2text(param.salt, param.salt_length, + salttext, sizeof(salttext)); + } dnssec_log(zone, ISC_LOG_DEBUG(3), - "setnsec3param:nsec3 %u %u %u %s", + "setnsec3param:nsec3 %u %u %u %u:%s", param.hash, param.flags, param.iterations, - salttext); + param.salt_length, + param.salt == NULL ? "unknown" + : (char *)salttext); } } From d51aed71124716abdaec501e188091ed2f3ecdf1 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Wed, 2 Jun 2021 11:16:10 +0200 Subject: [PATCH 3/3] Add release note and change entry for [#2725] --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 3 +++ 2 files changed, 7 insertions(+) diff --git a/CHANGES b/CHANGES index d4f2df86eb..28b3edd3af 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5653. [bug] Fixed a bug that caused the NSEC3 salt to be changed + for KASP zones on restart. + [GL #2725] + 5652. [bug] Copy and paste error caused the socket option to be enabled instead of disabled. [GL #2746] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 330a86c8be..92f0a693ed 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -76,3 +76,6 @@ Bug Fixes - Fix an error that would enable don't fragment socket option instead of disabling it leading to errors when sending the oversized UDP packets. [GL #2746] + +- Fixed a bug that caused the NSEC salt to be changed for KASP zones on + every startup. :gl:`#2725`