From d2ba96488ea30eaeb225c12ea77e71de399b6175 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 29 Jun 2023 09:15:38 +1000 Subject: [PATCH 01/25] Address races in dns_tsigkey_find() 1) Restart the process with a write lock if we discover an expired key while holding the read lock. 2) Move incrementing the key reference inside the lock block of code. --- lib/dns/tsig.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c index 5583688e88..e4821194c0 100644 --- a/lib/dns/tsig.c +++ b/lib/dns/tsig.c @@ -1539,38 +1539,44 @@ isc_result_t dns_tsigkey_find(dns_tsigkey_t **tsigkey, const dns_name_t *name, const dns_name_t *algorithm, dns_tsigkeyring_t *ring) { dns_tsigkey_t *key = NULL; - isc_stdtime_t now = isc_stdtime_now(); isc_result_t result; + isc_rwlocktype_t locktype = isc_rwlocktype_read; + isc_stdtime_t now = isc_stdtime_now(); REQUIRE(name != NULL); REQUIRE(VALID_TSIGKEYRING(ring)); REQUIRE(tsigkey != NULL && *tsigkey == NULL); - RWLOCK(&ring->lock, isc_rwlocktype_read); +again: + RWLOCK(&ring->lock, locktype); result = isc_hashmap_find(ring->keys, dns_name_hash(name), tkey_match, name, (void **)&key); if (result == ISC_R_NOTFOUND) { - RWUNLOCK(&ring->lock, isc_rwlocktype_read); + RWUNLOCK(&ring->lock, locktype); return (result); } if (algorithm != NULL && !dns_name_equal(key->algorithm, algorithm)) { - RWUNLOCK(&ring->lock, isc_rwlocktype_read); + RWUNLOCK(&ring->lock, locktype); return (ISC_R_NOTFOUND); } if (key->inception != key->expire && isc_serial_lt(key->expire, now)) { /* * The key has expired. */ - RWUNLOCK(&ring->lock, isc_rwlocktype_read); - RWLOCK(&ring->lock, isc_rwlocktype_write); + if (locktype == isc_rwlocktype_read) { + RWUNLOCK(&ring->lock, locktype); + locktype = isc_rwlocktype_write; + key = NULL; + goto again; + } rm_lru(key); rm_hashmap(key); - RWUNLOCK(&ring->lock, isc_rwlocktype_write); + RWUNLOCK(&ring->lock, locktype); return (ISC_R_NOTFOUND); } - RWUNLOCK(&ring->lock, isc_rwlocktype_read); - adjust_lru(key); dns_tsigkey_ref(key); + RWUNLOCK(&ring->lock, locktype); + adjust_lru(key); *tsigkey = key; return (ISC_R_SUCCESS); } From a62cda787ffdced3fc2ae31d8da86cba17a3f95f Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 29 Jun 2023 09:27:29 +1000 Subject: [PATCH 02/25] Add CHANGES note for [GL #4182] --- CHANGES | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index be62c90fd0..b524775c09 100644 --- a/CHANGES +++ b/CHANGES @@ -8,7 +8,8 @@ 6315. [placeholder] -6314. [placeholder] +6314. [bug] Address race conditions in dns_tsigkey_find(). + [GL #4182] 6313. [bug] When dnssec-policy is in effect the DNSKEY's TTLs in the zone where not being updated to match the policy. From b8a96317544c7b310b4f74360825a87b6402ddc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 11 Sep 2023 10:35:28 +0200 Subject: [PATCH 03/25] Use hashmap when parsing a message When parsing messages use a hashmap instead of a linear search to reduce the amount of work done in findname when there's more than one name in the section. There are two hashmaps: 1) hashmap for owner names - that's constructed for each section when we hit the second name in the section and destroyed right after parsing that section; 2) per-name hashmap - for each name in the section, we construct a new hashmap for that name if there are more than one rdataset for that particular name. --- lib/dns/include/dns/message.h | 21 -- lib/dns/include/dns/name.h | 2 + lib/dns/message.c | 362 ++++++++++++++++++++++------------ 3 files changed, 242 insertions(+), 143 deletions(-) diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index dc0c05846f..24acd4b8dc 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -859,27 +859,6 @@ dns_message_findtype(const dns_name_t *name, dns_rdatatype_t type, *\li #ISC_R_NOTFOUND -- the desired type does not exist. */ -isc_result_t -dns_message_find(const dns_name_t *name, dns_rdataclass_t rdclass, - dns_rdatatype_t type, dns_rdatatype_t covers, - dns_rdataset_t **rdataset); -/*%< - * Search the name for the specified rdclass and type. If it is found, - * *rdataset is filled in with a pointer to that rdataset. - * - * Requires: - *\li if '**rdataset' is non-NULL, *rdataset needs to be NULL. - * - *\li 'type' be a valid type, and NOT dns_rdatatype_any. - * - *\li If 'type' is dns_rdatatype_rrsig, 'covers' must be a valid type. - * Otherwise it should be 0. - * - * Returns: - *\li #ISC_R_SUCCESS -- all is well. - *\li #ISC_R_NOTFOUND -- the desired type does not exist. - */ - void dns_message_addname(dns_message_t *msg, dns_name_t *name, dns_section_t section); diff --git a/lib/dns/include/dns/name.h b/lib/dns/include/dns/name.h index d0d92bbe51..7a3ae95477 100644 --- a/lib/dns/include/dns/name.h +++ b/lib/dns/include/dns/name.h @@ -68,6 +68,7 @@ #include #include +#include #include #include #include /* Required for storage size of dns_label_t. */ @@ -119,6 +120,7 @@ struct dns_name { isc_buffer_t *buffer; ISC_LINK(dns_name_t) link; ISC_LIST(dns_rdataset_t) list; + isc_hashmap_t *hashmap; }; #define DNS_NAME_MAGIC ISC_MAGIC('D', 'N', 'S', 'n') diff --git a/lib/dns/message.c b/lib/dns/message.c index c85e579b02..8280aa287f 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -22,6 +22,8 @@ #include #include +#include +#include #include #include #include @@ -779,6 +781,11 @@ ISC_REFCOUNT_TRACE_IMPL(dns_message, dns__message_destroy); ISC_REFCOUNT_IMPL(dns_message, dns__message_destroy); #endif +static bool +name_match(void *node, const void *key) { + return (dns_name_equal(node, key)); +} + static isc_result_t findname(dns_name_t **foundname, const dns_name_t *target, dns_namelist_t *section) { @@ -796,26 +803,25 @@ findname(dns_name_t **foundname, const dns_name_t *target, return (ISC_R_NOTFOUND); } -isc_result_t -dns_message_find(const dns_name_t *name, dns_rdataclass_t rdclass, - dns_rdatatype_t type, dns_rdatatype_t covers, - dns_rdataset_t **rdatasetp) { - dns_rdataset_t *rds = NULL; +static uint32_t +rds_hash(dns_rdataset_t *rds) { + isc_hash32_t state; - REQUIRE(name != NULL); - REQUIRE(rdatasetp == NULL || *rdatasetp == NULL); + isc_hash32_init(&state); + isc_hash32_hash(&state, &rds->rdclass, sizeof(rds->rdclass), true); + isc_hash32_hash(&state, &rds->type, sizeof(rds->type), true); + isc_hash32_hash(&state, &rds->covers, sizeof(rds->covers), true); - ISC_LIST_FOREACH_REV (name->list, rds, link) { - if (rds->rdclass == rdclass && rds->type == type && - rds->covers == covers) - { - SET_IF_NOT_NULL(rdatasetp, rds); + return (isc_hash32_finalize(&state)); +} - return (ISC_R_SUCCESS); - } - } +static bool +rds_match(void *node, const void *key0) { + const dns_rdataset_t *rds = node; + const dns_rdataset_t *key = key0; - return (ISC_R_NOTFOUND); + return (rds->rdclass == key->rdclass && rds->type == key->type && + rds->covers == key->covers); } isc_result_t @@ -939,22 +945,38 @@ getrdata(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, } \ } while (0) +static void +cleanup_name_hashmaps(dns_namelist_t *section) { + dns_name_t *name = NULL; + ISC_LIST_FOREACH (*section, name, link) { + if (name->hashmap != NULL) { + isc_hashmap_destroy(&name->hashmap); + } + } +} + static isc_result_t getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, unsigned int options) { isc_region_t r; unsigned int count; dns_name_t *name = NULL; - dns_name_t *name2 = NULL; + dns_name_t *found_name = NULL; dns_rdataset_t *rdataset = NULL; dns_rdatalist_t *rdatalist = NULL; - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; dns_rdatatype_t rdtype; dns_rdataclass_t rdclass; dns_namelist_t *section = &msg->sections[DNS_SECTION_QUESTION]; bool best_effort = ((options & DNS_MESSAGEPARSE_BESTEFFORT) != 0); bool seen_problem = false; bool free_name = false; + bool free_hashmaps = false; + isc_hashmap_t *name_map = NULL; + + if (msg->counts[DNS_SECTION_QUESTION] > 1) { + isc_hashmap_create(msg->mctx, 1, &name_map); + } for (count = 0; count < msg->counts[DNS_SECTION_QUESTION]; count++) { name = NULL; @@ -972,13 +994,21 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, goto cleanup; } + /* If there is only one QNAME, skip the duplicity checks */ + if (name_map == NULL) { + result = ISC_R_SUCCESS; + goto skip_name_check; + } + /* * Run through the section, looking to see if this name * is already there. If it is found, put back the allocated * name since we no longer need it, and set our name pointer * to point to the name we found. */ - result = findname(&name2, name, section); + result = isc_hashmap_add(name_map, dns_name_hash(name), + name_match, name, name, + (void **)&found_name); /* * If it is the first name in the section, accept it. @@ -990,19 +1020,25 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, * this should be legal or not. In either case we no longer * need this name pointer. */ - if (result != ISC_R_SUCCESS) { + skip_name_check: + switch (result) { + case ISC_R_SUCCESS: if (!ISC_LIST_EMPTY(*section)) { DO_ERROR(DNS_R_FORMERR); } ISC_LIST_APPEND(*section, name, link); - free_name = false; - } else { + break; + case ISC_R_EXISTS: dns_message_puttempname(msg, &name); - name = name2; - name2 = NULL; - free_name = false; + name = found_name; + found_name = NULL; + break; + default: + UNREACHABLE(); } + free_name = false; + /* * Get type and class. */ @@ -1032,53 +1068,83 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, msg->tkey = 1; } - /* - * Can't ask the same question twice. - */ - result = dns_message_find(name, rdclass, rdtype, 0, NULL); - if (result == ISC_R_SUCCESS) { - DO_ERROR(DNS_R_FORMERR); - } - /* * Allocate a new rdatalist. */ rdatalist = newrdatalist(msg); - if (rdatalist == NULL) { - result = ISC_R_NOMEMORY; - goto cleanup; - } - rdataset = isc_mempool_get(msg->rdspool); + rdatalist->type = rdtype; + rdatalist->rdclass = rdclass; + rdatalist->covers = 0; /* * Convert rdatalist to rdataset, and attach the latter to * the name. */ - rdatalist->type = rdtype; - rdatalist->rdclass = rdclass; - - dns_rdataset_init(rdataset); + dns_message_gettemprdataset(msg, &rdataset); dns_rdatalist_tordataset(rdatalist, rdataset); rdataset->attributes |= DNS_RDATASETATTR_QUESTION; + /* + * Skip the duplicity check for first rdataset + */ + if (ISC_LIST_EMPTY(name->list)) { + result = ISC_R_SUCCESS; + goto skip_rds_check; + } + + /* + * Can't ask the same question twice. + */ + if (name->hashmap == NULL) { + isc_hashmap_create(msg->mctx, 1, &name->hashmap); + free_hashmaps = true; + + INSIST(ISC_LIST_HEAD(name->list) == + ISC_LIST_TAIL(name->list)); + + dns_rdataset_t *old_rdataset = + ISC_LIST_HEAD(name->list); + + result = isc_hashmap_add( + name->hashmap, rds_hash(old_rdataset), + rds_match, old_rdataset, old_rdataset, NULL); + + INSIST(result == ISC_R_SUCCESS); + } + result = isc_hashmap_add(name->hashmap, rds_hash(rdataset), + rds_match, rdataset, rdataset, NULL); + if (result == ISC_R_EXISTS) { + DO_ERROR(DNS_R_FORMERR); + } + + skip_rds_check: ISC_LIST_APPEND(name->list, rdataset, link); + rdataset = NULL; } if (seen_problem) { - return (DNS_R_RECOVERABLE); + result = DNS_R_RECOVERABLE; } - return (ISC_R_SUCCESS); cleanup: if (rdataset != NULL) { dns_message_puttemprdataset(msg, &rdataset); } + if (free_name) { dns_message_puttempname(msg, &name); } + if (free_hashmaps) { + cleanup_name_hashmaps(section); + } + + if (name_map != NULL) { + isc_hashmap_destroy(&name_map); + } + return (result); } @@ -1105,7 +1171,6 @@ auth_signed(dns_namelist_t *section) { ISC_LIST_FOREACH (*section, name, link) { int auth_dnssec = 0, auth_rrsig = 0; dns_rdataset_t *rds = NULL; - ISC_LIST_FOREACH (name->list, rds, link) { switch (rds->type) { case dns_rdatatype_ds: @@ -1152,19 +1217,26 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, isc_region_t r; unsigned int count, rdatalen; dns_name_t *name = NULL; - dns_name_t *name2 = NULL; + dns_name_t *found_name = NULL; dns_rdataset_t *rdataset = NULL; + dns_rdataset_t *found_rdataset = NULL; dns_rdatalist_t *rdatalist = NULL; - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; dns_rdatatype_t rdtype, covers; dns_rdataclass_t rdclass; dns_rdata_t *rdata = NULL; dns_ttl_t ttl; dns_namelist_t *section = &msg->sections[sectionid]; - bool free_name = false, free_rdataset = false, seen_problem = false; + bool free_name = false, seen_problem = false; + bool free_hashmaps = false; bool preserve_order = ((options & DNS_MESSAGEPARSE_PRESERVEORDER) != 0); bool best_effort = ((options & DNS_MESSAGEPARSE_BESTEFFORT) != 0); bool isedns, issigzero, istsig; + isc_hashmap_t *name_map = NULL; + + if (msg->counts[sectionid] > 1) { + isc_hashmap_create(msg->mctx, 1, &name_map); + } for (count = 0; count < msg->counts[sectionid]; count++) { int recstart = source->current; @@ -1172,7 +1244,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, skip_name_search = false; skip_type_search = false; - free_rdataset = false; isedns = false; issigzero = false; istsig = false; @@ -1212,8 +1283,8 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, if (msg->rdclass_set == 0 && rdtype != dns_rdatatype_opt && /* class is UDP SIZE */ rdtype != dns_rdatatype_tsig && /* class is ANY */ - rdtype != dns_rdatatype_tkey) - { /* class is undefined */ + rdtype != dns_rdatatype_tkey) /* class is undefined */ + { msg->rdclass = rdclass; msg->rdclass_set = 1; } @@ -1412,61 +1483,128 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, free_name = false; } } else { + if (name_map == NULL) { + result = ISC_R_SUCCESS; + goto skip_name_check; + } + /* * Run through the section, looking to see if this name * is already there. If it is found, put back the * allocated name since we no longer need it, and set * our name pointer to point to the name we found. */ - result = findname(&name2, name, section); + result = isc_hashmap_add(name_map, dns_name_hash(name), + name_match, name, name, + (void **)&found_name); /* * If it is a new name, append to the section. */ - if (result == ISC_R_SUCCESS) { - dns_message_puttempname(msg, &name); - name = name2; - } else { + skip_name_check: + switch (result) { + case ISC_R_SUCCESS: ISC_LIST_APPEND(*section, name, link); + break; + case ISC_R_EXISTS: + dns_message_puttempname(msg, &name); + name = found_name; + found_name = NULL; + break; + default: + UNREACHABLE(); } free_name = false; } + rdatalist = newrdatalist(msg); + rdatalist->type = rdtype; + rdatalist->covers = covers; + rdatalist->rdclass = rdclass; + rdatalist->ttl = ttl; + + dns_message_gettemprdataset(msg, &rdataset); + dns_rdatalist_tordataset(rdatalist, rdataset); + dns_rdataset_setownercase(rdataset, name); + rdatalist = NULL; + /* * Search name for the particular type and class. * Skip this stage if in update mode or this is a meta-type. */ - if (preserve_order || msg->opcode == dns_opcode_update || - skip_type_search) + if (isedns || istsig || issigzero) { + /* Skip adding the rdataset to the tables */ + } else if (preserve_order || msg->opcode == dns_opcode_update || + skip_type_search) { - result = ISC_R_NOTFOUND; + result = ISC_R_SUCCESS; + + ISC_LIST_APPEND(name->list, rdataset, link); } else { /* * If this is a type that can only occur in * the question section, fail. */ if (dns_rdatatype_questiononly(rdtype)) { + dns_message_puttemprdataset(msg, &rdataset); DO_ERROR(DNS_R_FORMERR); } - rdataset = NULL; - result = dns_message_find(name, rdclass, rdtype, covers, - &rdataset); - } + if (ISC_LIST_EMPTY(name->list)) { + result = ISC_R_SUCCESS; + goto skip_rds_check; + } + + if (name->hashmap == NULL) { + isc_hashmap_create(msg->mctx, 1, + &name->hashmap); + free_hashmaps = true; + + INSIST(ISC_LIST_HEAD(name->list) == + ISC_LIST_TAIL(name->list)); + + dns_rdataset_t *old_rdataset = + ISC_LIST_HEAD(name->list); + + result = isc_hashmap_add( + name->hashmap, rds_hash(old_rdataset), + rds_match, old_rdataset, old_rdataset, + NULL); + + INSIST(result == ISC_R_SUCCESS); + } + + result = isc_hashmap_add( + name->hashmap, rds_hash(rdataset), rds_match, + rdataset, rdataset, (void **)&found_rdataset); + + /* + * If we found an rdataset that matches, we need to + * append this rdata to that set. If we did not, we + * need to create a new rdatalist, store the important + * bits there, convert it to an rdataset, and link the + * latter to the name. Yuck. When appending, make + * certain that the type isn't a singleton type, such as + * SOA or CNAME. + * + * Note that this check will be bypassed when preserving + * order, the opcode is an update, or the type search is + * skipped. + */ + skip_rds_check: + switch (result) { + case ISC_R_EXISTS: + /* Free the rdataset we used as the key */ + dns__message_putassociatedrdataset(msg, + &rdataset); + rdataset = found_rdataset; + + result = ISC_R_SUCCESS; + + if (!dns_rdatatype_issingleton(rdtype)) { + break; + } - /* - * If we found an rdataset that matches, we need to - * append this rdata to that set. If we did not, we need - * to create a new rdatalist, store the important bits there, - * convert it to an rdataset, and link the latter to the name. - * Yuck. When appending, make certain that the type isn't - * a singleton type, such as SOA or CNAME. - * - * Note that this check will be bypassed when preserving order, - * the opcode is an update, or the type search is skipped. - */ - if (result == ISC_R_SUCCESS) { - if (dns_rdatatype_issingleton(rdtype)) { dns_rdata_t *first; dns_rdatalist_fromrdataset(rdataset, &rdatalist); @@ -1475,31 +1613,12 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, if (dns_rdata_compare(rdata, first) != 0) { DO_ERROR(DNS_R_FORMERR); } - } - } - - if (result == ISC_R_NOTFOUND) { - rdataset = isc_mempool_get(msg->rdspool); - free_rdataset = true; - - rdatalist = newrdatalist(msg); - if (rdatalist == NULL) { - result = ISC_R_NOMEMORY; - goto cleanup; - } - - rdatalist->type = rdtype; - rdatalist->covers = covers; - rdatalist->rdclass = rdclass; - rdatalist->ttl = ttl; - - dns_rdataset_init(rdataset); - dns_rdatalist_tordataset(rdatalist, rdataset); - dns_rdataset_setownercase(rdataset, name); - - if (!isedns && !istsig && !issigzero) { + break; + case ISC_R_SUCCESS: ISC_LIST_APPEND(name->list, rdataset, link); - free_rdataset = false; + break; + default: + UNREACHABLE(); } } @@ -1535,7 +1654,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, msg->opt = rdataset; rdataset = NULL; - free_rdataset = false; ercode = (dns_rcode_t)((msg->opt->ttl & DNS_MESSAGE_EDNSRCODE_MASK) >> 20); @@ -1547,7 +1665,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, msg->sig0name = name; msg->sigstart = recstart; rdataset = NULL; - free_rdataset = false; free_name = false; } else if (istsig) { msg->tsig = rdataset; @@ -1558,7 +1675,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, */ msg->tsigname->attributes.nocompress = true; rdataset = NULL; - free_rdataset = false; free_name = false; } @@ -1566,14 +1682,11 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, if (free_name) { dns_message_puttempname(msg, &name); } - if (free_rdataset) { - dns__message_putassociatedrdataset(msg, - &rdataset); - } - free_name = free_rdataset = false; + free_name = false; } INSIST(!free_name); - INSIST(!free_rdataset); + + rdataset = NULL; } /* @@ -1591,16 +1704,20 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, } if (seen_problem) { - return (DNS_R_RECOVERABLE); + result = DNS_R_RECOVERABLE; } - return (ISC_R_SUCCESS); cleanup: if (free_name) { dns_message_puttempname(msg, &name); } - if (free_rdataset) { - dns__message_putassociatedrdataset(msg, &rdataset); + + if (free_hashmaps) { + cleanup_name_hashmaps(section); + } + + if (name_map != NULL) { + isc_hashmap_destroy(&name_map); } return (result); @@ -1661,6 +1778,7 @@ dns_message_parse(dns_message_t *msg, isc_buffer_t *source, dctx = DNS_DECOMPRESS_ALWAYS; ret = getquestions(source, msg, dctx, options); + if (ret == ISC_R_UNEXPECTEDEND && ignore_tc) { goto truncated; } @@ -2349,15 +2467,13 @@ dns_message_renderreset(dns_message_t *msg) { dns_message_puttempname(msg, &msg->tsigname); } if (msg->tsig != NULL) { - dns_rdataset_disassociate(msg->tsig); - dns_message_puttemprdataset(msg, &msg->tsig); + dns__message_putassociatedrdataset(msg, &msg->tsig); } if (msg->sig0name != NULL) { dns_message_puttempname(msg, &msg->sig0name); } if (msg->sig0 != NULL) { - dns_rdataset_disassociate(msg->sig0); - dns_message_puttemprdataset(msg, &msg->sig0); + dns__message_putassociatedrdataset(msg, &msg->sig0); } } @@ -2406,7 +2522,7 @@ dns_message_findname(dns_message_t *msg, dns_section_t section, const dns_name_t *target, dns_rdatatype_t type, dns_rdatatype_t covers, dns_name_t **name, dns_rdataset_t **rdataset) { - dns_name_t *foundname; + dns_name_t *foundname = NULL; isc_result_t result; /* @@ -2524,6 +2640,10 @@ dns_message_puttempname(dns_message_t *msg, dns_name_t **itemp) { REQUIRE(!ISC_LINK_LINKED(item, link)); REQUIRE(ISC_LIST_HEAD(item->list) == NULL); + if (item->hashmap != NULL) { + isc_hashmap_destroy(&item->hashmap); + } + /* * we need to check this in case dns_name_dup() was used. */ @@ -2551,8 +2671,7 @@ dns_message_puttemprdata(dns_message_t *msg, dns_rdata_t **item) { static void dns__message_putassociatedrdataset(dns_message_t *msg, dns_rdataset_t **item) { dns_rdataset_disassociate(*item); - isc_mempool_put(msg->rdspool, *item); - *item = NULL; + dns_message_puttemprdataset(msg, item); } void @@ -2730,8 +2849,7 @@ dns_message_setopt(dns_message_t *msg, dns_rdataset_t *opt) { return (ISC_R_SUCCESS); cleanup: - dns_rdataset_disassociate(opt); - dns_message_puttemprdataset(msg, &opt); + dns__message_putassociatedrdataset(msg, &opt); return (result); } From 30d27928cff8a82774131b401c26b171a2367e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 11 Oct 2023 09:15:13 +0200 Subject: [PATCH 04/25] Add CHANGES and release note for [GL #4234] --- CHANGES | 3 ++- doc/notes/notes-current.rst | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGES b/CHANGES index b524775c09..d3d5fa5a6e 100644 --- a/CHANGES +++ b/CHANGES @@ -6,7 +6,8 @@ 6316. [placeholder] -6315. [placeholder] +6315. [security] Speed up parsing of DNS messages with many different + names. (CVE-2023-4408) [GL #4234] 6314. [bug] Address race conditions in dns_tsigkey_find(). [GL #4182] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index bebf1cd3b7..518a160826 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -15,7 +15,13 @@ Notes for BIND 9.19.20 Security Fixes ~~~~~~~~~~~~~~ -- None. +- Parsing DNS messages with many different names could cause excessive + CPU load. This has been fixed. :cve:`2023-4408` + + ISC would like to thank Shoham Danino from Reichman University, Anat + Bremler-Barr from Tel-Aviv University, Yehuda Afek from Tel-Aviv + University, and Yuval Shavitt from Tel-Aviv University for bringing + this vulnerability to our attention. :gl:`#4234` New Features ~~~~~~~~~~~~ From 9d0fa07c5e7a39db89862a4f843d2190059afb4b Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 10 Oct 2023 10:58:18 +1100 Subject: [PATCH 05/25] Save the correct result value to resume with nxdomain-redirect The wrong result value was being saved for resumption with nxdomain-redirect when performing the fetch. This lead to an assert when checking that RFC 1918 reverse queries where not leaking to the global internet. --- lib/ns/query.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 34e47e2b1e..07537a8040 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -477,10 +477,10 @@ static void query_addnxrrsetnsec(query_ctx_t *qctx); static isc_result_t -query_nxdomain(query_ctx_t *qctx, isc_result_t res); +query_nxdomain(query_ctx_t *qctx, isc_result_t result); static isc_result_t -query_redirect(query_ctx_t *qctx); +query_redirect(query_ctx_t *qctx, isc_result_t result); static isc_result_t query_ncache(query_ctx_t *qctx, isc_result_t result); @@ -7695,8 +7695,7 @@ query_usestale(query_ctx_t *qctx, isc_result_t result) { * result from the search. */ static isc_result_t -query_gotanswer(query_ctx_t *qctx, isc_result_t res) { - isc_result_t result = res; +query_gotanswer(query_ctx_t *qctx, isc_result_t result) { char errmsg[256]; CCTRACE(ISC_LOG_DEBUG(3), "query_gotanswer"); @@ -7772,7 +7771,7 @@ root_key_sentinel: return (query_coveringnsec(qctx)); case DNS_R_NCACHENXDOMAIN: - result = query_redirect(qctx); + result = query_redirect(qctx, result); if (result != ISC_R_COMPLETE) { return (result); } @@ -9513,11 +9512,10 @@ query_addnxrrsetnsec(query_ctx_t *qctx) { * Handle NXDOMAIN and empty wildcard responses. */ static isc_result_t -query_nxdomain(query_ctx_t *qctx, isc_result_t res) { +query_nxdomain(query_ctx_t *qctx, isc_result_t result) { dns_section_t section; uint32_t ttl; - isc_result_t result = res; - bool empty_wild = (res == DNS_R_EMPTYWILD); + bool empty_wild = (result == DNS_R_EMPTYWILD); CCTRACE(ISC_LOG_DEBUG(3), "query_nxdomain"); @@ -9526,7 +9524,7 @@ query_nxdomain(query_ctx_t *qctx, isc_result_t res) { INSIST(qctx->is_zone || REDIRECT(qctx->client)); if (!empty_wild) { - result = query_redirect(qctx); + result = query_redirect(qctx, result); if (result != ISC_R_COMPLETE) { return (result); } @@ -9614,7 +9612,7 @@ cleanup: * redirecting, so query processing should continue past it. */ static isc_result_t -query_redirect(query_ctx_t *qctx) { +query_redirect(query_ctx_t *qctx, isc_result_t saved_result) { isc_result_t result; CCTRACE(ISC_LOG_DEBUG(3), "query_redirect"); @@ -9655,7 +9653,7 @@ query_redirect(query_ctx_t *qctx) { SAVE(qctx->client->query.redirect.rdataset, qctx->rdataset); SAVE(qctx->client->query.redirect.sigrdataset, qctx->sigrdataset); - qctx->client->query.redirect.result = DNS_R_NCACHENXDOMAIN; + qctx->client->query.redirect.result = saved_result; dns_name_copy(qctx->fname, qctx->client->query.redirect.fname); qctx->client->query.redirect.authoritative = qctx->authoritative; @@ -10250,7 +10248,7 @@ query_coveringnsec(query_ctx_t *qctx) { * We now have the proof that we have an NXDOMAIN. Apply * NXDOMAIN redirection if configured. */ - result = query_redirect(qctx); + result = query_redirect(qctx, DNS_R_COVERINGNSEC); if (result != ISC_R_COMPLETE) { redirected = true; goto cleanup; From 0748965b7ce7b80ff0062c7ffc661d52335c780e Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 10 Oct 2023 11:09:11 +1100 Subject: [PATCH 06/25] Add CHANGES note for [GL #4281] --- CHANGES | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index d3d5fa5a6e..1b27d586c0 100644 --- a/CHANGES +++ b/CHANGES @@ -4,7 +4,8 @@ 6317. [placeholder] -6316. [placeholder] +6316. [security] Specific queries could trigger an assertion check with + nxdomain-redirect enabled. (CVE-2023-5517) [GL #4281] 6315. [security] Speed up parsing of DNS messages with many different names. (CVE-2023-4408) [GL #4234] From 2fbafc2675d19136993980765c0589da599f8403 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 10 Oct 2023 11:12:24 +1100 Subject: [PATCH 07/25] Add release note for [GL #4281] --- doc/notes/notes-current.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 518a160826..f793805ae4 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -23,6 +23,10 @@ Security Fixes University, and Yuval Shavitt from Tel-Aviv University for bringing this vulnerability to our attention. :gl:`#4234` +- Specific queries could cause :iscman:`named` to crash with an + assertion failure when :any:`nxdomain-redirect` was enabled. This has + been fixed. :cve:`2023-5517` :gl:`#4281` + New Features ~~~~~~~~~~~~ From 1fcc483df13e049b96f620e515f0d4d45f3680b7 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 12 Oct 2023 12:01:46 +1100 Subject: [PATCH 08/25] Restore dns64 state during serve-stale processing If we are in the process of looking for the A records as part of dns64 processing and the server-stale timeout triggers, redo the dns64 changes that had been made to the orignal qctx. --- lib/ns/query.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/ns/query.c b/lib/ns/query.c index 07537a8040..b819dc4fdf 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6251,6 +6251,13 @@ query_lookup_stale(ns_client_t *client) { query_ctx_t qctx; qctx_init(client, NULL, client->query.qtype, &qctx); + if (DNS64(client)) { + qctx.qtype = qctx.type = dns_rdatatype_a; + qctx.dns64 = true; + } + if (DNS64EXCLUDE(client)) { + qctx.dns64_exclude = true; + } dns_db_attach(client->view->cachedb, &qctx.db); client->query.attributes &= ~NS_QUERYATTR_RECURSIONOK; client->query.dboptions |= DNS_DBFIND_STALETIMEOUT; From 26671f8c478a66296ee5874adbe741c890e435d1 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 12 Oct 2023 13:39:57 +1100 Subject: [PATCH 09/25] Add CHANGES note for [GL #4334] --- CHANGES | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 1b27d586c0..548208b3ef 100644 --- a/CHANGES +++ b/CHANGES @@ -2,7 +2,8 @@ 6318. [placeholder] -6317. [placeholder] +6317. [security] Restore DNS64 state when handling a serve-stale timeout. + (CVE-2023-5679) [GL #4334] 6316. [security] Specific queries could trigger an assertion check with nxdomain-redirect enabled. (CVE-2023-5517) [GL #4281] From c4faf5c69f7eba3b23b8a932e66fc89ec3bee4a9 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 12 Oct 2023 13:42:38 +1100 Subject: [PATCH 10/25] Add release note for [GL #4334] --- doc/notes/notes-current.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index f793805ae4..0021e57c5f 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -27,6 +27,11 @@ Security Fixes assertion failure when :any:`nxdomain-redirect` was enabled. This has been fixed. :cve:`2023-5517` :gl:`#4281` +- A bad interaction between DNS64 and serve-stale could cause + :iscman:`named` to crash with an assertion failure, when both of these + features were enabled. This has been fixed. :cve:`2023-5679` + :gl:`#4334` + New Features ~~~~~~~~~~~~ From 24381cc36d8528f5a4046fb2614451aeac4cdfc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 4 Jan 2024 13:39:27 +0100 Subject: [PATCH 11/25] Limit isc_async_run() overhead for tree pruning Instead of issuing a separate isc_async_run() call for every RBTDB node that triggers tree pruning, maintain a list of nodes from which tree pruning can be started from and only issue an isc_async_run() call if pruning has not yet been triggered by another RBTDB node. In some older BIND 9 branches, the extra queuing overhead eliminated by this change could be remotely exploited to cause excessive memory use. Due to architectural shift, this branch is not vulnerable to that issue, but applying the fix to the latter is nevertheless deemed prudent for consistency and to make the code future-proof. --- lib/dns/include/dns/rbt.h | 6 ++ lib/dns/rbt.c | 1 + lib/dns/rbtdb.c | 134 ++++++++++++++++++++++++-------------- lib/dns/rbtdb_p.h | 12 ++-- 4 files changed, 96 insertions(+), 57 deletions(-) diff --git a/lib/dns/include/dns/rbt.h b/lib/dns/include/dns/rbt.h index 93e429069b..ca974e5873 100644 --- a/lib/dns/include/dns/rbt.h +++ b/lib/dns/include/dns/rbt.h @@ -118,6 +118,12 @@ struct dns_rbtnode { */ ISC_LINK(dns_rbtnode_t) deadlink; + /*% + * This linked list is used to store nodes from which tree pruning can + * be started. + */ + ISC_LINK(dns_rbtnode_t) prunelink; + /*@{*/ /*! * These values are used in the RBT DB implementation. The appropriate diff --git a/lib/dns/rbt.c b/lib/dns/rbt.c index 54ddfd9175..34ec175bc3 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -1520,6 +1520,7 @@ create_node(isc_mem_t *mctx, const dns_name_t *name, dns_rbtnode_t **nodep) { }; ISC_LINK_INIT(node, deadlink); + ISC_LINK_INIT(node, prunelink); isc_refcount_init(&node->references, 0); diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 900b580058..3761cef590 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -439,6 +439,7 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { unsigned int i; isc_result_t result; char buf[DNS_NAME_FORMATSIZE]; + dns_rbtnode_t *node = NULL; dns_rbt_t **treep = NULL; isc_time_t start; @@ -461,8 +462,6 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { * the overhead of unlinking all nodes here should be negligible. */ for (i = 0; i < rbtdb->node_lock_count; i++) { - dns_rbtnode_t *node = NULL; - node = ISC_LIST_HEAD(rbtdb->deadnodes[i]); while (node != NULL) { ISC_LIST_UNLINK(rbtdb->deadnodes[i], node, deadlink); @@ -470,6 +469,12 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { } } + node = ISC_LIST_HEAD(rbtdb->prunenodes); + while (node != NULL) { + ISC_LIST_UNLINK(rbtdb->prunenodes, node, prunelink); + node = ISC_LIST_HEAD(rbtdb->prunenodes); + } + rbtdb->quantum = (rbtdb->loop != NULL) ? 100 : 0; for (;;) { @@ -1148,16 +1153,26 @@ is_leaf(dns_rbtnode_t *node) { node->left == NULL && node->right == NULL); } +/*% + * The tree lock must be held when this function is called as it reads and + * updates rbtdb->prunenodes. + */ static void send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, isc_rwlocktype_t locktype DNS__DB_FLARG) { - prune_t *prune = isc_mem_get(rbtdb->common.mctx, sizeof(*prune)); - *prune = (prune_t){ .node = node }; + bool pruning_queued = (ISC_LIST_HEAD(rbtdb->prunenodes) != NULL); + + INSIST(locktype == isc_rwlocktype_write); - dns_db_attach((dns_db_t *)rbtdb, &prune->db); dns__rbtdb_newref(rbtdb, node, locktype DNS__DB_FLARG_PASS); + INSIST(!ISC_LINK_LINKED(node, prunelink)); + ISC_LIST_APPEND(rbtdb->prunenodes, node, prunelink); - isc_async_run(rbtdb->loop, prune_tree, prune); + if (!pruning_queued) { + dns_db_t *db = NULL; + dns_db_attach((dns_db_t *)rbtdb, &db); + isc_async_run(rbtdb->loop, prune_tree, db); + } } /*% @@ -1455,64 +1470,83 @@ restore_locks: } /* - * Prune the tree by recursively cleaning-up single leaves. In the worst - * case, the number of iteration is the number of tree levels, which is at - * most the maximum number of domain name labels, i.e, 127. In practice, this - * should be much smaller (only a few times), and even the worst case would be - * acceptable for a single event. + * Prune the tree by recursively cleaning up single leaves. Go through all + * nodes stored in the rbtdb->prunenodes list; for each of them, in the worst + * case, it will be necessary to traverse a number of tree levels equal to the + * maximum legal number of domain name labels (127); in practice, the number of + * tree levels to traverse will virtually always be much smaller (a few levels + * at most). While holding the tree lock throughout this entire operation is + * less than ideal, so is splitting the latter up by queueing a separate + * prune_tree() run for each node to start pruning from (as queueing requires + * allocating memory and can therefore potentially be exploited to exhaust + * available memory). Also note that actually freeing up the memory used by + * RBTDB nodes (which is what this function does) is essential to keeping cache + * memory use in check, so since the tree lock needs to be acquired anyway, + * freeing as many nodes as possible before the tree lock gets released is + * prudent. */ static void prune_tree(void *arg) { - prune_t *prune = (prune_t *)arg; - dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)prune->db; - dns_rbtnode_t *node = prune->node; + dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)arg; + dns_rbtnode_t *node = NULL; dns_rbtnode_t *parent = NULL; unsigned int locknum; isc_rwlocktype_t tlocktype = isc_rwlocktype_none; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - isc_mem_put(rbtdb->common.mctx, prune, sizeof(*prune)); - TREE_WRLOCK(&rbtdb->tree_lock, &tlocktype); - locknum = node->locknum; - NODE_WRLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); - do { - parent = node->parent; - dns__rbtdb_decref(rbtdb, node, 0, &nlocktype, &tlocktype, true, - true DNS__DB_FILELINE); - if (parent != NULL && parent->down == NULL) { - /* - * node was the only down child of the parent and has - * just been removed. We'll then need to examine the - * parent. Keep the lock if possible; otherwise, - * release the old lock and acquire one for the parent. - */ - if (parent->locknum != locknum) { - NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, - &nlocktype); - locknum = parent->locknum; - NODE_WRLOCK(&rbtdb->node_locks[locknum].lock, - &nlocktype); + while ((node = ISC_LIST_HEAD(rbtdb->prunenodes)) != NULL) { + locknum = node->locknum; + NODE_WRLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); + do { + if (ISC_LINK_LINKED(node, prunelink)) { + ISC_LIST_UNLINK(rbtdb->prunenodes, node, + prunelink); } - /* - * We need to gain a reference to the node before - * decrementing it in the next iteration. - */ - if (ISC_LINK_LINKED(parent, deadlink)) { - ISC_LIST_UNLINK(rbtdb->deadnodes[locknum], + parent = node->parent; + dns__rbtdb_decref(rbtdb, node, 0, &nlocktype, + &tlocktype, true, + true DNS__DB_FILELINE); + + if (parent != NULL && parent->down == NULL) { + /* + * node was the only down child of the parent + * and has just been removed. We'll then need + * to examine the parent. Keep the lock if + * possible; otherwise, release the old lock and + * acquire one for the parent. + */ + if (parent->locknum != locknum) { + NODE_UNLOCK( + &rbtdb->node_locks[locknum].lock, + &nlocktype); + locknum = parent->locknum; + NODE_WRLOCK( + &rbtdb->node_locks[locknum].lock, + &nlocktype); + } + + /* + * We need to gain a reference to the node + * before decrementing it in the next iteration. + */ + if (ISC_LINK_LINKED(parent, deadlink)) { + ISC_LIST_UNLINK( + rbtdb->deadnodes[locknum], parent, deadlink); + } + dns__rbtdb_newref(rbtdb, parent, + nlocktype DNS__DB_FILELINE); + } else { + parent = NULL; } - dns__rbtdb_newref(rbtdb, parent, - nlocktype DNS__DB_FILELINE); - } else { - parent = NULL; - } - node = parent; - } while (node != NULL); - NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); + node = parent; + } while (node != NULL); + NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); + } TREE_UNLOCK(&rbtdb->tree_lock, &tlocktype); dns_db_detach((dns_db_t **)&rbtdb); @@ -3858,6 +3892,8 @@ dns__rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, ISC_LIST_INIT(rbtdb->deadnodes[i]); } + ISC_LIST_INIT(rbtdb->prunenodes); + rbtdb->active = rbtdb->node_lock_count; for (i = 0; i < (int)(rbtdb->node_lock_count); i++) { diff --git a/lib/dns/rbtdb_p.h b/lib/dns/rbtdb_p.h index 1aa5672533..84ab5ec4d8 100644 --- a/lib/dns/rbtdb_p.h +++ b/lib/dns/rbtdb_p.h @@ -296,6 +296,10 @@ struct dns_rbtdb { */ dns_rbtnodelist_t *deadnodes; + /* List of nodes from which recursive tree pruning can be started from. + * Locked by tree_lock. */ + dns_rbtnodelist_t prunenodes; + /* * Heaps. These are used for TTL based expiry in a cache, * or for zone resigning in a zone DB. hmctx is the memory @@ -342,14 +346,6 @@ typedef struct { isc_stdtime_t now; } rbtdb_load_t; -/*% - * Prune context - */ -typedef struct { - dns_db_t *db; - dns_rbtnode_t *node; -} prune_t; - extern dns_dbmethods_t dns__rbtdb_zonemethods; extern dns_dbmethods_t dns__rbtdb_cachemethods; From 04df558d57105ccfb9853ba892021c9de01d30c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 4 Jan 2024 13:39:27 +0100 Subject: [PATCH 12/25] Add CHANGES entry for GL #4383 --- CHANGES | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 548208b3ef..2df84ce57a 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,5 @@ -6319. [placeholder] +6319. [func] Limit isc_async_run() overhead for RBTDB tree pruning. + [GL #4383] 6318. [placeholder] From 8cad2c59237d99ea8854d598c45c2f71b9edc1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 5 Jan 2024 12:51:13 +0100 Subject: [PATCH 13/25] Prepare release notes for BIND 9.19.20 --- doc/arm/notes.rst | 2 +- doc/notes/{notes-current.rst => notes-9.19.20.rst} | 12 ------------ 2 files changed, 1 insertion(+), 13 deletions(-) rename doc/notes/{notes-current.rst => notes-9.19.20.rst} (95%) diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index fec0cd43ed..e3c8787b93 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -38,7 +38,7 @@ information about each release, and source code. .. include:: ../notes/notes-known-issues.rst -.. include:: ../notes/notes-current.rst +.. include:: ../notes/notes-9.19.20.rst .. include:: ../notes/notes-9.19.19.rst .. include:: ../notes/notes-9.19.18.rst .. include:: ../notes/notes-9.19.17.rst diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-9.19.20.rst similarity index 95% rename from doc/notes/notes-current.rst rename to doc/notes/notes-9.19.20.rst index 0021e57c5f..78c7b53e05 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-9.19.20.rst @@ -32,21 +32,9 @@ Security Fixes features were enabled. This has been fixed. :cve:`2023-5679` :gl:`#4334` -New Features -~~~~~~~~~~~~ - -- None. - -Removed Features -~~~~~~~~~~~~~~~~ - -- None. - Feature Changes ~~~~~~~~~~~~~~~ -- None. - - :program:`named-compilezone` no longer performs zone integrity checks by default; this allows faster conversion of a zone file from one format to another. Zone checks can be performed by running :program:`named-checkzone` From 1708fe24b46072bbb00dcd73956f681c1e88aa9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 5 Jan 2024 12:51:13 +0100 Subject: [PATCH 14/25] Tweak and reword release notes --- doc/notes/notes-9.19.20.rst | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/doc/notes/notes-9.19.20.rst b/doc/notes/notes-9.19.20.rst index 78c7b53e05..33aec1d0e5 100644 --- a/doc/notes/notes-9.19.20.rst +++ b/doc/notes/notes-9.19.20.rst @@ -35,19 +35,23 @@ Security Fixes Feature Changes ~~~~~~~~~~~~~~~ -- :program:`named-compilezone` no longer performs zone integrity checks - by default; this allows faster conversion of a zone file from one format - to another. Zone checks can be performed by running :program:`named-checkzone` - separately, or the previous default behavior can be restored by using - ``named-compilezone -i full -k fail -n fail -r warn -m warn -M warn - -S warn -T warn -W warn -C check-svcb:fail``. :gl:`#4364` +- :iscman:`named-compilezone` no longer performs zone integrity checks + by default; this allows faster conversion of a zone file from one + format to another. :gl:`#4364` + + Zone checks can be performed by running :iscman:`named-checkzone` + separately, or the previous default behavior can be restored by using: + + :: + + named-compilezone -i full -k fail -n fail -r warn -m warn -M warn -S warn -T warn -W warn -C check-svcb:fail Bug Fixes ~~~~~~~~~ -- Fix statistics export to use full 64 bit signed numbers instead of truncating - values to unsigned 32 bits. Export was truncating values since BIND 9.15.0. - :gl:`#4467` +- The counters exported via the statistics channel were changed back to + 64-bit signed values; they were being inadvertently truncated to + unsigned 32-bit values since BIND 9.15.0. :gl:`#4467` Known Issues ~~~~~~~~~~~~ From 0fec404c64cf367b9c2d676535c50f228355b6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 5 Jan 2024 12:51:13 +0100 Subject: [PATCH 15/25] Fix Danger rules for flagging release note issues The logic contained in dangerfile.py incorrectly warns about missing release note changes for merge requests preparing release documentation as such merge requests rename files in the doc/notes/ directory. This (correctly) causes these files to be passed to dangerfile.py via danger.git.created_files and danger.git.deleted_files rather than via danger.git.modified_files, which in turn causes the logic checking the use of the "Release Notes" label to assume that no release notes are added, removed, or modified by a given merge request. Fix by considering all types of file changes (modifications, additions, and removals - which also covers file renaming) when checking whether a given merge request modifies release notes. Update the warning messages accordingly. However, when trying to find release notes added by a given merge request, deleted files must not be considered. Tweak the logic looking for GitLab identifiers in the release notes added by a given merge request so that it only scans modified and added (or renamed) files. --- dangerfile.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 2c1e7de14c..d3d3a8f4c6 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -347,18 +347,18 @@ if changes_added_lines: # MR. release_notes_regex = re.compile(r"doc/(arm|notes)/notes-.*\.(rst|xml)") -release_notes_changed = list(filter(release_notes_regex.match, modified_files)) +release_notes_changed = list(filter(release_notes_regex.match, affected_files)) release_notes_label_set = "Release Notes" in mr_labels if not release_notes_changed: if release_notes_label_set: fail( "This merge request has the *Release Notes* label set. " - "Add a release note or unset the *Release Notes* label." + "Update release notes or unset the *Release Notes* label." ) elif "Customer" in mr_labels: warn( "This merge request has the *Customer* label set. " - "Add a release note unless the changes introduced are trivial." + "Update release notes unless the changes introduced are trivial." ) if release_notes_changed and not release_notes_label_set: fail( @@ -367,7 +367,9 @@ if release_notes_changed and not release_notes_label_set: ) if release_notes_changed: - notes_added_lines = added_lines(target_branch, release_notes_changed) + modified_or_new_files = danger.git.modified_files + danger.git.created_files + release_notes_added = list(filter(release_notes_regex.match, modified_or_new_files)) + notes_added_lines = added_lines(target_branch, release_notes_added) identifiers_found = filter(relnotes_issue_or_mr_id_regex.search, notes_added_lines) if notes_added_lines and not any(identifiers_found): warn("No valid issue/MR identifiers found in added release notes.") From b0b4a6f58cb2b792e5d4693254317e36c12e60d9 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 5 Jan 2024 14:19:30 +0100 Subject: [PATCH 16/25] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 2df84ce57a..3a1f47440c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.19.20 released --- + 6319. [func] Limit isc_async_run() overhead for RBTDB tree pruning. [GL #4383] From aca85323d604f036a8285d9f7289424665422ad4 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 5 Jan 2024 14:20:36 +0100 Subject: [PATCH 17/25] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index bd01fce7cb..ae7d41159b 100644 --- a/configure.ac +++ b/configure.ac @@ -17,7 +17,7 @@ m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 19)dnl m4_define([bind_VERSION_PATCH], 20)dnl -m4_define([bind_VERSION_EXTRA], -dev)dnl +m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Development Release)])dnl m4_define([bind_SRCID], [m4_esyscmd_s([git rev-parse --short HEAD | cut -b1-7])])dnl m4_define([bind_PKG_VERSION], [[bind_VERSION_MAJOR.bind_VERSION_MINOR.bind_VERSION_PATCH]bind_VERSION_EXTRA])dnl From 4c19d35614f8cd80d8748156a5bad361e19abc28 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 16 Jan 2024 15:58:53 -0800 Subject: [PATCH 18/25] fix a message parsing regression the fix for CVE-2023-4408 introduced a regression in the message parser, which could cause a crash if duplicate rdatasets were found in the question section. this commit ensures that rdatasets are correctly disassociated and freed when this occurs. --- lib/dns/message.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/dns/message.c b/lib/dns/message.c index 8280aa287f..b0d5f16da7 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -1130,6 +1130,9 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, cleanup: if (rdataset != NULL) { + if (dns_rdataset_isassociated(rdataset)) { + dns_rdataset_disassociate(rdataset); + } dns_message_puttemprdataset(msg, &rdataset); } From 510f1de8a6add516b842a55750366944701d3d9a Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Fri, 19 Jan 2024 14:26:17 +0000 Subject: [PATCH 19/25] fix another message parsing regression The fix for CVE-2023-4408 introduced a regression in the message parser, which could cause a crash if an rdata type that can only occur in the question was found in another section. Use 'dns__message_putassociatedrdataset()' instead of 'dns__message_puttemprdataset()', because after calling the 'dns_rdatalist_tordataset()' function earlier the 'rdataset' is associated. --- lib/dns/message.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/dns/message.c b/lib/dns/message.c index b0d5f16da7..b301819480 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -1250,6 +1250,7 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, isedns = false; issigzero = false; istsig = false; + found_rdataset = NULL; name = NULL; dns_message_gettempname(msg, &name); @@ -1394,10 +1395,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, * Then put the meta-class back into the finished rdata. */ rdata = newrdata(msg); - if (rdata == NULL) { - result = ISC_R_NOMEMORY; - goto cleanup; - } if (msg->opcode == dns_opcode_update && update(sectionid, rdclass)) { @@ -1549,7 +1546,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, * the question section, fail. */ if (dns_rdatatype_questiononly(rdtype)) { - dns_message_puttemprdataset(msg, &rdataset); DO_ERROR(DNS_R_FORMERR); } @@ -1600,18 +1596,17 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, /* Free the rdataset we used as the key */ dns__message_putassociatedrdataset(msg, &rdataset); - rdataset = found_rdataset; - result = ISC_R_SUCCESS; + rdataset = found_rdataset; if (!dns_rdatatype_issingleton(rdtype)) { break; } - dns_rdata_t *first; dns_rdatalist_fromrdataset(rdataset, &rdatalist); - first = ISC_LIST_HEAD(rdatalist->rdata); + dns_rdata_t *first = + ISC_LIST_HEAD(rdatalist->rdata); INSIST(first != NULL); if (dns_rdata_compare(rdata, first) != 0) { DO_ERROR(DNS_R_FORMERR); @@ -1656,7 +1651,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, dns_rcode_t ercode; msg->opt = rdataset; - rdataset = NULL; ercode = (dns_rcode_t)((msg->opt->ttl & DNS_MESSAGE_EDNSRCODE_MASK) >> 20); @@ -1667,7 +1661,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, msg->sig0 = rdataset; msg->sig0name = name; msg->sigstart = recstart; - rdataset = NULL; free_name = false; } else if (istsig) { msg->tsig = rdataset; @@ -1677,9 +1670,9 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, * Windows doesn't like TSIG names to be compressed. */ msg->tsigname->attributes.nocompress = true; - rdataset = NULL; free_name = false; } + rdataset = NULL; if (seen_problem) { if (free_name) { @@ -1688,8 +1681,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, free_name = false; } INSIST(!free_name); - - rdataset = NULL; } /* @@ -1711,6 +1702,9 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, } cleanup: + if (rdataset != NULL && rdataset != found_rdataset) { + dns__message_putassociatedrdataset(msg, &rdataset); + } if (free_name) { dns_message_puttempname(msg, &name); } From 04ba284e1ace177d095599c8708126093a91336d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 31 Jan 2024 11:52:19 +0100 Subject: [PATCH 20/25] Add a CHANGES entry --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 3a1f47440c..095d027bb5 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6321. [security] Change 6315 inadvertently introduced regressions that + could cause named to crash. [GL #4234] + --- 9.19.20 released --- 6319. [func] Limit isc_async_run() overhead for RBTDB tree pruning. From 15096aefdf544d8f71c4d1ad84f4b8558c8756ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 11 Dec 2023 16:50:12 +0100 Subject: [PATCH 21/25] Make the dns_validator validations asynchronous and limit it Instead of running all the cryptographic validation in a tight loop, spread it out into multiple event loop "ticks", but moving every single validation into own isc_async_run() asynchronous event. Move the cryptographic operations - both verification and DNSKEY selection - to the offloaded threads (isc_work_enqueue), this further limits the time we spend doing expensive operations on the event loops that should be fast. Limit the impact of invalid or malicious RRSets that contain crafted records causing the dns_validator to do many validations per single fetch by adding a cap on the maximum number of validations and maximum number of validation failures that can happen before the resolving fails. --- bin/named/server.c | 15 + doc/arm/reference.rst | 15 + doc/misc/options | 4 + lib/dns/dst_api.c | 27 +- lib/dns/include/dns/resolver.h | 8 + lib/dns/include/dns/validator.h | 9 + lib/dns/include/dst/dst.h | 4 + lib/dns/resolver.c | 35 +- lib/dns/validator.c | 1277 +++++++++++++++++++------------ lib/isc/loop.c | 2 + lib/isccfg/namedconf.c | 4 + 11 files changed, 901 insertions(+), 499 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 80c5b5f1bb..67cafe633f 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -5455,6 +5455,21 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, INSIST(result == ISC_R_SUCCESS); dns_resolver_setmaxqueries(view->resolver, cfg_obj_asuint32(obj)); + obj = NULL; + result = named_config_get(maps, "max-validations-per-fetch", &obj); + if (result == ISC_R_SUCCESS) { + dns_resolver_setmaxvalidations(view->resolver, + cfg_obj_asuint32(obj)); + } + + obj = NULL; + result = named_config_get(maps, "max-validation-failures-per-fetch", + &obj); + if (result == ISC_R_SUCCESS) { + dns_resolver_setmaxvalidationfails(view->resolver, + cfg_obj_asuint32(obj)); + } + obj = NULL; result = named_config_get(maps, "fetches-per-zone", &obj); INSIST(result == ISC_R_SUCCESS); diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 8468a785ea..6fb5937fa2 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -3691,6 +3691,21 @@ system. set to zero, :any:`max-clients-per-query` no longer applies and there is no upper bound, other than that imposed by :any:`recursive-clients`. +.. namedconf:statement:: max-validations-per-fetch + :tags: server + :short: Set the maximum number of DNSSEC validations that can happen in single fetch + + This is an **experimental** setting to set the maximum number of DNSSEC + validations that can happen in a single resolver fetch. The default is 16. + +.. namedconf:statement:: max-validation-failures-per-fetch + :tags: server + :short: Set the maximum number of DNSSEC validation failures that can happen in single fetch + + This is an **experimental** setting to set the maximum number of DNSSEC + validation failures that can happen in a single resolver fetch. The default + is 1. + .. namedconf:statement:: fetches-per-zone :tags: server, query :short: Sets the maximum number of simultaneous iterative queries allowed to any one domain before the server blocks new queries for data in or beneath that zone. diff --git a/doc/misc/options b/doc/misc/options index ac5dd66794..5fe4f32685 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -188,6 +188,8 @@ options { max-transfer-time-in ; max-transfer-time-out ; max-udp-size ; + max-validation-failures-per-fetch ; // experimental + max-validations-per-fetch ; // experimental max-zone-ttl ( unlimited | ); // deprecated memstatistics ; memstatistics-file ; @@ -469,6 +471,8 @@ view [ ] { max-transfer-time-in ; max-transfer-time-out ; max-udp-size ; + max-validation-failures-per-fetch ; // experimental + max-validations-per-fetch ; // experimental max-zone-ttl ( unlimited | ); // deprecated message-compression ; min-cache-ttl ; diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c index af53947ac6..ce41b99e5d 100644 --- a/lib/dns/dst_api.c +++ b/lib/dns/dst_api.c @@ -164,7 +164,8 @@ computeid(dst_key_t *key); static isc_result_t frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, unsigned int protocol, dns_rdataclass_t rdclass, - isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp); + isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, + dst_key_t **keyp); static isc_result_t algorithm_status(unsigned int alg); @@ -750,6 +751,13 @@ dst_key_todns(const dst_key_t *key, isc_buffer_t *target) { isc_result_t dst_key_fromdns(const dns_name_t *name, dns_rdataclass_t rdclass, isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp) { + return (dst_key_fromdns_ex(name, rdclass, source, mctx, false, keyp)); +} + +isc_result_t +dst_key_fromdns_ex(const dns_name_t *name, dns_rdataclass_t rdclass, + isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, + dst_key_t **keyp) { uint8_t alg, proto; uint32_t flags, extflags; dst_key_t *key = NULL; @@ -780,7 +788,7 @@ dst_key_fromdns(const dns_name_t *name, dns_rdataclass_t rdclass, } result = frombuffer(name, alg, flags, proto, rdclass, source, mctx, - &key); + no_rdata, &key); if (result != ISC_R_SUCCESS) { return (result); } @@ -801,7 +809,7 @@ dst_key_frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, REQUIRE(dst_initialized); result = frombuffer(name, alg, flags, protocol, rdclass, source, mctx, - &key); + false, &key); if (result != ISC_R_SUCCESS) { return (result); } @@ -2302,7 +2310,8 @@ computeid(dst_key_t *key) { static isc_result_t frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, unsigned int protocol, dns_rdataclass_t rdclass, - isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp) { + isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, + dst_key_t **keyp) { dst_key_t *key; isc_result_t ret; @@ -2324,10 +2333,12 @@ frombuffer(const dns_name_t *name, unsigned int alg, unsigned int flags, return (DST_R_UNSUPPORTEDALG); } - ret = key->func->fromdns(key, source); - if (ret != ISC_R_SUCCESS) { - dst_key_free(&key); - return (ret); + if (!no_rdata) { + ret = key->func->fromdns(key, source); + if (ret != ISC_R_SUCCESS) { + dst_key_free(&key); + return (ret); + } } } diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h index e9258827e4..7f0dde65a5 100644 --- a/lib/dns/include/dns/resolver.h +++ b/lib/dns/include/dns/resolver.h @@ -578,6 +578,14 @@ dns_resolver_printbadcache(dns_resolver_t *resolver, FILE *fp); * \li resolver to be valid. */ +void +dns_resolver_setmaxvalidations(dns_resolver_t *resolver, uint32_t max); +void +dns_resolver_setmaxvalidationfails(dns_resolver_t *resolver, uint32_t max); +/*% + * Set maximum numbers of validations and maximum validation failures per fetch. + */ + void dns_resolver_setmaxdepth(dns_resolver_t *resolver, unsigned int maxdepth); unsigned int diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index d42cdca637..0b0222c5c6 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -53,6 +53,7 @@ #include #include +#include #include #include /* for dns_rdata_rrsig_t */ #include @@ -144,6 +145,13 @@ struct dns_validator { unsigned int authcount; unsigned int authfail; isc_stdtime_t start; + + bool digest_sha1; + bool supported_algorithm; + dns_rdata_t rdata; + bool resume; + uint32_t *nvalidations; + uint32_t *nfails; }; /*% @@ -161,6 +169,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, dns_message_t *message, unsigned int options, isc_loop_t *loop, isc_job_cb cb, void *arg, + uint32_t *nvalidations, uint32_t *nfails, dns_validator_t **validatorp); /*%< * Start a DNSSEC validation. diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h index c0912f3e67..6c7a4e50db 100644 --- a/lib/dns/include/dst/dst.h +++ b/lib/dns/include/dst/dst.h @@ -482,6 +482,10 @@ dst_key_tofile(const dst_key_t *key, int type, const char *directory); */ isc_result_t +dst_key_fromdns_ex(const dns_name_t *name, dns_rdataclass_t rdclass, + isc_buffer_t *source, isc_mem_t *mctx, bool no_rdata, + dst_key_t **keyp); +isc_result_t dst_key_fromdns(const dns_name_t *name, dns_rdataclass_t rdclass, isc_buffer_t *source, isc_mem_t *mctx, dst_key_t **keyp); /*%< diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index f0f48d990d..442dfabce0 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -178,6 +178,16 @@ */ #define MINIMUM_QUERY_TIMEOUT (MAX_SINGLE_QUERY_TIMEOUT + 1000U) +/* + * The default maximum number of validations and validation failures per-fetch + */ +#ifndef DEFAULT_MAX_VALIDATIONS +#define DEFAULT_MAX_VALIDATIONS 16 +#endif +#ifndef DEFAULT_MAX_VALIDATION_FAILURES +#define DEFAULT_MAX_VALIDATION_FAILURES 1 +#endif + /* The default time in seconds for the whole query to live. */ #ifndef DEFAULT_QUERY_TIMEOUT #define DEFAULT_QUERY_TIMEOUT MINIMUM_QUERY_TIMEOUT @@ -457,6 +467,9 @@ struct fetchctx { dns_adbaddrinfo_t *addrinfo; unsigned int depth; char clientstr[ISC_SOCKADDR_FORMATSIZE]; + + uint32_t nvalidations; + uint32_t nfails; }; #define FCTX_MAGIC ISC_MAGIC('F', '!', '!', '!') @@ -567,6 +580,9 @@ struct dns_resolver { atomic_bool exiting; atomic_bool priming; + atomic_uint_fast32_t maxvalidations; + atomic_uint_fast32_t maxvalidationfails; + /* Locked by lock. */ unsigned int spillat; /* clients-per-query */ @@ -961,7 +977,8 @@ valcreate(fetchctx_t *fctx, dns_message_t *message, dns_adbaddrinfo_t *addrinfo, result = dns_validator_create( fctx->res->view, name, type, rdataset, sigrdataset, message, - valoptions, fctx->loop, validated, valarg, &validator); + valoptions, fctx->loop, validated, valarg, &fctx->nvalidations, + &fctx->nfails, &validator); RUNTIME_CHECK(result == ISC_R_SUCCESS); inc_stats(fctx->res, dns_resstatscounter_val); if ((valoptions & DNS_VALIDATOR_DEFER) == 0) { @@ -4518,6 +4535,8 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, .fwdpolicy = dns_fwdpolicy_none, .result = ISC_R_FAILURE, .loop = loop, + .nvalidations = atomic_load_relaxed(&res->maxvalidations), + .nfails = atomic_load_relaxed(&res->maxvalidationfails), }; isc_mem_attach(mctx, &fctx->mctx); @@ -9960,6 +9979,8 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr, isc_nm_t *nm, .maxqueries = DEFAULT_MAX_QUERIES, .alternates = ISC_LIST_INITIALIZER, .nloops = isc_loopmgr_nloops(loopmgr), + .maxvalidations = DEFAULT_MAX_VALIDATIONS, + .maxvalidationfails = DEFAULT_MAX_VALIDATION_FAILURES, }; RTRACE("create"); @@ -10925,6 +10946,18 @@ dns_resolver_settimeout(dns_resolver_t *resolver, unsigned int timeout) { resolver->query_timeout = timeout; } +void +dns_resolver_setmaxvalidations(dns_resolver_t *resolver, uint32_t max) { + REQUIRE(VALID_RESOLVER(resolver)); + atomic_store(&resolver->maxvalidations, max); +} + +void +dns_resolver_setmaxvalidationfails(dns_resolver_t *resolver, uint32_t max) { + REQUIRE(VALID_RESOLVER(resolver)); + atomic_store(&resolver->maxvalidationfails, max); +} + void dns_resolver_setmaxdepth(dns_resolver_t *resolver, unsigned int maxdepth) { REQUIRE(VALID_RESOLVER(resolver)); diff --git a/lib/dns/validator.c b/lib/dns/validator.c index b679392c49..9afd2ea11f 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -55,7 +56,7 @@ * validator_start -> proveunsecure * * \li When called with no rdataset or sigrdataset: - * validator_start -> validate_nx-> proveunsecure + * validator_start -> validate_nx -> proveunsecure * * validator_start: determine what type of validation to do. * validate_answer: attempt to perform a positive validation. @@ -66,29 +67,32 @@ #define VALIDATOR_MAGIC ISC_MAGIC('V', 'a', 'l', '?') #define VALID_VALIDATOR(v) ISC_MAGIC_VALID(v, VALIDATOR_MAGIC) -#define VALATTR_CANCELED 0x0002 /*%< Canceled. */ -#define VALATTR_TRIEDVERIFY \ - 0x0004 /*%< We have found a key and \ - * have attempted a verify. */ -#define VALATTR_COMPLETE 0x0008 /*%< Completion event sent. */ -#define VALATTR_INSECURITY 0x0010 /*%< Attempting proveunsecure. */ +enum valattr { + VALATTR_CANCELED = 1 << 1, /*%< Canceled. */ + VALATTR_TRIEDVERIFY = 1 << 2, /*%< We have found a key and have + attempted a verify. */ + VALATTR_COMPLETE = 1 << 3, /*%< Completion event sent. */ + VALATTR_INSECURITY = 1 << 4, /*%< Attempting proveunsecure. */ + VALATTR_MAXVALIDATIONS = 1 << 5, /*%< Max validations quota */ + VALATTR_MAXVALIDATIONFAILS = 1 << 6, /*%< Max validation fails quota */ -/*! - * NSEC proofs to be looked for. - */ -#define VALATTR_NEEDNOQNAME 0x00000100 -#define VALATTR_NEEDNOWILDCARD 0x00000200 -#define VALATTR_NEEDNODATA 0x00000400 + /*! + * NSEC proofs to be looked for. + */ + VALATTR_NEEDNOQNAME = 1 << 8, + VALATTR_NEEDNOWILDCARD = 1 << 9, + VALATTR_NEEDNODATA = 1 << 10, -/*! - * NSEC proofs that have been found. - */ -#define VALATTR_FOUNDNOQNAME 0x00001000 -#define VALATTR_FOUNDNOWILDCARD 0x00002000 -#define VALATTR_FOUNDNODATA 0x00004000 -#define VALATTR_FOUNDCLOSEST 0x00008000 -#define VALATTR_FOUNDOPTOUT 0x00010000 -#define VALATTR_FOUNDUNKNOWN 0x00020000 + /*! + * NSEC proofs that have been found. + */ + VALATTR_FOUNDNOQNAME = 1 << 12, + VALATTR_FOUNDNOWILDCARD = 1 << 13, + VALATTR_FOUNDNODATA = 1 << 14, + VALATTR_FOUNDCLOSEST = 1 << 15, + VALATTR_FOUNDOPTOUT = 1 << 16, + VALATTR_FOUNDUNKNOWN = 1 << 17, +}; #define NEEDNODATA(val) ((val->attributes & VALATTR_NEEDNODATA) != 0) #define NEEDNOQNAME(val) ((val->attributes & VALATTR_NEEDNOQNAME) != 0) @@ -105,17 +109,27 @@ #define NEGATIVE(r) (((r)->attributes & DNS_RDATASETATTR_NEGATIVE) != 0) #define NXDOMAIN(r) (((r)->attributes & DNS_RDATASETATTR_NXDOMAIN) != 0) +#define MAXVALIDATIONS(r) (((r)->attributes & VALATTR_MAXVALIDATIONS) != 0) +#define MAXVALIDATIONFAILS(r) \ + (((r)->attributes & VALATTR_MAXVALIDATIONFAILS) != 0) + static void destroy_validator(dns_validator_t *val); static isc_result_t select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset); +static void +resume_answer(void *arg); +static void +validate_async_done(dns_validator_t *val, isc_result_t result); static isc_result_t -validate_answer(dns_validator_t *val, bool resume); +validate_async_run(dns_validator_t *val, isc_job_cb cb); -static isc_result_t -validate_dnskey(dns_validator_t *val); +static void +validate_dnskey(void *arg); +static void +validate_dnskey_dsset_done(dns_validator_t *val, isc_result_t result); static isc_result_t validate_nx(dns_validator_t *val, bool resume); @@ -204,18 +218,9 @@ marksecure(dns_validator_t *val) { val->secure = true; } -static void -validator_done_cb(void *arg) { - dns_validator_t *val = arg; - val->cb(val); - dns_validator_detach(&val); -} - /* * Validator 'val' is finished; send the completion event to the loop * that called dns_validator_create(), with result `result`. - * - * Caller must be holding the validator lock. */ static void validator_done(dns_validator_t *val, isc_result_t result) { @@ -226,8 +231,7 @@ validator_done(dns_validator_t *val, isc_result_t result) { val->attributes |= VALATTR_COMPLETE; val->result = result; - dns_validator_ref(val); - isc_async_run(val->loop, validator_done_cb, val); + isc_async_run(val->loop, val->cb, val); } /*% @@ -349,6 +353,24 @@ trynsec3: return (found); } +static void +resume_answer_with_key(void *arg) { + dns_validator_t *val = arg; + dns_rdataset_t *rdataset = &val->frdataset; + + isc_result_t result = select_signing_key(val, rdataset); + if (result == ISC_R_SUCCESS) { + val->keyset = &val->frdataset; + } +} + +static void +resume_answer_with_key_done(void *arg) { + dns_validator_t *val = arg; + + resume_answer(val); +} + /*% * We have been asked to look for a key. * If found, resume the validation process. @@ -361,8 +383,6 @@ fetch_callback_dnskey(void *arg) { dns_rdataset_t *rdataset = &val->frdataset; isc_result_t eresult = resp->result; isc_result_t result; - isc_result_t saved_result; - dns_fetch_t *fetch = NULL; INSIST(resp->type == FETCHDONE); @@ -376,14 +396,18 @@ fetch_callback_dnskey(void *arg) { if (dns_rdataset_isassociated(&val->fsigrdataset)) { dns_rdataset_disassociate(&val->fsigrdataset); } - isc_mem_putanddetach(&resp->mctx, resp, sizeof(*resp)); validator_log(val, ISC_LOG_DEBUG(3), "in fetch_callback_dnskey"); - fetch = val->fetch; - val->fetch = NULL; + dns_resolver_destroyfetch(&val->fetch); + if (CANCELED(val)) { - validator_done(val, ISC_R_CANCELED); - } else if (eresult == ISC_R_SUCCESS || eresult == DNS_R_NCACHENXRRSET) { + result = ISC_R_CANCELED; + goto cleanup; + } + + switch (eresult) { + case ISC_R_SUCCESS: + case DNS_R_NCACHENXRRSET: /* * We have an answer to our DNSKEY query. Either the DNSKEY * RRset or a NODATA response. @@ -398,41 +422,23 @@ fetch_callback_dnskey(void *arg) { if (eresult == ISC_R_SUCCESS && rdataset->trust >= dns_trust_secure) { - result = select_signing_key(val, rdataset); - if (result == ISC_R_SUCCESS) { - val->keyset = &val->frdataset; - } + isc_work_enqueue(val->loop, resume_answer_with_key, + resume_answer, val); + result = DNS_R_WAIT; + } else { + result = validate_async_run(val, resume_answer); } - result = validate_answer(val, true); - if (result == DNS_R_NOVALIDSIG && - (val->attributes & VALATTR_TRIEDVERIFY) == 0) - { - saved_result = result; - validator_log(val, ISC_LOG_DEBUG(3), - "falling back to insecurity proof"); - result = proveunsecure(val, false, false); - if (result == DNS_R_NOTINSECURE) { - result = saved_result; - } - } - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - } else { + break; + default: validator_log(val, ISC_LOG_DEBUG(3), "fetch_callback_dnskey: got %s", isc_result_totext(eresult)); - if (eresult == ISC_R_CANCELED) { - validator_done(val, eresult); - } else { - validator_done(val, DNS_R_BROKENCHAIN); - } - } - - if (fetch != NULL) { - dns_resolver_destroyfetch(&fetch); + result = DNS_R_BROKENCHAIN; } +cleanup: + isc_mem_putanddetach(&resp->mctx, resp, sizeof(*resp)); + validate_async_done(val, result); dns_validator_detach(&val); } @@ -447,7 +453,6 @@ fetch_callback_ds(void *arg) { dns_rdataset_t *rdataset = &val->frdataset; isc_result_t eresult = resp->result; isc_result_t result; - dns_fetch_t *fetch = NULL; bool trustchain; INSIST(resp->type == FETCHDONE); @@ -470,28 +475,16 @@ fetch_callback_ds(void *arg) { } validator_log(val, ISC_LOG_DEBUG(3), "in fetch_callback_ds"); - fetch = val->fetch; - val->fetch = NULL; + dns_resolver_destroyfetch(&val->fetch); if (CANCELED(val)) { - validator_done(val, ISC_R_CANCELED); - goto done; + result = ISC_R_CANCELED; + goto cleanup; } - switch (eresult) { - case DNS_R_NXDOMAIN: - case DNS_R_NCACHENXDOMAIN: - /* - * These results only make sense if we're attempting - * an insecurity proof, not when walking a chain of trust. - */ - if (trustchain) { - goto unexpected; - } - - FALLTHROUGH; - case ISC_R_SUCCESS: - if (trustchain) { + if (trustchain) { + switch (eresult) { + case ISC_R_SUCCESS: /* * We looked for a DS record as part of * following a key chain upwards; resume following @@ -501,29 +494,12 @@ fetch_callback_ds(void *arg) { "dsset with trust %s", dns_trust_totext(rdataset->trust)); val->dsset = &val->frdataset; - result = validate_dnskey(val); - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - } else { - /* - * There is a DS which may or may not be a zone cut. - * In either case we are still in a secure zone, - * so keep looking for the break in the chain - * of trust. - */ - result = proveunsecure(val, (eresult == ISC_R_SUCCESS), - true); - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - } - break; - case DNS_R_CNAME: - case DNS_R_NXRRSET: - case DNS_R_NCACHENXRRSET: - case DNS_R_SERVFAIL: /* RFC 1034 parent? */ - if (trustchain) { + result = validate_async_run(val, validate_dnskey); + break; + case DNS_R_CNAME: + case DNS_R_NXRRSET: + case DNS_R_NCACHENXRRSET: + case DNS_R_SERVFAIL: /* RFC 1034 parent? */ /* * Failed to find a DS while following the * chain of trust; now we need to prove insecurity. @@ -532,54 +508,69 @@ fetch_callback_ds(void *arg) { "falling back to insecurity proof (%s)", isc_result_totext(eresult)); result = proveunsecure(val, false, false); - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - } else if (eresult == DNS_R_SERVFAIL) { - goto unexpected; - } else if (eresult != DNS_R_CNAME && - isdelegation(resp->foundname, &val->frdataset, - eresult)) - { + break; + default: + validator_log(val, ISC_LOG_DEBUG(3), + "fetch_callback_ds: got %s", + isc_result_totext(eresult)); + result = DNS_R_BROKENCHAIN; + break; + } + } else { + switch (eresult) { + case DNS_R_NXDOMAIN: + case DNS_R_NCACHENXDOMAIN: /* - * Failed to find a DS while trying to prove - * insecurity. If this is a zone cut, that - * means we're insecure. + * These results only make sense if we're attempting + * an insecurity proof, not when walking a chain of + * trust. */ - result = markanswer(val, "fetch_callback_ds", - "no DS and this is a delegation"); - validator_done(val, result); - } else { + + result = proveunsecure(val, false, true); + break; + case ISC_R_SUCCESS: + /* + * There is a DS which may or may not be a zone cut. + * In either case we are still in a secure zone, + * so keep looking for the break in the chain + * of trust. + */ + result = proveunsecure(val, true, true); + break; + case DNS_R_NXRRSET: + case DNS_R_NCACHENXRRSET: + if (isdelegation(resp->foundname, &val->frdataset, + eresult)) + { + /* + * Failed to find a DS while trying to prove + * insecurity. If this is a zone cut, that + * means we're insecure. + */ + result = markanswer( + val, "fetch_callback_ds", + "no DS and this is a delegation"); + break; + } + FALLTHROUGH; + case DNS_R_CNAME: /* * Not a zone cut, so we have to keep looking for * the break point in the chain of trust. */ result = proveunsecure(val, false, true); - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - } - break; - - default: - unexpected: - validator_log(val, ISC_LOG_DEBUG(3), - "fetch_callback_ds: got %s", - isc_result_totext(eresult)); - if (eresult == ISC_R_CANCELED) { - validator_done(val, eresult); - } else { - validator_done(val, DNS_R_BROKENCHAIN); + break; + default: + validator_log(val, ISC_LOG_DEBUG(3), + "fetch_callback_ds: got %s", + isc_result_totext(eresult)); + result = DNS_R_BROKENCHAIN; } } -done: +cleanup: isc_mem_putanddetach(&resp->mctx, resp, sizeof(*resp)); - - if (fetch != NULL) { - dns_resolver_destroyfetch(&fetch); - } - + validate_async_done(val, result); dns_validator_detach(&val); } @@ -592,52 +583,43 @@ static void validator_callback_dnskey(void *arg) { dns_validator_t *subvalidator = (dns_validator_t *)arg; dns_validator_t *val = subvalidator->parent; - isc_result_t result; - isc_result_t eresult = subvalidator->result; - isc_result_t saved_result; + isc_result_t result = subvalidator->result; val->subvalidator = NULL; - subvalidator->parent = NULL; + + if (CANCELED(val)) { + result = ISC_R_CANCELED; + goto cleanup; + } validator_log(val, ISC_LOG_DEBUG(3), "in validator_callback_dnskey"); - if (CANCELED(val)) { - validator_done(val, ISC_R_CANCELED); - } else if (eresult == ISC_R_SUCCESS) { + if (result == ISC_R_SUCCESS) { validator_log(val, ISC_LOG_DEBUG(3), "keyset with trust %s", dns_trust_totext(val->frdataset.trust)); /* * Only extract the dst key if the keyset is secure. */ if (val->frdataset.trust >= dns_trust_secure) { - (void)select_signing_key(val, &val->frdataset); - } - result = validate_answer(val, true); - if (result == DNS_R_NOVALIDSIG && - (val->attributes & VALATTR_TRIEDVERIFY) == 0) - { - saved_result = result; - validator_log(val, ISC_LOG_DEBUG(3), - "falling back to insecurity proof"); - result = proveunsecure(val, false, false); - if (result == DNS_R_NOTINSECURE) { - result = saved_result; - } - } - if (result != DNS_R_WAIT) { - validator_done(val, result); + isc_work_enqueue(val->loop, resume_answer_with_key, + resume_answer_with_key_done, val); + result = DNS_R_WAIT; + } else { + result = validate_async_run(val, resume_answer); } } else { - if (eresult != DNS_R_BROKENCHAIN) { + if (result != DNS_R_BROKENCHAIN) { expire_rdatasets(val); } validator_log(val, ISC_LOG_DEBUG(3), "validator_callback_dnskey: got %s", - isc_result_totext(eresult)); - validator_done(val, DNS_R_BROKENCHAIN); + isc_result_totext(result)); + result = DNS_R_BROKENCHAIN; } +cleanup: + dns_validator_detach(&subvalidator->parent); dns_validator_destroy(&subvalidator); - dns_validator_detach(&val); + validate_async_done(val, result); } /*% @@ -653,12 +635,14 @@ validator_callback_ds(void *arg) { isc_result_t eresult = subvalidator->result; val->subvalidator = NULL; - subvalidator->parent = NULL; + + if (CANCELED(val)) { + result = ISC_R_CANCELED; + goto cleanup; + } validator_log(val, ISC_LOG_DEBUG(3), "in validator_callback_ds"); - if (CANCELED(val)) { - validator_done(val, ISC_R_CANCELED); - } else if (eresult == ISC_R_SUCCESS) { + if (eresult == ISC_R_SUCCESS) { bool have_dsset; dns_name_t *name; validator_log(val, ISC_LOG_DEBUG(3), "%s with trust %s", @@ -678,10 +662,7 @@ validator_callback_ds(void *arg) { } else if ((val->attributes & VALATTR_INSECURITY) != 0) { result = proveunsecure(val, have_dsset, true); } else { - result = validate_dnskey(val); - } - if (result != DNS_R_WAIT) { - validator_done(val, result); + result = validate_async_run(val, validate_dnskey); } } else { if (eresult != DNS_R_BROKENCHAIN) { @@ -690,11 +671,13 @@ validator_callback_ds(void *arg) { validator_log(val, ISC_LOG_DEBUG(3), "validator_callback_ds: got %s", isc_result_totext(eresult)); - validator_done(val, DNS_R_BROKENCHAIN); + result = DNS_R_BROKENCHAIN; } +cleanup: + dns_validator_detach(&subvalidator->parent); dns_validator_destroy(&subvalidator); - dns_validator_detach(&val); + validate_async_done(val, result); } /*% @@ -713,16 +696,16 @@ validator_callback_cname(void *arg) { val->subvalidator = NULL; - validator_log(val, ISC_LOG_DEBUG(3), "in validator_callback_cname"); if (CANCELED(val)) { - validator_done(val, ISC_R_CANCELED); - } else if (eresult == ISC_R_SUCCESS) { + result = ISC_R_CANCELED; + goto cleanup; + } + + validator_log(val, ISC_LOG_DEBUG(3), "in validator_callback_cname"); + if (eresult == ISC_R_SUCCESS) { validator_log(val, ISC_LOG_DEBUG(3), "cname with trust %s", dns_trust_totext(val->frdataset.trust)); result = proveunsecure(val, false, true); - if (result != DNS_R_WAIT) { - validator_done(val, result); - } } else { if (eresult != DNS_R_BROKENCHAIN) { expire_rdatasets(val); @@ -730,11 +713,13 @@ validator_callback_cname(void *arg) { validator_log(val, ISC_LOG_DEBUG(3), "validator_callback_cname: got %s", isc_result_totext(eresult)); - validator_done(val, DNS_R_BROKENCHAIN); + result = DNS_R_BROKENCHAIN; } +cleanup: + dns_validator_detach(&subvalidator->parent); dns_validator_destroy(&subvalidator); - dns_validator_detach(&val); + validate_async_done(val, result); } /*% @@ -749,30 +734,19 @@ validator_callback_nsec(void *arg) { dns_validator_t *subvalidator = (dns_validator_t *)arg; dns_validator_t *val = subvalidator->parent; dns_rdataset_t *rdataset = subvalidator->rdataset; - isc_result_t result = subvalidator->result; + isc_result_t result; + isc_result_t eresult = subvalidator->result; bool exists, data; val->subvalidator = NULL; - validator_log(val, ISC_LOG_DEBUG(3), "in validator_callback_nsec"); if (CANCELED(val)) { - validator_done(val, ISC_R_CANCELED); - } else if (result != ISC_R_SUCCESS) { - validator_log(val, ISC_LOG_DEBUG(3), - "validator_callback_nsec: got %s", - isc_result_totext(result)); - if (result == DNS_R_BROKENCHAIN) { - val->authfail++; - } - if (result == ISC_R_CANCELED) { - validator_done(val, result); - } else { - result = validate_nx(val, true); - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - } - } else { + result = ISC_R_CANCELED; + goto cleanup; + } + + validator_log(val, ISC_LOG_DEBUG(3), "in validator_callback_nsec"); + if (eresult == ISC_R_SUCCESS) { dns_name_t **proofs = val->proofs; dns_name_t *wild = dns_fixedname_name(&val->wild); @@ -824,13 +798,27 @@ validator_callback_nsec(void *arg) { } result = validate_nx(val, true); - if (result != DNS_R_WAIT) { - validator_done(val, result); + } else { + validator_log(val, ISC_LOG_DEBUG(3), + "validator_callback_nsec: got %s", + isc_result_totext(eresult)); + switch (eresult) { + case ISC_R_CANCELED: + case ISC_R_SHUTTINGDOWN: + result = eresult; + break; + case DNS_R_BROKENCHAIN: + val->authfail++; + FALLTHROUGH; + default: + result = validate_nx(val, true); } } +cleanup: + dns_validator_detach(&subvalidator->parent); dns_validator_destroy(&subvalidator); - dns_validator_detach(&val); + validate_async_done(val, result); } /*% @@ -949,7 +937,6 @@ create_fetch(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, validator_logcreate(val, name, type, caller, "fetch"); dns_validator_ref(val); - result = dns_resolver_createfetch( val->view->resolver, name, type, NULL, NULL, NULL, NULL, 0, fopts, 0, NULL, val->loop, callback, val, &val->frdataset, @@ -987,9 +974,9 @@ create_validator(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, (DNS_VALIDATOR_NOCDFLAG | DNS_VALIDATOR_NONTA)); validator_logcreate(val, name, type, caller, "validator"); - result = dns_validator_create(val->view, name, type, rdataset, sig, - NULL, vopts, val->loop, cb, val, - &val->subvalidator); + result = dns_validator_create( + val->view, name, type, rdataset, sig, NULL, vopts, val->loop, + cb, val, val->nvalidations, val->nfails, &val->subvalidator); if (result == ISC_R_SUCCESS) { dns_validator_attach(val, &val->subvalidator->parent); val->subvalidator->depth = val->depth + 1; @@ -1016,59 +1003,59 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { isc_buffer_t b; dns_rdata_t rdata = DNS_RDATA_INIT; dst_key_t *oldkey = val->key; - bool foundold; + bool no_rdata = false; if (oldkey == NULL) { - foundold = true; + result = dns_rdataset_first(rdataset); } else { - foundold = false; + dst_key_free(&oldkey); val->key = NULL; + result = dns_rdataset_next(rdataset); + } + if (result != ISC_R_SUCCESS) { + goto done; } - result = dns_rdataset_first(rdataset); - if (result != ISC_R_SUCCESS) { - goto failure; - } do { dns_rdataset_current(rdataset, &rdata); isc_buffer_init(&b, rdata.data, rdata.length); isc_buffer_add(&b, rdata.length); INSIST(val->key == NULL); - result = dst_key_fromdns(&siginfo->signer, rdata.rdclass, &b, - val->view->mctx, &val->key); + result = dst_key_fromdns_ex(&siginfo->signer, rdata.rdclass, &b, + val->view->mctx, no_rdata, + &val->key); if (result == ISC_R_SUCCESS) { if (siginfo->algorithm == (dns_secalg_t)dst_key_alg(val->key) && siginfo->keyid == (dns_keytag_t)dst_key_id(val->key) && + (dst_key_flags(val->key) & DNS_KEYFLAG_REVOKE) == + 0 && dst_key_iszonekey(val->key)) { - if (foundold) { - /* - * This is the key we're looking for. - */ - return (ISC_R_SUCCESS); - } else if (dst_key_compare(oldkey, val->key)) { - foundold = true; - dst_key_free(&oldkey); + if (no_rdata) { + /* Retry with full key */ + dns_rdata_reset(&rdata); + dst_key_free(&val->key); + no_rdata = false; + continue; } + /* This is the key we're looking for. */ + goto done; } dst_key_free(&val->key); } dns_rdata_reset(&rdata); result = dns_rdataset_next(rdataset); + no_rdata = true; } while (result == ISC_R_SUCCESS); +done: if (result == ISC_R_NOMORE) { result = ISC_R_NOTFOUND; } -failure: - if (oldkey != NULL) { - dst_key_free(&oldkey); - } - return (result); } @@ -1181,15 +1168,22 @@ seek_dnskey(dns_validator_t *val) { validator_log(val, ISC_LOG_DEBUG(3), "keyset with trust %s", dns_trust_totext(val->frdataset.trust)); - result = select_signing_key(val, val->keyset); - if (result != ISC_R_SUCCESS) { - /* - * Either the key we're looking for is not - * in the rrset, or something bad happened. - * Give up. - */ - result = DNS_R_CONTINUE; + + /* + * Cleanup before passing control to the offload thread + */ + if (dns_rdataset_isassociated(&val->frdataset) && + val->keyset != &val->frdataset) + { + dns_rdataset_disassociate(&val->frdataset); } + if (dns_rdataset_isassociated(&val->fsigrdataset)) { + dns_rdataset_disassociate(&val->fsigrdataset); + } + + isc_work_enqueue(val->loop, resume_answer_with_key, + resume_answer_with_key_done, val); + return (DNS_R_WAIT); } break; @@ -1246,20 +1240,47 @@ compute_keytag(dns_rdata_t *rdata) { return (dst_region_computeid(&r)); } +static bool +over_max_validations(dns_validator_t *val) { + if (val->nvalidations == NULL) { + return (false); + } + if (*val->nvalidations > 0) { + (*val->nvalidations)--; + return (false); + } + + val->attributes |= VALATTR_MAXVALIDATIONS; + return (true); +} + +static bool +over_max_fails(dns_validator_t *val) { + if (val->nfails == NULL) { + return (false); + } + if (*val->nfails > 0) { + (*val->nfails)--; + return (false); + } + + val->attributes |= VALATTR_MAXVALIDATIONFAILS; + return (true); +} + /*% * Is the DNSKEY rrset in val->rdataset self-signed? */ -static bool +static isc_result_t selfsigned_dnskey(dns_validator_t *val) { dns_rdataset_t *rdataset = val->rdataset; dns_rdataset_t *sigrdataset = val->sigrdataset; dns_name_t *name = val->name; isc_result_t result; isc_mem_t *mctx = val->view->mctx; - bool answer = false; if (rdataset->type != dns_rdatatype_dnskey) { - return (false); + return (DNS_R_NOKEYMATCH); } for (result = dns_rdataset_first(rdataset); result == ISC_R_SUCCESS; @@ -1301,8 +1322,7 @@ selfsigned_dnskey(dns_validator_t *val) { * This will be verified later. */ if ((key.flags & DNS_KEYFLAG_REVOKE) == 0) { - answer = true; - continue; + return (ISC_R_SUCCESS); } result = dns_dnssec_keyfromrdata(name, &keyrdata, mctx, @@ -1318,6 +1338,10 @@ selfsigned_dnskey(dns_validator_t *val) { if (DNS_TRUST_PENDING(rdataset->trust) && dns_view_istrusted(val->view, name, &key)) { + if (over_max_validations(val)) { + dst_key_free(&dstkey); + return (ISC_R_QUOTA); + } result = dns_dnssec_verify( name, rdataset, dstkey, true, val->view->maxbits, mctx, &sigrdata, @@ -1329,6 +1353,9 @@ selfsigned_dnskey(dns_validator_t *val) { * good. */ dns_view_untrust(val->view, name, &key); + } else if (over_max_fails(val)) { + dst_key_free(&dstkey); + return (ISC_R_QUOTA); } } else if (rdataset->trust >= dns_trust_secure) { /* @@ -1342,7 +1369,7 @@ selfsigned_dnskey(dns_validator_t *val) { } } - return (answer); + return (DNS_R_NOKEYMATCH); } /*% @@ -1365,6 +1392,9 @@ verify(dns_validator_t *val, dst_key_t *key, dns_rdata_t *rdata, val->attributes |= VALATTR_TRIEDVERIFY; wild = dns_fixedname_initname(&fixed); again: + if (over_max_validations(val)) { + return (ISC_R_QUOTA); + } result = dns_dnssec_verify(val->name, val->rdataset, key, ignore, val->view->maxbits, val->view->mctx, rdata, wild); @@ -1408,6 +1438,10 @@ again: } result = ISC_R_SUCCESS; } + + if (result != ISC_R_SUCCESS && over_max_fails(val)) { + result = ISC_R_QUOTA; + } return (result); } @@ -1420,132 +1454,323 @@ again: * for an event. * \li Other return codes are possible and all indicate failure. */ -static isc_result_t -validate_answer(dns_validator_t *val, bool resume) { - isc_result_t result, vresult = DNS_R_NOVALIDSIG; - dns_rdata_t rdata = DNS_RDATA_INIT; + +static void +validate_answer_iter_next(void *arg); +static void +validate_answer_process(void *arg); +static void +validate_answer_iter_done(dns_validator_t *val, isc_result_t result); + +static void +validate_answer_iter_start(dns_validator_t *val) { + isc_result_t result = ISC_R_SUCCESS; /* * Caller must be holding the validator lock. */ - if (resume) { - /* - * We already have a sigrdataset. - */ + if (CANCELED(val)) { + result = ISC_R_CANCELED; + goto cleanup; + } + + if (val->resume) { + /* We already have a sigrdataset. */ result = ISC_R_SUCCESS; validator_log(val, ISC_LOG_DEBUG(3), "resuming validate"); } else { result = dns_rdataset_first(val->sigrdataset); } - for (; result == ISC_R_SUCCESS; - result = dns_rdataset_next(val->sigrdataset)) - { - dns_rdata_reset(&rdata); - dns_rdataset_current(val->sigrdataset, &rdata); - if (val->siginfo == NULL) { - val->siginfo = isc_mem_get(val->view->mctx, - sizeof(*val->siginfo)); - } - result = dns_rdata_tostruct(&rdata, val->siginfo, NULL); - if (result != ISC_R_SUCCESS) { - return (result); - } +cleanup: + if (result != ISC_R_SUCCESS) { + validate_answer_iter_done(val, result); + return; + } - /* - * At this point we could check that the signature algorithm - * was known and "sufficiently good". - */ - if (!dns_resolver_algorithm_supported(val->view->resolver, - val->name, - val->siginfo->algorithm)) - { - resume = false; - continue; - } + result = validate_async_run(val, validate_answer_process); + INSIST(result == DNS_R_WAIT); +} - if (!resume) { - result = seek_dnskey(val); - if (result == DNS_R_CONTINUE) { - continue; /* Try the next SIG RR. */ - } - if (result != ISC_R_SUCCESS) { - return (result); - } - } +static void +validate_answer_iter_next(void *arg) { + dns_validator_t *val = arg; + isc_result_t result; - /* - * There isn't a secure DNSKEY for this signature so move - * onto the next RRSIG. - */ - if (val->key == NULL) { - resume = false; - continue; - } + if (CANCELED(val)) { + result = ISC_R_CANCELED; + goto cleanup; + } - do { - isc_result_t tresult; - vresult = verify(val, val->key, &rdata, - val->siginfo->keyid); - if (vresult == ISC_R_SUCCESS) { - break; - } + val->resume = false; + result = dns_rdataset_next(val->sigrdataset); - tresult = select_signing_key(val, val->keyset); - if (tresult != ISC_R_SUCCESS) { - break; - } - } while (1); - if (vresult != ISC_R_SUCCESS) { - validator_log(val, ISC_LOG_DEBUG(3), - "failed to verify rdataset"); - } else { - dns_rdataset_trimttl(val->rdataset, val->sigrdataset, - val->siginfo, val->start, - val->view->acceptexpired); - } +cleanup: + if (result != ISC_R_SUCCESS) { + validate_answer_iter_done(val, result); + return; + } + (void)validate_async_run(val, validate_answer_process); +} + +static void +validate_answer_finish(void *arg); + +static void +validate_answer_signing_key(void *arg) { + dns_validator_t *val = arg; + isc_result_t result = ISC_R_NOTFOUND; + + if (CANCELED(val)) { + val->result = ISC_R_CANCELED; + } else { + val->result = verify(val, val->key, &val->rdata, + val->siginfo->keyid); + } + + switch (val->result) { + case ISC_R_CANCELED: /* Validation was canceled */ + case ISC_R_SHUTTINGDOWN: /* Server shutting down */ + case ISC_R_QUOTA: /* Validation fails quota reached */ + case ISC_R_SUCCESS: /* We found our valid signature, we are done! */ if (val->key != NULL) { dst_key_free(&val->key); + val->key = NULL; } - if (val->keyset != NULL) { - dns_rdataset_disassociate(val->keyset); - val->keyset = NULL; - } - val->key = NULL; - if (NEEDNOQNAME(val)) { - if (val->message == NULL) { - validator_log(val, ISC_LOG_DEBUG(3), - "no message available " - "for noqname proof"); - return (DNS_R_NOVALIDSIG); - } - validator_log(val, ISC_LOG_DEBUG(3), - "looking for noqname proof"); - return (validate_nx(val, false)); - } else if (vresult == ISC_R_SUCCESS) { - marksecure(val); - validator_log(val, ISC_LOG_DEBUG(3), - "marking as secure, " - "noqname proof not needed"); - return (ISC_R_SUCCESS); - } else { - validator_log(val, ISC_LOG_DEBUG(3), - "verify failure: %s", - isc_result_totext(result)); - resume = false; + + break; + default: + /* Select next signing key */ + result = select_signing_key(val, val->keyset); + break; + } + + if (result == ISC_R_SUCCESS) { + INSIST(val->key != NULL); + } else { + INSIST(val->key == NULL); + } +} + +static void +validate_answer_signing_key_done(void *arg) { + dns_validator_t *val = arg; + + if (CANCELED(val)) { + val->result = ISC_R_CANCELED; + } else if (val->key != NULL) { + /* Process with next key if we selected one */ + isc_work_enqueue(val->loop, validate_answer_signing_key, + validate_answer_signing_key_done, val); + return; + } + + validate_answer_finish(val); +} + +static void +validate_answer_process(void *arg) { + dns_validator_t *val = arg; + isc_result_t result; + + if (CANCELED(val)) { + result = ISC_R_CANCELED; + goto cleanup; + } + + dns_rdata_reset(&val->rdata); + + dns_rdataset_current(val->sigrdataset, &val->rdata); + if (val->siginfo == NULL) { + val->siginfo = isc_mem_get(val->view->mctx, + sizeof(*val->siginfo)); + } + result = dns_rdata_tostruct(&val->rdata, val->siginfo, NULL); + if (result != ISC_R_SUCCESS) { + goto cleanup; + } + + /* + * At this point we could check that the signature algorithm + * was known and "sufficiently good". + */ + if (!dns_resolver_algorithm_supported(val->view->resolver, val->name, + val->siginfo->algorithm)) + { + goto next_key; + } + + if (!val->resume) { + result = seek_dnskey(val); + switch (result) { + case ISC_R_SUCCESS: + break; + case DNS_R_CONTINUE: + goto next_key; + case DNS_R_WAIT: + goto cleanup; + default: + goto cleanup; } } + + /* + * There isn't a secure DNSKEY for this signature so move + * onto the next RRSIG. + */ + if (val->key == NULL) { + val->resume = false; + goto next_key; + } + + isc_work_enqueue(val->loop, validate_answer_signing_key, + validate_answer_signing_key_done, val); + return; + +next_key: + result = validate_async_run(val, validate_answer_iter_next); + goto cleanup; + +cleanup: + validate_async_done(val, result); +} + +static void +validate_answer_finish(void *arg) { + dns_validator_t *val = arg; + isc_result_t result = ISC_R_UNSET; + + if (val->result != ISC_R_SUCCESS) { + validator_log(val, ISC_LOG_DEBUG(3), + "failed to verify rdataset: %s", + isc_result_totext(val->result)); + } else { + dns_rdataset_trimttl(val->rdataset, val->sigrdataset, + val->siginfo, val->start, + val->view->acceptexpired); + } + + if (val->key != NULL) { + dst_key_free(&val->key); + val->key = NULL; + } + if (val->keyset != NULL) { + dns_rdataset_disassociate(val->keyset); + val->keyset = NULL; + } + + switch (val->result) { + case ISC_R_CANCELED: + /* The validation was canceled */ + validator_log(val, ISC_LOG_DEBUG(3), "validation was canceled"); + validate_async_done(val, val->result); + return; + case ISC_R_SHUTTINGDOWN: + validator_log(val, ISC_LOG_DEBUG(3), "server is shutting down"); + validate_async_done(val, val->result); + return; + case ISC_R_QUOTA: + if (MAXVALIDATIONS(val)) { + validator_log(val, ISC_LOG_DEBUG(3), + "maximum number of validations exceeded"); + } else if (MAXVALIDATIONFAILS(val)) { + validator_log(val, ISC_LOG_DEBUG(3), + "maximum number of validation failures " + "exceeded"); + } else { + validator_log( + val, ISC_LOG_DEBUG(3), + "unknown error: validation quota exceeded"); + } + validate_async_done(val, val->result); + return; + default: + break; + } + + if (NEEDNOQNAME(val)) { + if (val->message == NULL) { + validator_log(val, ISC_LOG_DEBUG(3), + "no message available for noqname proof"); + validate_async_done(val, DNS_R_NOVALIDSIG); + return; + } + + validator_log(val, ISC_LOG_DEBUG(3), + "looking for noqname proof"); + result = validate_nx(val, false); + validate_async_done(val, result); + return; + } + + if (val->result == ISC_R_SUCCESS) { + marksecure(val); + validator_log(val, ISC_LOG_DEBUG(3), + "marking as secure, noqname proof not needed"); + validate_async_done(val, val->result); + return; + } + + validator_log(val, ISC_LOG_DEBUG(3), "verify failure: %s", + isc_result_totext(val->result)); + (void)validate_async_run(val, validate_answer_iter_next); +} + +static void +validate_answer_iter_done(dns_validator_t *val, isc_result_t result) { if (result != ISC_R_NOMORE) { validator_log(val, ISC_LOG_DEBUG(3), "failed to iterate signatures: %s", isc_result_totext(result)); - return (result); + validate_async_done(val, result); + return; } validator_log(val, ISC_LOG_INFO, "no valid signature found"); - return (vresult); + validate_async_done(val, val->result); +} + +static void +resume_answer(void *arg) { + dns_validator_t *val = arg; + val->resume = true; + validate_answer_iter_start(val); +} + +static void +validate_answer(void *arg) { + dns_validator_t *val = arg; + val->resume = false; + validate_answer_iter_start(val); +} + +static isc_result_t +validate_async_run(dns_validator_t *val, isc_job_cb cb) { + isc_async_run(val->loop, cb, val); + return (DNS_R_WAIT); +} + +static void +validate_async_done(dns_validator_t *val, isc_result_t result) { + if (result == DNS_R_NOVALIDSIG && + (val->attributes & VALATTR_TRIEDVERIFY) == 0) + { + isc_result_t saved_result = result; + validator_log(val, ISC_LOG_DEBUG(3), + "falling back to insecurity proof"); + result = proveunsecure(val, false, false); + if (result == DNS_R_NOTINSECURE) { + result = saved_result; + } + } + + if (result != DNS_R_WAIT) { + /* We are still continuing */ + validator_done(val, result); + dns_validator_detach(&val); + } } /*% @@ -1582,7 +1807,7 @@ check_signer(dns_validator_t *val, dns_rdata_t *keyrdata, uint16_t keyid, } } result = verify(val, dstkey, &rdata, sig.keyid); - if (result == ISC_R_SUCCESS) { + if (result == ISC_R_SUCCESS || result == ISC_R_QUOTA) { break; } } @@ -1683,27 +1908,163 @@ get_dsset(dns_validator_t *val, dns_name_t *tname, isc_result_t *resp) { return (DNS_R_CONTINUE); } -/*% - * Attempts positive response validation of an RRset containing zone keys - * (i.e. a DNSKEY rrset). - * - * Caller must be holding the validator lock. - * - * Returns: - * \li ISC_R_SUCCESS Validation completed successfully - * \li DNS_R_WAIT Validation has started but is waiting - * for an event. - * \li Other return codes are possible and all indicate failure. - */ +static void +validate_dnskey_dsset_done(dns_validator_t *val, isc_result_t result) { + if (result == ISC_R_SUCCESS) { + marksecure(val); + validator_log(val, ISC_LOG_DEBUG(3), "marking as secure (DS)"); + } else if (result == ISC_R_NOMORE && !val->supported_algorithm) { + validator_log(val, ISC_LOG_DEBUG(3), + "no supported algorithm/digest (DS)"); + result = markanswer(val, "validate_dnskey (3)", + "no supported algorithm/digest (DS)"); + } else { + validator_log(val, ISC_LOG_INFO, + "no valid signature found (DS)"); + result = DNS_R_NOVALIDSIG; + } + + if (val->dsset == &val->fdsset) { + val->dsset = NULL; + dns_rdataset_disassociate(&val->fdsset); + } + + validate_async_done(val, result); +} + static isc_result_t -validate_dnskey(dns_validator_t *val) { - isc_result_t result; +validate_dnskey_dsset(dns_validator_t *val) { dns_rdata_t dsrdata = DNS_RDATA_INIT; dns_rdata_t keyrdata = DNS_RDATA_INIT; + isc_result_t result; + dns_rdata_ds_t ds; + + dns_rdata_reset(&dsrdata); + dns_rdataset_current(val->dsset, &dsrdata); + result = dns_rdata_tostruct(&dsrdata, &ds, NULL); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + + if (ds.digest_type == DNS_DSDIGEST_SHA1 && val->digest_sha1 == false) { + return (DNS_R_BADALG); + } + + if (!dns_resolver_ds_digest_supported(val->view->resolver, val->name, + ds.digest_type)) + { + return (DNS_R_BADALG); + } + + if (!dns_resolver_algorithm_supported(val->view->resolver, val->name, + ds.algorithm)) + { + return (DNS_R_BADALG); + } + + val->supported_algorithm = true; + + /* + * Find the DNSKEY matching the DS... + */ + result = dns_dnssec_matchdskey(val->name, &dsrdata, val->rdataset, + &keyrdata); + if (result != ISC_R_SUCCESS) { + validator_log(val, ISC_LOG_DEBUG(3), "no DNSKEY matching DS"); + return (DNS_R_NOKEYMATCH); + } + + /* + * ... and check that it signed the DNSKEY RRset. + */ + result = check_signer(val, &keyrdata, ds.key_tag, ds.algorithm); + if (result != ISC_R_SUCCESS) { + validator_log(val, ISC_LOG_DEBUG(3), + "no RRSIG matching DS key"); + + return (result); + } + + return (ISC_R_SUCCESS); +} + +static void +validate_dnskey_dsset_next(void *arg) { + dns_validator_t *val = arg; + + if (CANCELED(val)) { + val->result = ISC_R_CANCELED; + } else { + val->result = dns_rdataset_next(val->dsset); + } + + if (val->result == ISC_R_SUCCESS) { + /* continue async run */ + val->result = validate_dnskey_dsset(val); + } +} + +static void +validate_dnskey_dsset_next_done(void *arg) { + dns_validator_t *val = arg; + isc_result_t result = val->result; + + if (CANCELED(val)) { + result = ISC_R_CANCELED; + } + + switch (result) { + case ISC_R_CANCELED: + case ISC_R_SHUTTINGDOWN: + /* Abort, abort, abort! */ + break; + case ISC_R_SUCCESS: + case ISC_R_NOMORE: + /* We are done */ + break; + default: + /* Continue validation until we have success or no more data */ + isc_work_enqueue(val->loop, validate_dnskey_dsset_next, + validate_dnskey_dsset_next_done, val); + return; + } + + validate_dnskey_dsset_done(val, result); + return; +} + +static void +validate_dnskey_dsset_first(dns_validator_t *val) { + isc_result_t result; + + if (CANCELED(val)) { + result = ISC_R_CANCELED; + } else { + result = dns_rdataset_first(val->dsset); + } + + if (result == ISC_R_SUCCESS) { + /* continue async run */ + result = validate_dnskey_dsset(val); + if (result != ISC_R_SUCCESS) { + isc_work_enqueue(val->loop, validate_dnskey_dsset_next, + validate_dnskey_dsset_next_done, val); + return; + } + } + + validate_dnskey_dsset_done(val, result); +} + +static void +validate_dnskey(void *arg) { + dns_validator_t *val = arg; + isc_result_t result = ISC_R_SUCCESS; dns_keynode_t *keynode = NULL; dns_rdata_ds_t ds; - bool supported_algorithm; - char digest_types[256]; + + if (CANCELED(val)) { + result = ISC_R_CANCELED; + goto cleanup; + } /* * If we don't already have a DS RRset, check to see if there's @@ -1767,7 +2128,7 @@ validate_dnskey(dns_validator_t *val) { * verification. */ - supported_algorithm = false; + val->supported_algorithm = false; /* * If DNS_DSDIGEST_SHA256 or DNS_DSDIGEST_SHA384 is present we @@ -1775,7 +2136,8 @@ validate_dnskey(dns_validator_t *val) { * practice means that we need to ignore DNS_DSDIGEST_SHA1 if a * DNS_DSDIGEST_SHA256 or DNS_DSDIGEST_SHA384 is present. */ - memset(digest_types, 1, sizeof(digest_types)); + val->digest_sha1 = true; + dns_rdata_t dsrdata = DNS_RDATA_INIT; for (result = dns_rdataset_first(val->dsset); result == ISC_R_SUCCESS; result = dns_rdataset_next(val->dsset)) { @@ -1801,80 +2163,20 @@ validate_dnskey(dns_validator_t *val) { (ds.digest_type == DNS_DSDIGEST_SHA384 && ds.length == ISC_SHA384_DIGESTLENGTH)) { - digest_types[DNS_DSDIGEST_SHA1] = 0; + val->digest_sha1 = false; break; } } - for (result = dns_rdataset_first(val->dsset); result == ISC_R_SUCCESS; - result = dns_rdataset_next(val->dsset)) - { - dns_rdata_reset(&dsrdata); - dns_rdataset_current(val->dsset, &dsrdata); - result = dns_rdata_tostruct(&dsrdata, &ds, NULL); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - - if (digest_types[ds.digest_type] == 0) { - continue; - } - - if (!dns_resolver_ds_digest_supported( - val->view->resolver, val->name, ds.digest_type)) - { - continue; - } - - if (!dns_resolver_algorithm_supported(val->view->resolver, - val->name, ds.algorithm)) - { - continue; - } - - supported_algorithm = true; - - /* - * Find the DNSKEY matching the DS... - */ - result = dns_dnssec_matchdskey(val->name, &dsrdata, - val->rdataset, &keyrdata); - if (result != ISC_R_SUCCESS) { - validator_log(val, ISC_LOG_DEBUG(3), - "no DNSKEY matching DS"); - continue; - } - - /* - * ... and check that it signed the DNSKEY RRset. - */ - result = check_signer(val, &keyrdata, ds.key_tag, ds.algorithm); - if (result == ISC_R_SUCCESS) { - break; - } - validator_log(val, ISC_LOG_DEBUG(3), - "no RRSIG matching DS key"); - } - - if (result == ISC_R_SUCCESS) { - marksecure(val); - validator_log(val, ISC_LOG_DEBUG(3), "marking as secure (DS)"); - } else if (result == ISC_R_NOMORE && !supported_algorithm) { - validator_log(val, ISC_LOG_DEBUG(3), - "no supported algorithm/digest (DS)"); - result = markanswer(val, "validate_dnskey (3)", - "no supported algorithm/digest (DS)"); - } else { - validator_log(val, ISC_LOG_INFO, - "no valid signature found (DS)"); - result = DNS_R_NOVALIDSIG; - } + validate_dnskey_dsset_first(val); + return; cleanup: if (val->dsset == &val->fdsset) { val->dsset = NULL; dns_rdataset_disassociate(&val->fdsset); } - - return (result); + validate_async_done(val, result); } /*% @@ -2521,7 +2823,6 @@ validate_nx(dns_validator_t *val, bool resume) { return (DNS_R_BROKENCHAIN); } - validator_log(val, ISC_LOG_DEBUG(3), "nonexistence proof(s) not found"); return (proveunsecure(val, false, false)); } @@ -2597,10 +2898,10 @@ seek_ds(dns_validator_t *val, isc_result_t *resp) { */ if (val->frdataset.trust >= dns_trust_secure) { if (!check_ds_algs(val, tname, &val->frdataset)) { - validator_log(val, ISC_LOG_DEBUG(3), - "no supported algorithm/" - "digest (%s/DS)", - namebuf); + validator_log( + val, ISC_LOG_DEBUG(3), + "no supported algorithm/digest (%s/DS)", + namebuf); *resp = markanswer(val, "proveunsecure (5)", "no supported " "algorithm/digest (DS)"); @@ -2912,14 +3213,13 @@ validator_start(void *arg) { isc_result_t result = ISC_R_FAILURE; if (CANCELED(val)) { - return; + result = ISC_R_CANCELED; + goto cleanup; } validator_log(val, ISC_LOG_DEBUG(3), "starting"); if (val->rdataset != NULL && val->sigrdataset != NULL) { - isc_result_t saved_result; - /* * This looks like a simple validation. We say "looks like" * because it might end up requiring an insecurity proof. @@ -2929,21 +3229,19 @@ validator_start(void *arg) { INSIST(dns_rdataset_isassociated(val->rdataset)); INSIST(dns_rdataset_isassociated(val->sigrdataset)); - if (selfsigned_dnskey(val)) { - result = validate_dnskey(val); - } else { - result = validate_answer(val, false); - } - if (result == DNS_R_NOVALIDSIG && - (val->attributes & VALATTR_TRIEDVERIFY) == 0) - { - saved_result = result; - validator_log(val, ISC_LOG_DEBUG(3), - "falling back to insecurity proof"); - result = proveunsecure(val, false, false); - if (result == DNS_R_NOTINSECURE) { - result = saved_result; - } + + result = selfsigned_dnskey(val); + switch (result) { + case ISC_R_QUOTA: + goto cleanup; + case ISC_R_SUCCESS: + result = validate_async_run(val, validate_dnskey); + break; + case DNS_R_NOKEYMATCH: + result = validate_async_run(val, validate_answer); + break; + default: + UNREACHABLE(); } } else if (val->rdataset != NULL && val->rdataset->type != 0) { /* @@ -2996,11 +3294,8 @@ validator_start(void *arg) { UNREACHABLE(); } - if (result != DNS_R_WAIT) { - validator_done(val, result); - } - - dns_validator_detach(&val); +cleanup: + validate_async_done(val, result); } isc_result_t @@ -3008,6 +3303,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, dns_message_t *message, unsigned int options, isc_loop_t *loop, isc_job_cb cb, void *arg, + uint32_t *nvalidations, uint32_t *nfails, dns_validator_t **validatorp) { isc_result_t result = ISC_R_FAILURE; dns_validator_t *val = NULL; @@ -3024,18 +3320,23 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, } val = isc_mem_get(view->mctx, sizeof(*val)); - *val = (dns_validator_t){ .tid = isc_tid(), - .result = ISC_R_FAILURE, - .rdataset = rdataset, - .sigrdataset = sigrdataset, - .name = name, - .type = type, - .options = options, - .keytable = kt, - .link = ISC_LINK_INITIALIZER, - .loop = loop, - .cb = cb, - .arg = arg }; + *val = (dns_validator_t){ + .tid = isc_tid(), + .result = DNS_R_NOVALIDSIG, + .rdataset = rdataset, + .sigrdataset = sigrdataset, + .name = name, + .type = type, + .options = options, + .keytable = kt, + .link = ISC_LINK_INITIALIZER, + .loop = loop, + .cb = cb, + .arg = arg, + .rdata = DNS_RDATA_INIT, + .nvalidations = nvalidations, + .nfails = nfails, + }; isc_refcount_init(&val->references, 1); dns_view_attach(view, &val->view); @@ -3054,7 +3355,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, if ((options & DNS_VALIDATOR_DEFER) == 0) { dns_validator_ref(val); - isc_async_run(val->loop, validator_start, val); + (void)validate_async_run(val, validator_start); } *validatorp = val; @@ -3063,15 +3364,15 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, } void -dns_validator_send(dns_validator_t *validator) { - REQUIRE(VALID_VALIDATOR(validator)); - REQUIRE(validator->tid == isc_tid()); +dns_validator_send(dns_validator_t *val) { + REQUIRE(VALID_VALIDATOR(val)); + REQUIRE(val->tid == isc_tid()); - INSIST((validator->options & DNS_VALIDATOR_DEFER) != 0); - validator->options &= ~DNS_VALIDATOR_DEFER; + INSIST((val->options & DNS_VALIDATOR_DEFER) != 0); + val->options &= ~DNS_VALIDATOR_DEFER; - dns_validator_ref(validator); - isc_async_run(validator->loop, validator_start, validator); + dns_validator_ref(val); + (void)validate_async_run(val, validator_start); } void @@ -3092,7 +3393,6 @@ dns_validator_cancel(dns_validator_t *validator) { validator->options &= ~DNS_VALIDATOR_DEFER; validator_done(validator, ISC_R_CANCELED); } - validator->attributes |= VALATTR_CANCELED; } } @@ -3111,9 +3411,6 @@ destroy_validator(dns_validator_t *val) { if (val->keytable != NULL) { dns_keytable_detach(&val->keytable); } - if (val->subvalidator != NULL) { - dns_validator_destroy(&val->subvalidator); - } disassociate_rdatasets(val); mctx = val->view->mctx; if (val->siginfo != NULL) { diff --git a/lib/isc/loop.c b/lib/isc/loop.c index e31563b63d..829cf168a4 100644 --- a/lib/isc/loop.c +++ b/lib/isc/loop.c @@ -138,6 +138,8 @@ static void shutdown_trigger_close_cb(uv_handle_t *handle) { isc_loop_t *loop = uv_handle_get_data(handle); + loop->shuttingdown = true; + isc_loop_detach(&loop); } diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index 10aa92ee22..3ffa2c95c4 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -2103,6 +2103,10 @@ static cfg_clausedef_t view_clauses[] = { { "max-recursion-queries", &cfg_type_uint32, 0 }, { "max-stale-ttl", &cfg_type_duration, 0 }, { "max-udp-size", &cfg_type_uint32, 0 }, + { "max-validations-per-fetch", &cfg_type_uint32, + CFG_CLAUSEFLAG_EXPERIMENTAL }, + { "max-validation-failures-per-fetch", &cfg_type_uint32, + CFG_CLAUSEFLAG_EXPERIMENTAL }, { "message-compression", &cfg_type_boolean, 0 }, { "min-cache-ttl", &cfg_type_duration, 0 }, { "min-ncache-ttl", &cfg_type_duration, 0 }, From abed39ec0deeed276bd3075dadb92ae51ed2df85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 16 Jan 2024 11:46:29 +0100 Subject: [PATCH 22/25] Add CHANGES and release note for [GL #4424] --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 doc/notes/notes-current.rst diff --git a/CHANGES b/CHANGES index 095d027bb5..5a59e438b9 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6322. [security] Specific DNS answers could cause a denial-of-service + condition due to DNS validation taking a long time. + (CVE-2023-50387) [GL #4424] + 6321. [security] Change 6315 inadvertently introduced regressions that could cause named to crash. [GL #4234] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst new file mode 100644 index 0000000000..0a84c2cadc --- /dev/null +++ b/doc/notes/notes-current.rst @@ -0,0 +1,31 @@ +.. Copyright (C) Internet Systems Consortium, Inc. ("ISC") +.. +.. SPDX-License-Identifier: MPL-2.0 +.. +.. 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 https://mozilla.org/MPL/2.0/. +.. +.. See the COPYRIGHT file distributed with this work for additional +.. information regarding copyright ownership. + +Notes for BIND 9.19.21 +---------------------- + +Security Fixes +~~~~~~~~~~~~~~ + +- Validating DNS messages containing a lot of DNSSEC signatures could + cause excessive CPU load, leading to a denial-of-service condition. + This has been fixed. :cve:`2023-50387` + + ISC would like to thank Elias Heftrig, Haya Schulmann, Niklas Vogel, + and Michael Waidner from the German National Research Center for + Applied Cybersecurity ATHENE. :gl:`#4424` + +Known Issues +~~~~~~~~~~~~ + +- There are no new known issues with this release. See :ref:`above + ` for a list of all known issues affecting this + BIND 9 branch. From 265deccb85bef9d092e43f4400f9953c46f76c9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 2 Feb 2024 09:21:49 +0100 Subject: [PATCH 23/25] Prepare release notes for BIND 9.19.21 --- doc/arm/notes.rst | 1 + doc/notes/notes-9.19.20.rst | 50 +++----------------------- doc/notes/notes-9.19.21.rst | 70 +++++++++++++++++++++++++++++++++++++ doc/notes/notes-current.rst | 31 ---------------- 4 files changed, 75 insertions(+), 77 deletions(-) create mode 100644 doc/notes/notes-9.19.21.rst delete mode 100644 doc/notes/notes-current.rst diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index e3c8787b93..7ae4697f18 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -38,6 +38,7 @@ information about each release, and source code. .. include:: ../notes/notes-known-issues.rst +.. include:: ../notes/notes-9.19.21.rst .. include:: ../notes/notes-9.19.20.rst .. include:: ../notes/notes-9.19.19.rst .. include:: ../notes/notes-9.19.18.rst diff --git a/doc/notes/notes-9.19.20.rst b/doc/notes/notes-9.19.20.rst index 33aec1d0e5..c794662095 100644 --- a/doc/notes/notes-9.19.20.rst +++ b/doc/notes/notes-9.19.20.rst @@ -12,50 +12,8 @@ Notes for BIND 9.19.20 ---------------------- -Security Fixes -~~~~~~~~~~~~~~ +.. note:: -- Parsing DNS messages with many different names could cause excessive - CPU load. This has been fixed. :cve:`2023-4408` - - ISC would like to thank Shoham Danino from Reichman University, Anat - Bremler-Barr from Tel-Aviv University, Yehuda Afek from Tel-Aviv - University, and Yuval Shavitt from Tel-Aviv University for bringing - this vulnerability to our attention. :gl:`#4234` - -- Specific queries could cause :iscman:`named` to crash with an - assertion failure when :any:`nxdomain-redirect` was enabled. This has - been fixed. :cve:`2023-5517` :gl:`#4281` - -- A bad interaction between DNS64 and serve-stale could cause - :iscman:`named` to crash with an assertion failure, when both of these - features were enabled. This has been fixed. :cve:`2023-5679` - :gl:`#4334` - -Feature Changes -~~~~~~~~~~~~~~~ - -- :iscman:`named-compilezone` no longer performs zone integrity checks - by default; this allows faster conversion of a zone file from one - format to another. :gl:`#4364` - - Zone checks can be performed by running :iscman:`named-checkzone` - separately, or the previous default behavior can be restored by using: - - :: - - named-compilezone -i full -k fail -n fail -r warn -m warn -M warn -S warn -T warn -W warn -C check-svcb:fail - -Bug Fixes -~~~~~~~~~ - -- The counters exported via the statistics channel were changed back to - 64-bit signed values; they were being inadvertently truncated to - unsigned 32-bit values since BIND 9.15.0. :gl:`#4467` - -Known Issues -~~~~~~~~~~~~ - -- There are no new known issues with this release. See :ref:`above - ` for a list of all known issues affecting this - BIND 9 branch. + The BIND 9.19.20 release was withdrawn after the discovery of a + regression in a security fix in it during pre-release testing. ISC + would like to acknowledge the assistance of Curtis Tuplin of SaskTel. diff --git a/doc/notes/notes-9.19.21.rst b/doc/notes/notes-9.19.21.rst new file mode 100644 index 0000000000..16f1b7bc3b --- /dev/null +++ b/doc/notes/notes-9.19.21.rst @@ -0,0 +1,70 @@ +.. Copyright (C) Internet Systems Consortium, Inc. ("ISC") +.. +.. SPDX-License-Identifier: MPL-2.0 +.. +.. 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 https://mozilla.org/MPL/2.0/. +.. +.. See the COPYRIGHT file distributed with this work for additional +.. information regarding copyright ownership. + +Notes for BIND 9.19.21 +---------------------- + +Security Fixes +~~~~~~~~~~~~~~ + +- Validating DNS messages containing a lot of DNSSEC signatures could + cause excessive CPU load, leading to a denial-of-service condition. + This has been fixed. :cve:`2023-50387` + + ISC would like to thank Elias Heftrig, Haya Schulmann, Niklas Vogel, + and Michael Waidner from the German National Research Center for + Applied Cybersecurity ATHENE for bringing this vulnerability to our + attention. :gl:`#4424` + +- Parsing DNS messages with many different names could cause excessive + CPU load. This has been fixed. :cve:`2023-4408` + + ISC would like to thank Shoham Danino from Reichman University, Anat + Bremler-Barr from Tel-Aviv University, Yehuda Afek from Tel-Aviv + University, and Yuval Shavitt from Tel-Aviv University for bringing + this vulnerability to our attention. :gl:`#4234` + +- Specific queries could cause :iscman:`named` to crash with an + assertion failure when :any:`nxdomain-redirect` was enabled. This has + been fixed. :cve:`2023-5517` :gl:`#4281` + +- A bad interaction between DNS64 and serve-stale could cause + :iscman:`named` to crash with an assertion failure, when both of these + features were enabled. This has been fixed. :cve:`2023-5679` + :gl:`#4334` + +Feature Changes +~~~~~~~~~~~~~~~ + +- :iscman:`named-compilezone` no longer performs zone integrity checks + by default; this allows faster conversion of a zone file from one + format to another. :gl:`#4364` + + Zone checks can be performed by running :iscman:`named-checkzone` + separately, or the previous default behavior can be restored by using: + + :: + + named-compilezone -i full -k fail -n fail -r warn -m warn -M warn -S warn -T warn -W warn -C check-svcb:fail + +Bug Fixes +~~~~~~~~~ + +- The counters exported via the statistics channel were changed back to + 64-bit signed values; they were being inadvertently truncated to + unsigned 32-bit values since BIND 9.15.0. :gl:`#4467` + +Known Issues +~~~~~~~~~~~~ + +- There are no new known issues with this release. See :ref:`above + ` for a list of all known issues affecting this + BIND 9 branch. diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst deleted file mode 100644 index 0a84c2cadc..0000000000 --- a/doc/notes/notes-current.rst +++ /dev/null @@ -1,31 +0,0 @@ -.. Copyright (C) Internet Systems Consortium, Inc. ("ISC") -.. -.. SPDX-License-Identifier: MPL-2.0 -.. -.. 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 https://mozilla.org/MPL/2.0/. -.. -.. See the COPYRIGHT file distributed with this work for additional -.. information regarding copyright ownership. - -Notes for BIND 9.19.21 ----------------------- - -Security Fixes -~~~~~~~~~~~~~~ - -- Validating DNS messages containing a lot of DNSSEC signatures could - cause excessive CPU load, leading to a denial-of-service condition. - This has been fixed. :cve:`2023-50387` - - ISC would like to thank Elias Heftrig, Haya Schulmann, Niklas Vogel, - and Michael Waidner from the German National Research Center for - Applied Cybersecurity ATHENE. :gl:`#4424` - -Known Issues -~~~~~~~~~~~~ - -- There are no new known issues with this release. See :ref:`above - ` for a list of all known issues affecting this - BIND 9 branch. From 4dd683526d1c49789bfc6a1296dbd62a737bd3a8 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 2 Feb 2024 10:55:35 +0100 Subject: [PATCH 24/25] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 5a59e438b9..09cb361587 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.19.21 released --- + 6322. [security] Specific DNS answers could cause a denial-of-service condition due to DNS validation taking a long time. (CVE-2023-50387) [GL #4424] From c030a677aec54ab32cfa96a2edf6d77c6099e6ce Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 2 Feb 2024 10:56:43 +0100 Subject: [PATCH 25/25] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index ae7d41159b..512cb83a78 100644 --- a/configure.ac +++ b/configure.ac @@ -16,7 +16,7 @@ # m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 19)dnl -m4_define([bind_VERSION_PATCH], 20)dnl +m4_define([bind_VERSION_PATCH], 21)dnl m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Development Release)])dnl m4_define([bind_SRCID], [m4_esyscmd_s([git rev-parse --short HEAD | cut -b1-7])])dnl