From: Matthijs Mekking Date: Thu, 5 Nov 2020 10:12:24 +0000 (+0100) Subject: Detect NSEC3 salt collisions X-Git-Tag: v9.17.8~27^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6b5d7357dfb9d695f02dabb510dbe2ea404ba241;p=thirdparty%2Fbind9.git Detect NSEC3 salt collisions When generating a new salt, compare it with the previous NSEC3 paremeters to ensure the new parameters are different from the previous ones. This moves the salt generation call from 'bin/named/*.s' to 'lib/dns/zone.c'. When setting new NSEC3 parameters, you can set a new function parameter 'resalt' to enforce a new salt to be generated. A new salt will also be generated if 'salt' is set to NULL. Logging salt with zone context can now be done with 'dnssec_log', removing the need for 'dns_nsec3_log_salt'. --- diff --git a/CHANGES b/CHANGES index 18101e6c95d..dbb7f5bcb4d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ 5538. [func] Add NSEC3 support for zones that manage DNSSEC with the 'dnssec-policy' configuration. A new option 'nsec3param' can be used to set the NSEC3 parameters. + This now also detects salt collisions, and logs salt + generation messages with zone context. [GL #1620] 5537. [func] The query plugin mechanism has been extended diff --git a/bin/named/server.c b/bin/named/server.c index f4ed7fb8787..9761a07ffaa 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -14369,7 +14369,7 @@ named_server_signing(named_server_t *server, isc_lex_t *lex, bool list = false, clear = false; bool chain = false; bool setserial = false; - bool log_salt = false; + bool resalt = false; uint32_t serial = 0; char keystr[DNS_SECALG_FORMATSIZE + 7]; /* <5-digit keyid>/ */ unsigned short hash = 0, flags = 0, iter = 0, saltlen = 0; @@ -14452,8 +14452,7 @@ named_server_signing(named_server_t *server, isc_lex_t *lex, * configurable. */ saltlen = 8; - CHECK(dns_nsec3_generate_salt(salt, saltlen)); - log_salt = true; + resalt = true; } else if (strcmp(ptr, "-") != 0) { isc_buffer_t buf; @@ -14491,19 +14490,9 @@ named_server_signing(named_server_t *server, isc_lex_t *lex, (void)putstr(text, "request queued"); (void)putnull(text); } else if (chain) { - if (log_salt) { - char zonetext[DNS_NAME_MAXTEXT + 32]; - dns_zone_name(zone, zonetext, sizeof(zonetext)); - dns_nsec3_log_salt( - named_g_lctx, NAMED_LOGCATEGORY_GENERAL, - NAMED_LOGMODULE_SERVER, ISC_LOG_INFO, salt, - saltlen, - "generated salt for zone %s:", zonetext); - } - - CHECK(dns_zone_setnsec3param(zone, (uint8_t)hash, - (uint8_t)flags, iter, - (uint8_t)saltlen, salt, true)); + CHECK(dns_zone_setnsec3param( + zone, (uint8_t)hash, (uint8_t)flags, iter, + (uint8_t)saltlen, salt, true, resalt)); (void)putstr(text, "nsec3param request queued"); (void)putnull(text); } else if (setserial) { diff --git a/bin/named/zoneconf.c b/bin/named/zoneconf.c index cca3feb3811..91023674705 100644 --- a/bin/named/zoneconf.c +++ b/bin/named/zoneconf.c @@ -1562,50 +1562,15 @@ named_zone_configure(const cfg_obj_t *config, const cfg_obj_t *vconfig, bool sigvalinsecs; if (kasp != NULL) { - unsigned char saltbuf[255]; - unsigned char *salt; - DE_CONST("-", salt); - if (dns_kasp_nsec3(kasp)) { - result = dns_zone_checknsec3param( + result = dns_zone_setnsec3param( zone, 1, dns_kasp_nsec3flags(kasp), dns_kasp_nsec3iter(kasp), - dns_kasp_nsec3saltlen(kasp), NULL); - if (result != ISC_R_SUCCESS) { - if (dns_kasp_nsec3saltlen(kasp) > 0) { - char zonetext[DNS_NAME_MAXTEXT + - 32]; - dns_zone_name(zone, zonetext, - sizeof(zonetext)); - - RETERR(dns_nsec3_generate_salt( - saltbuf, - dns_kasp_nsec3saltlen( - kasp))); - salt = saltbuf; - - dns_nsec3_log_salt( - named_g_lctx, - NAMED_LOGCATEGORY_GENERAL, - NAMED_LOGMODULE_SERVER, - ISC_LOG_INFO, salt, - dns_kasp_nsec3saltlen( - kasp), - "generated salt for " - "zone %s:", - zonetext); - } - result = dns_zone_setnsec3param( - zone, 1, - dns_kasp_nsec3flags(kasp), - dns_kasp_nsec3iter(kasp), - dns_kasp_nsec3saltlen(kasp), - salt, true); - } - + dns_kasp_nsec3saltlen(kasp), NULL, true, + false); } else { - result = dns_zone_setnsec3param(zone, 0, 0, 0, - 0, salt, true); + result = dns_zone_setnsec3param( + zone, 0, 0, 0, 0, NULL, true, false); } INSIST(result == ISC_R_SUCCESS); } diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 4820e8cfb1f..0823a0c490e 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -62,7 +62,7 @@ Feature Changes - Add NSEC3 support for zones that manage their DNSSEC with the `dnssec-policy` configuration. A new option 'nsec3param' can be used to set the desired - NSEC3 parameters. [GL #1620]. + NSEC3 parameters, and will detect collisions when resalting. [GL #1620]. Bug Fixes ~~~~~~~~~ diff --git a/lib/dns/include/dns/nsec3.h b/lib/dns/include/dns/nsec3.h index f001fc0acb0..1c12a464e96 100644 --- a/lib/dns/include/dns/nsec3.h +++ b/lib/dns/include/dns/nsec3.h @@ -79,14 +79,6 @@ dns_nsec3_generate_salt(unsigned char *salt, size_t saltlen); * Generate a salt with the given salt length. */ -void -dns_nsec3_log_salt(isc_log_t *lctx, isc_logcategory_t *category, - isc_logmodule_t *module, int level, unsigned char *salt, - size_t saltlen, const char *fmt, ...); -/*%< - * Utility to log the salt. - */ - isc_result_t dns_nsec3_hashname(dns_fixedname_t *result, unsigned char rethash[NSEC3_MAX_HASH_LENGTH], diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index 75882ba395a..af7208f56bb 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -2378,33 +2378,17 @@ dns_zone_getraw(dns_zone_t *zone, dns_zone_t **raw); isc_result_t dns_zone_keydone(dns_zone_t *zone, const char *data); -isc_result_t -dns_zone_checknsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, - uint16_t iter, uint8_t saltlen, unsigned char *salt); -/*% - * Check if the NSEC3 parameters for the zone match the requested parameters. - * - * If 'salt' is NULL, a match is found if the salt has the requested length, - * otherwise the NSEC3 salt must match the requested salt value too. - * - * Requires: - * \li 'zone' to be valid. - * - * Returns: - * \li ISC_R_SUCCESS, if a match is found. - * \li Error, if no match is found, or if the db lookup failed. - */ - isc_result_t dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, uint16_t iter, uint8_t saltlen, unsigned char *salt, - bool replace); + bool replace, bool resalt); /*% * Set the NSEC3 parameters for the zone. * * If 'replace' is true, then the existing NSEC3 chain, if any, will * be replaced with the new one. If 'hash' is zero, then the replacement - * chain will be NSEC rather than NSEC3. + * chain will be NSEC rather than NSEC3. If 'resalt' is true, or if 'salt' + * is NULL, generate a new salt with the given salt length. * * Requires: * \li 'zone' to be valid. diff --git a/lib/dns/nsec3.c b/lib/dns/nsec3.c index 3dbe33e8d6c..a6401e92f51 100644 --- a/lib/dns/nsec3.c +++ b/lib/dns/nsec3.c @@ -235,41 +235,6 @@ dns_nsec3_generate_salt(unsigned char *salt, size_t saltlen) { return (ISC_R_SUCCESS); } -void -dns_nsec3_log_salt(isc_log_t *lctx, isc_logcategory_t *category, - isc_logmodule_t *module, int level, unsigned char *salt, - size_t saltlen, const char *fmt, ...) { - va_list ap; - - char message[4096]; - unsigned char text[255 * 2 + 1]; - isc_region_t r; - isc_buffer_t buf; - isc_result_t result; - - if (!isc_log_wouldlog(dns_lctx, level)) { - return; - } - - va_start(ap, fmt); - - vsnprintf(message, sizeof(message), fmt, ap); - - r.base = salt; - r.length = (unsigned int)saltlen; - - isc_buffer_init(&buf, text, sizeof(text)); - result = isc_hex_totext(&r, 2, "", &buf); - if (result == ISC_R_SUCCESS) { - text[saltlen * 2] = 0; - } else { - text[0] = 0; - } - isc_log_write(lctx, category, module, level, "%s %s", message, text); - - va_end(ap); -} - isc_result_t dns_nsec3_hashname(dns_fixedname_t *result, unsigned char rethash[NSEC3_MAX_HASH_LENGTH], diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index f9e124f3aad..4f88cbb98f1 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -654,7 +654,6 @@ dns_nsec3_delnsec3sx dns_nsec3_generate_salt dns_nsec3_hashlength dns_nsec3_hashname -dns_nsec3_log_salt dns_nsec3_maxiterations dns_nsec3_noexistnodata dns_nsec3_supportedhash @@ -1165,7 +1164,6 @@ dns_zone_catz_enable dns_zone_catz_enable_db dns_zone_cdscheck dns_zone_checknames -dns_zone_checknsec3param dns_zone_clearforwardacl dns_zone_clearnotifyacl dns_zone_clearqueryacl diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 9fd1b3df406..629992cc380 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -21054,10 +21054,16 @@ failure: /* * Check if zone has NSEC3PARAM (and thus a chain) with the right parameters. + * + * If 'salt' is NULL, a match is found if the salt has the requested length, + * otherwise the NSEC3 salt must match the requested salt value too. + * + * Returns ISC_R_SUCCESS, if a match is found, or an error if no match is + * found, or if the db lookup failed. */ -isc_result_t -dns_zone_checknsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, - uint16_t iter, uint8_t saltlen, unsigned char *salt) { +static isc_result_t +zone_has_nsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, + uint16_t iter, uint8_t saltlen, unsigned char *salt) { isc_result_t result = ISC_R_UNEXPECTED; dns_dbnode_t *node = NULL; dns_db_t *db = NULL; @@ -21146,8 +21152,28 @@ cleanup: return (result); } +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; + } +} + /* - * Called when an "rndc signing -nsec3param ..." command is received. + * Called when an "rndc signing -nsec3param ..." command is received, or the + * 'dnssec-policy' has changed. * * Allocate and prepare an nsec3param_t structure which holds information about * the NSEC3 changes requested for the zone: @@ -21168,7 +21194,7 @@ cleanup: isc_result_t dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, uint16_t iter, uint8_t saltlen, unsigned char *salt, - bool replace) { + bool replace, bool resalt) { isc_result_t result = ISC_R_SUCCESS; dns_rdata_nsec3param_t param; dns_rdata_t nrdata = DNS_RDATA_INIT; @@ -21178,13 +21204,51 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, nsec3param_t *np; dns_zone_t *dummy = NULL; isc_buffer_t b; - isc_event_t *e; + isc_event_t *e = NULL; + unsigned char saltbuf[255]; + unsigned char salttext[255 * 2 + 1]; REQUIRE(DNS_ZONE_VALID(zone)); - REQUIRE(salt != NULL); LOCK_ZONE(zone); + result = zone_has_nsec3param(zone, hash, flags, iter, saltlen, salt); + if (result == ISC_R_SUCCESS) { + /* + * The right NSEC3 parameters are already set, no need to + * set again, unless resalting is enforced. + */ + if (!resalt) { + result = ISC_R_EXISTS; + goto failure; + } + } + + if (saltlen == 0) { + DE_CONST("-", salt); + salttext[0] = '-'; + salttext[1] = 0; + } else if (resalt || salt == NULL) { + do { + /* Generate a new salt. */ + CHECK(dns_nsec3_generate_salt(saltbuf, saltlen)); + if (result != ISC_R_SUCCESS) { + goto failure; + } + salt = saltbuf; + salt2text(salt, saltlen, salttext, sizeof(salttext)); + dnssec_log(zone, ISC_LOG_INFO, "generated salt: %s", + salttext); + /* + * Check for NSEC3 param conflicts, this is done to + * avoid salt collision. + */ + result = zone_has_nsec3param(zone, hash, flags, iter, + saltlen, salt); + } while (result == ISC_R_SUCCESS); + } + INSIST(salt != NULL); + e = isc_event_allocate(zone->mctx, zone, DNS_EVENT_SETNSEC3PARAM, setnsec3param, zone, sizeof(struct np3event)); @@ -21216,28 +21280,10 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, np->nsec = false; if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) { - unsigned char text[255 * 2 + 1]; - isc_buffer_t buf; - isc_result_t ret; - isc_region_t r; - - r.base = salt; - r.length = (unsigned int)saltlen; - if (saltlen > 0) { - isc_buffer_init(&buf, text, sizeof(text)); - ret = isc_hex_totext(&r, 2, "", &buf); - if (ret == ISC_R_SUCCESS) { - text[saltlen * 2] = 0; - } else { - text[0] = 0; - } - } else { - text[0] = '-'; - text[1] = 0; - } + salt2text(salt, saltlen, salttext, sizeof(salttext)); dnssec_log(zone, ISC_LOG_DEBUG(3), "setnsec3param:nsec3 %u %u %u %s", hash, - flags, iter, text); + flags, iter, salttext); } } @@ -21248,6 +21294,7 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, * receive_secure_db() if it ever gets called or simply freed by * zone_free() otherwise. */ + ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read); if (zone->db != NULL) { zone_iattach(zone, &dummy); @@ -21258,6 +21305,8 @@ dns_zone_setnsec3param(dns_zone_t *zone, uint8_t hash, uint8_t flags, } ZONEDB_UNLOCK(&zone->dblock, isc_rwlocktype_read); + result = ISC_R_SUCCESS; + failure: if (e != NULL) { isc_event_free(&e);