From 966c06a9e69e21f4de2755f30d346dcfe6c735ad Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Thu, 18 Jun 2020 14:35:09 -0300 Subject: [PATCH 1/7] Minor refactoring/cleanup This commit moves the warning message to the logical block where it belongs better. For more details check thread comment: https://gitlab.isc.org/isc-projects/bind9/merge_requests/291#note_12167 --- lib/dns/zoneverify.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index fcd8b1ddb4..c54be29074 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -310,18 +310,18 @@ check_no_rrsig(const vctx_t *vctx, const dns_rdataset_t *rdataset, if (sigrdataset.type == dns_rdatatype_rrsig && sigrdataset.covers == rdataset->type) { + dns_name_format(name, namebuf, sizeof(namebuf)); + dns_rdatatype_format(rdataset->type, typebuf, + sizeof(typebuf)); + zoneverify_log_error( + vctx, + "Warning: Found unexpected signatures " + "for %s/%s", + namebuf, typebuf); break; } dns_rdataset_disassociate(&sigrdataset); } - if (result == ISC_R_SUCCESS) { - dns_name_format(name, namebuf, sizeof(namebuf)); - dns_rdatatype_format(rdataset->type, typebuf, sizeof(typebuf)); - zoneverify_log_error(vctx, - "Warning: Found unexpected signatures " - "for %s/%s", - namebuf, typebuf); - } if (dns_rdataset_isassociated(&sigrdataset)) { dns_rdataset_disassociate(&sigrdataset); } From 98d1e40143bf5c9dde5497ed9bfb465be511dcc8 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Thu, 18 Jun 2020 16:49:40 -0300 Subject: [PATCH 2/7] Minor refactoring on function match_nsec3 The logic for matching a set of nsec3 objects against an nsec3param object was moved to a specific function. For more details check thread: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12176 --- lib/dns/zoneverify.c | 55 ++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index c54be29074..5b1e185090 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -424,6 +424,40 @@ record_nsec3(const vctx_t *vctx, const unsigned char *rawhash, return (result); } +/* + * Check whether any NSEC3 within 'rdataset' matches the parameters in + * 'nsec3param'. + */ +static isc_result_t +find_nsec3_match(const dns_rdata_nsec3param_t *nsec3param, + dns_rdataset_t *rdataset, size_t rhsize, + dns_rdata_nsec3_t *nsec3_match) { + isc_result_t result; + + /* + * Find matching NSEC3 record. + */ + for (result = dns_rdataset_first(rdataset); result == ISC_R_SUCCESS; + result = dns_rdataset_next(rdataset)) + { + dns_rdata_t rdata = DNS_RDATA_INIT; + dns_rdataset_current(rdataset, &rdata); + result = dns_rdata_tostruct(&rdata, nsec3_match, NULL); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + if (nsec3_match->hash == nsec3param->hash && + nsec3_match->next_length == rhsize && + nsec3_match->iterations == nsec3param->iterations && + nsec3_match->salt_length == nsec3param->salt_length && + memcmp(nsec3_match->salt, nsec3param->salt, + nsec3param->salt_length) == 0) + { + return (ISC_R_SUCCESS); + } + } + + return (result); +} + static isc_result_t match_nsec3(const vctx_t *vctx, const dns_name_t *name, const dns_rdata_nsec3param_t *nsec3param, dns_rdataset_t *rdataset, @@ -436,26 +470,7 @@ match_nsec3(const vctx_t *vctx, const dns_name_t *name, isc_result_t result; unsigned int len; - /* - * Find matching NSEC3 record. - */ - for (result = dns_rdataset_first(rdataset); result == ISC_R_SUCCESS; - result = dns_rdataset_next(rdataset)) - { - dns_rdata_t rdata = DNS_RDATA_INIT; - dns_rdataset_current(rdataset, &rdata); - result = dns_rdata_tostruct(&rdata, &nsec3, NULL); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - if (nsec3.hash == nsec3param->hash && - nsec3.next_length == rhsize && - nsec3.iterations == nsec3param->iterations && - nsec3.salt_length == nsec3param->salt_length && - memcmp(nsec3.salt, nsec3param->salt, - nsec3param->salt_length) == 0) - { - break; - } - } + result = find_nsec3_match(nsec3param, rdataset, rhsize, &nsec3); if (result != ISC_R_SUCCESS) { dns_name_format(name, namebuf, sizeof(namebuf)); zoneverify_log_error(vctx, "Missing NSEC3 record for %s", From 87e970474f1a9a3eefbb7baf7f4d25022c1e6dce Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Thu, 18 Jun 2020 17:23:27 -0300 Subject: [PATCH 3/7] Initialize buffers with { 0 } instead of memset More details on threads: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12178 https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12181 --- lib/dns/zoneverify.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 5b1e185090..22427764fb 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -821,7 +821,7 @@ verifynsec3s(const vctx_t *vctx, const dns_name_t *name, static isc_result_t verifyset(vctx_t *vctx, dns_rdataset_t *rdataset, const dns_name_t *name, dns_dbnode_t *node, dst_key_t **dstkeys, size_t nkeys) { - unsigned char set_algorithms[256]; + unsigned char set_algorithms[256] = { 0 }; char namebuf[DNS_NAME_FORMATSIZE]; char algbuf[DNS_SECALG_FORMATSIZE]; char typebuf[DNS_RDATATYPE_FORMATSIZE]; @@ -862,7 +862,6 @@ verifyset(vctx_t *vctx, dns_rdataset_t *rdataset, const dns_name_t *name, goto done; } - memset(set_algorithms, 0, sizeof(set_algorithms)); for (result = dns_rdataset_first(&sigrdataset); result == ISC_R_SUCCESS; result = dns_rdataset_next(&sigrdataset)) { @@ -926,7 +925,7 @@ verifynode(vctx_t *vctx, const dns_name_t *name, dns_dbnode_t *node, bool delegation, dst_key_t **dstkeys, size_t nkeys, dns_rdataset_t *nsecset, dns_rdataset_t *nsec3paramset, const dns_name_t *nextname, isc_result_t *vresult) { - unsigned char types[8192]; + unsigned char types[8192] = { 0 }; unsigned int maxtype = 0; dns_rdataset_t rdataset; dns_rdatasetiter_t *rdsiter = NULL; @@ -934,7 +933,6 @@ verifynode(vctx_t *vctx, const dns_name_t *name, dns_dbnode_t *node, REQUIRE(vresult != NULL || (nsecset == NULL && nsec3paramset == NULL)); - memset(types, 0, sizeof(types)); result = dns_db_allrdatasets(vctx->db, node, vctx->ver, 0, &rdsiter); if (result != ISC_R_SUCCESS) { zoneverify_log_error(vctx, "dns_db_allrdatasets(): %s", From 6a12e37382c42ae349b860e450710c1927e42671 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Thu, 18 Jun 2020 17:26:46 -0300 Subject: [PATCH 4/7] Use sizeof instead of arbitrary number to iterate fixed size array More details on thread: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12180 --- lib/dns/zoneverify.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 22427764fb..335f9ec51a 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -828,7 +828,6 @@ verifyset(vctx_t *vctx, dns_rdataset_t *rdataset, const dns_name_t *name, dns_rdataset_t sigrdataset; dns_rdatasetiter_t *rdsiter = NULL; isc_result_t result; - int i; dns_rdataset_init(&sigrdataset); result = dns_db_allrdatasets(vctx->db, node, vctx->ver, 0, &rdsiter); @@ -853,7 +852,7 @@ verifyset(vctx_t *vctx, dns_rdataset_t *rdataset, const dns_name_t *name, dns_rdatatype_format(rdataset->type, typebuf, sizeof(typebuf)); zoneverify_log_error(vctx, "No signatures for %s/%s", namebuf, typebuf); - for (i = 0; i < 256; i++) { + for (size_t i = 0; i < ARRAY_SIZE(set_algorithms); i++) { if (vctx->act_algorithms[i] != 0) { vctx->bad_algorithms[i] = 1; } @@ -895,10 +894,10 @@ verifyset(vctx_t *vctx, dns_rdataset_t *rdataset, const dns_name_t *name, result = ISC_R_SUCCESS; if (memcmp(set_algorithms, vctx->act_algorithms, - sizeof(set_algorithms))) { + sizeof(set_algorithms)) != 0) { dns_name_format(name, namebuf, sizeof(namebuf)); dns_rdatatype_format(rdataset->type, typebuf, sizeof(typebuf)); - for (i = 0; i < 256; i++) { + for (size_t i = 0; i < ARRAY_SIZE(set_algorithms); i++) { if ((vctx->act_algorithms[i] != 0) && (set_algorithms[i] == 0)) { dns_secalg_format(i, algbuf, sizeof(algbuf)); @@ -939,6 +938,7 @@ verifynode(vctx_t *vctx, const dns_name_t *name, dns_dbnode_t *node, isc_result_totext(result)); return (result); } + result = dns_rdatasetiter_first(rdsiter); dns_rdataset_init(&rdataset); while (result == ISC_R_SUCCESS) { @@ -1670,11 +1670,10 @@ determine_active_algorithms(vctx_t *vctx, bool ignore_kskflag, bool keyset_kskonly, void (*report)(const char *, ...)) { char algbuf[DNS_SECALG_FORMATSIZE]; - int i; report("Verifying the zone using the following algorithms:"); - for (i = 0; i < 256; i++) { + for (size_t i = 0; i < ARRAY_SIZE(vctx->act_algorithms); i++) { if (ignore_kskflag) { vctx->act_algorithms[i] = (vctx->ksk_algorithms[i] != 0 || @@ -1696,7 +1695,7 @@ determine_active_algorithms(vctx_t *vctx, bool ignore_kskflag, return; } - for (i = 0; i < 256; i++) { + for (size_t i = 0; i < ARRAY_SIZE(vctx->ksk_algorithms); i++) { /* * The counts should both be zero or both be non-zero. Mark * the algorithm as bad if this is not met. @@ -1942,9 +1941,8 @@ static isc_result_t check_bad_algorithms(const vctx_t *vctx, void (*report)(const char *, ...)) { char algbuf[DNS_SECALG_FORMATSIZE]; bool first = true; - int i; - for (i = 0; i < 256; i++) { + for (size_t i = 0; i < ARRAY_SIZE(vctx->bad_algorithms); i++) { if (vctx->bad_algorithms[i] == 0) { continue; } @@ -1968,10 +1966,9 @@ static void print_summary(const vctx_t *vctx, bool keyset_kskonly, void (*report)(const char *, ...)) { char algbuf[DNS_SECALG_FORMATSIZE]; - int i; report("Zone fully signed:"); - for (i = 0; i < 256; i++) { + for (size_t i = 0; i < ARRAY_SIZE(vctx->ksk_algorithms); i++) { if ((vctx->ksk_algorithms[i] == 0) && (vctx->standby_ksk[i] == 0) && (vctx->revoked_ksk[i] == 0) && From 37f42d19a1d093b3297467fe519f5b1f44ba0e23 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Fri, 26 Jun 2020 18:53:04 -0300 Subject: [PATCH 5/7] Remove redundant function 'newchain' The removed function 'newchain(a, b)' was almost the same as calling !chain_equal(a, b), varying only in the amount of data compared in the non-fixed-length data portion of given chain nodes. A third argument 'data_size' has been introduced into 'chain_equal' function in order to allow it to know how many bytes to compare in the variable-length data portion of the chain nodes. A helper function 'chain_length(e)' has been introduced to allow easy calculation of the total length of the non-fixed-length data part of chain nodes. Check the thread below for more details: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12184 --- lib/dns/zoneverify.c | 46 +++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 335f9ec51a..7ab2b51002 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -95,6 +95,15 @@ struct nsec3_chain_fixed { */ }; +/* + * Helper function used to calculate length of variable-length + * data section in object pointed to by 'chain'. + */ +static inline size_t +chain_length(struct nsec3_chain_fixed *chain) { + return (chain->salt_length + 2 * chain->next_length); +} + /*% * Log a zone verification error described by 'fmt' and the variable arguments * following it. Either use dns_zone_logv() or print to stderr, depending on @@ -333,8 +342,6 @@ check_no_rrsig(const vctx_t *vctx, const dns_rdataset_t *rdataset, static bool chain_compare(void *arg1, void *arg2) { struct nsec3_chain_fixed *e1 = arg1, *e2 = arg2; - size_t len; - /* * Do each element in turn to get a stable sort. */ @@ -362,8 +369,7 @@ chain_compare(void *arg1, void *arg2) { if (e1->next_length > e2->next_length) { return (false); } - len = e1->salt_length + 2 * e1->next_length; - if (memcmp(e1 + 1, e2 + 1, len) < 0) { + if (memcmp(e1 + 1, e2 + 1, chain_length(e1)) < 0) { return (true); } return (false); @@ -371,9 +377,7 @@ chain_compare(void *arg1, void *arg2) { static bool chain_equal(const struct nsec3_chain_fixed *e1, - const struct nsec3_chain_fixed *e2) { - size_t len; - + const struct nsec3_chain_fixed *e2, size_t data_length) { if (e1->hash != e2->hash) { return (false); } @@ -386,11 +390,8 @@ chain_equal(const struct nsec3_chain_fixed *e1, if (e1->next_length != e2->next_length) { return (false); } - len = e1->salt_length + 2 * e1->next_length; - if (memcmp(e1 + 1, e2 + 1, len) != 0) { - return (false); - } - return (true); + + return (memcmp(e1 + 1, e2 + 1, data_length) == 0); } static isc_result_t @@ -1062,19 +1063,6 @@ check_no_nsec(const vctx_t *vctx, const dns_name_t *name, dns_dbnode_t *node) { return (nsec_exists ? ISC_R_FAILURE : ISC_R_SUCCESS); } -static bool -newchain(const struct nsec3_chain_fixed *first, - const struct nsec3_chain_fixed *e) { - if (first->hash != e->hash || first->iterations != e->iterations || - first->salt_length != e->salt_length || - first->next_length != e->next_length || - memcmp(first + 1, e + 1, first->salt_length) != 0) - { - return (true); - } - return (false); -} - static void free_element(isc_mem_t *mctx, struct nsec3_chain_fixed *e) { size_t len; @@ -1173,7 +1161,7 @@ verify_nsec3_chains(const vctx_t *vctx, isc_mem_t *mctx) { /* * Check that they match. */ - if (chain_equal(e, f)) { + if (chain_equal(e, f, chain_length(e))) { free_element(mctx, f); f = NULL; } else { @@ -1196,7 +1184,9 @@ verify_nsec3_chains(const vctx_t *vctx, isc_mem_t *mctx) { isc_heap_delete( vctx->found_chains, 1); } - if (f != NULL && chain_equal(e, f)) { + if (f != NULL && + chain_equal(e, f, chain_length(e))) + { free_element(mctx, f); f = NULL; break; @@ -1212,7 +1202,7 @@ verify_nsec3_chains(const vctx_t *vctx, isc_mem_t *mctx) { if (first == NULL) { prev = first = e; - } else if (newchain(first, e)) { + } else if (!chain_equal(first, e, first->salt_length)) { if (!checklast(mctx, vctx, first, prev)) { result = ISC_R_FAILURE; } From 90e724af4571bd28586c0845c8b0fc59fc5b1d93 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Sat, 27 Jun 2020 16:37:23 -0300 Subject: [PATCH 6/7] Avoid an empty block under if condition This commit doesn't change the logic flow from previous code, it only makes the code more readable and consistent. More details on thread: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12185 --- lib/dns/zoneverify.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 7ab2b51002..86f46b7092 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -1605,9 +1605,9 @@ check_dnskey(vctx_t *vctx) { RUNTIME_CHECK(result == ISC_R_SUCCESS); is_ksk = ((dnskey.flags & DNS_KEYFLAG_KSK) != 0); - if ((dnskey.flags & DNS_KEYOWNER_ZONE) == 0) { - /* Non zone key, skip. */ - } else if ((dnskey.flags & DNS_KEYFLAG_REVOKE) != 0) { + if ((dnskey.flags & DNS_KEYOWNER_ZONE) != 0 && + (dnskey.flags & DNS_KEYFLAG_REVOKE) != 0) + { if ((dnskey.flags & DNS_KEYFLAG_KSK) != 0 && !dns_dnssec_selfsigns(&rdata, vctx->origin, &vctx->keyset, &vctx->keysigs, From bd0cc048d170262299526239c642da0e285d1072 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Sat, 27 Jun 2020 16:59:41 -0300 Subject: [PATCH 7/7] Replace literal 255 with a more descriptive macro name More details on thread: https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/291#note_12186 --- lib/dns/include/dns/keyvalues.h | 1 + lib/dns/zoneverify.c | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/dns/include/dns/keyvalues.h b/lib/dns/include/dns/keyvalues.h index cc5dea7f02..db63d40150 100644 --- a/lib/dns/include/dns/keyvalues.h +++ b/lib/dns/include/dns/keyvalues.h @@ -70,6 +70,7 @@ #define DNS_KEYALG_INDIRECT 252 #define DNS_KEYALG_PRIVATEDNS 253 #define DNS_KEYALG_PRIVATEOID 254 /*%< Key begins with OID giving alg */ +#define DNS_KEYALG_MAX 255 /* Protocol values */ #define DNS_KEYPROTO_RESERVED 0 diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 86f46b7092..b51c0b1371 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -1486,18 +1486,18 @@ check_dnskey_sigs(vctx_t *vctx, const dns_rdata_dnskey_t *dnskey, dns_dnssec_signs(keyrdata, vctx->origin, &vctx->soaset, &vctx->soasigs, false, vctx->mctx)) { - if (active_keys[dnskey->algorithm] != 255) { + if (active_keys[dnskey->algorithm] != DNS_KEYALG_MAX) { active_keys[dnskey->algorithm]++; } } else { - if (standby_keys[dnskey->algorithm] != 255) { + if (standby_keys[dnskey->algorithm] != DNS_KEYALG_MAX) { standby_keys[dnskey->algorithm]++; } } return; } - if (active_keys[dnskey->algorithm] != 255) { + if (active_keys[dnskey->algorithm] != DNS_KEYALG_MAX) { active_keys[dnskey->algorithm]++; } @@ -1637,11 +1637,13 @@ check_dnskey(vctx_t *vctx) { return (ISC_R_FAILURE); } if ((dnskey.flags & DNS_KEYFLAG_KSK) != 0 && - vctx->revoked_ksk[dnskey.algorithm] != 255) + vctx->revoked_ksk[dnskey.algorithm] != + DNS_KEYALG_MAX) { vctx->revoked_ksk[dnskey.algorithm]++; } else if ((dnskey.flags & DNS_KEYFLAG_KSK) == 0 && - vctx->revoked_zsk[dnskey.algorithm] != 255) + vctx->revoked_zsk[dnskey.algorithm] != + DNS_KEYALG_MAX) { vctx->revoked_zsk[dnskey.algorithm]++; }