From f46a81932fd30ba19598682ddb0f3b9fa0e9b1dd Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 29 Jun 2023 09:15:38 +1000 Subject: [PATCH 01/40] Address race in dns_tsigkey_find() Restart the process with a write lock if we discover an expired key while holding the read lock. (cherry picked from commit d2ba96488ea30eaeb225c12ea77e71de399b6175) --- lib/dns/tsig.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c index 857ec4cfcd..8f96008e31 100644 --- a/lib/dns/tsig.c +++ b/lib/dns/tsig.c @@ -1757,8 +1757,9 @@ isc_result_t dns_tsigkey_find(dns_tsigkey_t **tsigkey, const dns_name_t *name, const dns_name_t *algorithm, dns_tsig_keyring_t *ring) { dns_tsigkey_t *key; - isc_stdtime_t now; isc_result_t result; + isc_rwlocktype_t locktype = isc_rwlocktype_read; + isc_stdtime_t now; REQUIRE(tsigkey != NULL); REQUIRE(*tsigkey == NULL); @@ -1770,25 +1771,30 @@ dns_tsigkey_find(dns_tsigkey_t **tsigkey, const dns_name_t *name, RWUNLOCK(&ring->lock, isc_rwlocktype_write); isc_stdtime_get(&now); - RWLOCK(&ring->lock, isc_rwlocktype_read); + +again: + RWLOCK(&ring->lock, locktype); key = NULL; result = dns_rbt_findname(ring->keys, name, 0, NULL, (void *)&key); if (result == DNS_R_PARTIALMATCH || result == ISC_R_NOTFOUND) { - RWUNLOCK(&ring->lock, isc_rwlocktype_read); + RWUNLOCK(&ring->lock, locktype); return (ISC_R_NOTFOUND); } 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; + goto again; + } remove_fromring(key); - RWUNLOCK(&ring->lock, isc_rwlocktype_write); + RWUNLOCK(&ring->lock, locktype); return (ISC_R_NOTFOUND); } #if 0 @@ -1803,7 +1809,7 @@ dns_tsigkey_find(dns_tsigkey_t **tsigkey, const dns_name_t *name, } #endif /* if 0 */ isc_refcount_increment(&key->refs); - RWUNLOCK(&ring->lock, isc_rwlocktype_read); + RWUNLOCK(&ring->lock, locktype); adjust_lru(key); *tsigkey = key; return (ISC_R_SUCCESS); From 0a8367e17b28a772bdff39922e2e19747263b654 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 29 Jun 2023 09:27:29 +1000 Subject: [PATCH 02/40] Add CHANGES note for [GL #4182] (cherry picked from commit a62cda787ffdced3fc2ae31d8da86cba17a3f95f) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 6faa12370c..d209c80678 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6314. [bug] Address race conditions in dns_tsigkey_find(). + [GL #4182] + 6312. [bug] Conversion from NSEC3 signed to NSEC signed could temporarily put the zone into a state where it was treated as unsigned until the NSEC chain was built. From 849c05adf43870ddcdc816ee7f6159e06e843d21 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/40] Use hashtable when parsing a message When parsing messages use a hashtable 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 hashtables: 1) hashtable 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 hashtable - for each name in the section, we construct a new hashtable for that name if there are more than one rdataset for that particular name. (cherry picked from commit b8a96317544c7b310b4f74360825a87b6402ddc2) --- lib/dns/include/dns/message.h | 38 ---- lib/dns/include/dns/name.h | 37 ++-- lib/dns/message.c | 355 ++++++++++++++++++++++------------ lib/dns/name.c | 1 + 4 files changed, 250 insertions(+), 181 deletions(-) diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index 940c9b1748..f15884a183 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -856,44 +856,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_movename(dns_message_t *msg, dns_name_t *name, - dns_section_t fromsection, dns_section_t tosection); -/*%< - * Move a name from one section to another. - * - * Requires: - * - *\li 'msg' be valid. - * - *\li 'name' must be a name already in 'fromsection'. - * - *\li 'fromsection' must be a valid section. - * - *\li 'tosection' must be a valid section. - */ - 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 a758c4d948..199856aae7 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. */ @@ -111,6 +112,7 @@ struct dns_name { isc_buffer_t *buffer; ISC_LINK(dns_name_t) link; ISC_LIST(dns_rdataset_t) list; + isc_ht_t *ht; }; #define DNS_NAME_MAGIC ISC_MAGIC('D', 'N', 'S', 'n') @@ -166,30 +168,24 @@ extern const dns_name_t *dns_wildcardname; * unsigned char offsets[] = { 0, 6 }; * dns_name_t value = DNS_NAME_INITABSOLUTE(data, offsets); */ -#define DNS_NAME_INITNONABSOLUTE(A, B) \ - { \ - DNS_NAME_MAGIC, A, (sizeof(A) - 1), sizeof(B), \ - DNS_NAMEATTR_READONLY, B, NULL, \ - { (void *)-1, (void *)-1 }, { \ - NULL, NULL \ - } \ +#define DNS_NAME_INITNONABSOLUTE(A, B) \ + { \ + DNS_NAME_MAGIC, A, (sizeof(A) - 1), sizeof(B), \ + DNS_NAMEATTR_READONLY, B, NULL, \ + { (void *)-1, (void *)-1 }, { NULL, NULL }, NULL \ } -#define DNS_NAME_INITABSOLUTE(A, B) \ - { \ - DNS_NAME_MAGIC, A, sizeof(A), sizeof(B), \ - DNS_NAMEATTR_READONLY | DNS_NAMEATTR_ABSOLUTE, B, \ - NULL, { (void *)-1, (void *)-1 }, { \ - NULL, NULL \ - } \ +#define DNS_NAME_INITABSOLUTE(A, B) \ + { \ + DNS_NAME_MAGIC, A, sizeof(A), sizeof(B), \ + DNS_NAMEATTR_READONLY | DNS_NAMEATTR_ABSOLUTE, B, \ + NULL, { (void *)-1, (void *)-1 }, { NULL, NULL }, NULL \ } -#define DNS_NAME_INITEMPTY \ - { \ - DNS_NAME_MAGIC, NULL, 0, 0, 0, NULL, NULL, \ - { (void *)-1, (void *)-1 }, { \ - NULL, NULL \ - } \ +#define DNS_NAME_INITEMPTY \ + { \ + DNS_NAME_MAGIC, NULL, 0, 0, 0, NULL, NULL, \ + { (void *)-1, (void *)-1 }, { NULL, NULL }, NULL \ } /*% @@ -1330,6 +1326,7 @@ ISC_LANG_ENDDECLS _n->buffer = NULL; \ ISC_LINK_INIT(_n, link); \ ISC_LIST_INIT(_n->list); \ + _n->ht = NULL; \ } while (0) #define DNS_NAME_RESET(n) \ diff --git a/lib/dns/message.c b/lib/dns/message.c index 761a8e1471..cc42b01ae0 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -22,6 +22,8 @@ #include #include +#include +#include #include #include #include @@ -493,9 +495,11 @@ msgresetsigs(dns_message_t *msg, bool replying) { } else { dns_rdataset_disassociate(msg->tsig); isc_mempool_put(msg->rdspool, msg->tsig); + msg->tsig = NULL; if (msg->querytsig != NULL) { dns_rdataset_disassociate(msg->querytsig); isc_mempool_put(msg->rdspool, msg->querytsig); + msg->querytsig = NULL; } } dns_message_puttempname(msg, &msg->tsigname); @@ -790,6 +794,18 @@ dns_message_detach(dns_message_t **messagep) { } } +static isc_result_t +name_hash_add(isc_ht_t *ht, dns_name_t *name, dns_name_t **foundp) { + isc_result_t result = isc_ht_find(ht, name->ndata, name->length, + (void **)foundp); + if (result == ISC_R_SUCCESS) { + return (ISC_R_EXISTS); + } + result = isc_ht_add(ht, name->ndata, name->length, (void *)name); + INSIST(result == ISC_R_SUCCESS); + return (ISC_R_SUCCESS); +} + static isc_result_t findname(dns_name_t **foundname, const dns_name_t *target, dns_namelist_t *section) { @@ -809,29 +825,26 @@ 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 **rdataset) { - dns_rdataset_t *curr; +typedef struct __attribute__((__packed__)) rds_key { + dns_rdataclass_t rdclass; + dns_rdatatype_t type; + dns_rdatatype_t covers; +} rds_key_t; - REQUIRE(name != NULL); - REQUIRE(rdataset == NULL || *rdataset == NULL); - - for (curr = ISC_LIST_TAIL(name->list); curr != NULL; - curr = ISC_LIST_PREV(curr, link)) - { - if (curr->rdclass == rdclass && curr->type == type && - curr->covers == covers) - { - if (rdataset != NULL) { - *rdataset = curr; - } - return (ISC_R_SUCCESS); - } +static isc_result_t +rds_hash_add(isc_ht_t *ht, dns_rdataset_t *rds, dns_rdataset_t **foundp) { + rds_key_t key = { .rdclass = rds->rdclass, + .type = rds->type, + .covers = rds->covers }; + isc_result_t result = isc_ht_find(ht, (const unsigned char *)&key, + sizeof(key), (void **)foundp); + if (result == ISC_R_SUCCESS) { + return (ISC_R_EXISTS); } - - return (ISC_R_NOTFOUND); + result = isc_ht_add(ht, (const unsigned char *)&key, sizeof(key), + (void *)rds); + INSIST(result == ISC_R_SUCCESS); + return (ISC_R_SUCCESS); } isc_result_t @@ -958,6 +971,18 @@ 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; + for (name = ISC_LIST_HEAD(*section); name != NULL; + name = ISC_LIST_NEXT(name, link)) + { + if (name->ht != NULL) { + isc_ht_destroy(&name->ht); + } + } +} + static isc_result_t getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, unsigned int options) { @@ -967,13 +992,19 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, dns_name_t *name2 = 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_ht = false; + isc_ht_t *name_map = NULL; + + if (msg->counts[DNS_SECTION_QUESTION] > 1) { + isc_ht_init(&name_map, msg->mctx, 1, ISC_HT_CASE_INSENSITIVE); + } for (count = 0; count < msg->counts[DNS_SECTION_QUESTION]; count++) { name = NULL; @@ -994,13 +1025,19 @@ 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 = name_hash_add(name_map, name, &name2); /* * If it is the first name in the section, accept it. @@ -1012,19 +1049,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; + break; + default: + UNREACHABLE(); } + free_name = false; + /* * Get type and class. */ @@ -1054,14 +1097,6 @@ 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. */ @@ -1071,6 +1106,7 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, goto cleanup; } rdataset = isc_mempool_get(msg->rdspool); + dns_rdataset_init(rdataset); /* * Convert rdatalist to rdataset, and attach the latter to @@ -1078,8 +1114,6 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, */ rdatalist->type = rdtype; rdatalist->rdclass = rdclass; - - dns_rdataset_init(rdataset); result = dns_rdatalist_tordataset(rdatalist, rdataset); if (result != ISC_R_SUCCESS) { goto cleanup; @@ -1087,14 +1121,46 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, 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->ht == NULL) { + isc_ht_init(&name->ht, msg->mctx, 1, + ISC_HT_CASE_SENSITIVE); + free_ht = true; + + INSIST(ISC_LIST_HEAD(name->list) == + ISC_LIST_TAIL(name->list)); + + dns_rdataset_t *old_rdataset = + ISC_LIST_HEAD(name->list); + + result = rds_hash_add(name->ht, old_rdataset, NULL); + + INSIST(result == ISC_R_SUCCESS); + } + result = rds_hash_add(name->ht, 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) { @@ -1105,6 +1171,14 @@ cleanup: dns_message_puttempname(msg, &name); } + if (free_ht) { + cleanup_name_hashmaps(section); + } + + if (name_map != NULL) { + isc_ht_destroy(&name_map); + } + return (result); } @@ -1184,17 +1258,24 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, dns_name_t *name = NULL; dns_name_t *name2 = 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_ht = false; bool preserve_order = ((options & DNS_MESSAGEPARSE_PRESERVEORDER) != 0); bool best_effort = ((options & DNS_MESSAGEPARSE_BESTEFFORT) != 0); bool isedns, issigzero, istsig; + isc_ht_t *name_map = NULL; + + if (msg->counts[sectionid] > 1) { + isc_ht_init(&name_map, msg->mctx, 1, ISC_HT_CASE_INSENSITIVE); + } for (count = 0; count < msg->counts[sectionid]; count++) { int recstart = source->current; @@ -1202,7 +1283,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; @@ -1245,8 +1325,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; } @@ -1445,61 +1525,124 @@ 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 = name_hash_add(name_map, name, &name2); /* * If it is a new name, append to the section. */ - if (result == ISC_R_SUCCESS) { + 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 = name2; - } else { - ISC_LIST_APPEND(*section, name, link); + name2 = 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); + RUNTIME_CHECK(dns_rdatalist_tordataset(rdatalist, rdataset) == + ISC_R_SUCCESS); + 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->ht == NULL) { + isc_ht_init(&name->ht, msg->mctx, 1, + ISC_HT_CASE_SENSITIVE); + free_ht = true; + + INSIST(ISC_LIST_HEAD(name->list) == + ISC_LIST_TAIL(name->list)); + + dns_rdataset_t *old_rdataset = + ISC_LIST_HEAD(name->list); + + result = rds_hash_add(name->ht, old_rdataset, + NULL); + + INSIST(result == ISC_R_SUCCESS); + } + found_rdataset = NULL; + result = rds_hash_add(name->ht, rdataset, + &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_rdataset_disassociate(rdataset); + isc_mempool_put(msg->rdspool, 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); @@ -1508,33 +1651,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); - RUNTIME_CHECK( - dns_rdatalist_tordataset(rdatalist, rdataset) == - ISC_R_SUCCESS); - 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(); } } @@ -1570,7 +1692,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); @@ -1582,7 +1703,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; @@ -1593,7 +1713,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, */ msg->tsigname->attributes |= DNS_NAMEATTR_NOCOMPRESS; rdataset = NULL; - free_rdataset = false; free_name = false; } @@ -1601,13 +1720,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) { - isc_mempool_put(msg->rdspool, rdataset); - } - free_name = free_rdataset = false; + free_name = false; } INSIST(!free_name); - INSIST(!free_rdataset); + + rdataset = NULL; } /* @@ -1625,16 +1742,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) { - isc_mempool_put(msg->rdspool, rdataset); + + if (free_ht) { + cleanup_name_hashmaps(section); + } + + if (name_map != NULL) { + isc_ht_destroy(&name_map); } return (result); @@ -2452,7 +2573,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; /* @@ -2499,22 +2620,6 @@ dns_message_findname(dns_message_t *msg, dns_section_t section, return (result); } -void -dns_message_movename(dns_message_t *msg, dns_name_t *name, - dns_section_t fromsection, dns_section_t tosection) { - REQUIRE(msg != NULL); - REQUIRE(msg->from_to_wire == DNS_MESSAGE_INTENTRENDER); - REQUIRE(name != NULL); - REQUIRE(VALID_NAMED_SECTION(fromsection)); - REQUIRE(VALID_NAMED_SECTION(tosection)); - - /* - * Unlink the name from the old section - */ - ISC_LIST_UNLINK(msg->sections[fromsection], name, link); - ISC_LIST_APPEND(msg->sections[tosection], name, link); -} - void dns_message_addname(dns_message_t *msg, dns_name_t *name, dns_section_t section) { @@ -2591,6 +2696,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->ht != NULL) { + isc_ht_destroy(&item->ht); + } + /* * we need to check this in case dns_name_dup() was used. */ diff --git a/lib/dns/name.c b/lib/dns/name.c index 8a258a2a2a..90044ba51c 100644 --- a/lib/dns/name.c +++ b/lib/dns/name.c @@ -188,6 +188,7 @@ dns_name_invalidate(dns_name_t *name) { name->offsets = NULL; name->buffer = NULL; ISC_LINK_INIT(name, link); + INSIST(name->ht == NULL); } bool From 968d0a1415bf566d76e264cbc0654061b4c7c267 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/40] Add CHANGES and release note for [GL #4234] (cherry picked from commit 30d27928cff8a82774131b401c26b171a2367e31) --- CHANGES | 3 +++ doc/notes/notes-current.rst | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index d209c80678..c537622cb8 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +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 be1bc21a15..045c3c79db 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -15,7 +15,13 @@ Notes for BIND 9.18.22 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 b42b1fe0519c9da5276ec7d3508ca328519080e3 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 10 Oct 2023 10:58:18 +1100 Subject: [PATCH 05/40] 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. (cherry picked from commit 9d0fa07c5e7a39db89862a4f843d2190059afb4b) --- 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 c1e91484cf..61749c873b 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -465,10 +465,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); @@ -7718,8 +7718,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"); @@ -7795,7 +7794,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); } @@ -9612,11 +9611,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"); @@ -9625,7 +9623,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); } @@ -9713,7 +9711,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"); @@ -9754,7 +9752,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; @@ -10415,7 +10413,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 121473e8b53e53d57decb99524c125dd71f6b6bc Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 10 Oct 2023 11:09:11 +1100 Subject: [PATCH 06/40] Add CHANGES note for [GL #4281] (cherry picked from commit 0748965b7ce7b80ff0062c7ffc661d52335c780e) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index c537622cb8..ead9b742d6 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +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 349d9d3feea2025571ce77fcdee7277b25e167a7 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 10 Oct 2023 11:12:24 +1100 Subject: [PATCH 07/40] Add release note for [GL #4281] (cherry picked from commit 2fbafc2675d19136993980765c0589da599f8403) --- 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 045c3c79db..a729f95ed3 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 f7e137f321359b99570d5451705759c4fe39aad4 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 12 Oct 2023 12:01:46 +1100 Subject: [PATCH 08/40] 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. (cherry picked from commit 1fcc483df13e049b96f620e515f0d4d45f3680b7) --- lib/ns/query.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/ns/query.c b/lib/ns/query.c index 61749c873b..40e1232391 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6228,6 +6228,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 67b93470574d9dd5970f1d80077b05bfc4b187a2 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 12 Oct 2023 13:39:57 +1100 Subject: [PATCH 09/40] Add CHANGES note for [GL #4334] (cherry picked from commit 26671f8c478a66296ee5874adbe741c890e435d1) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index ead9b742d6..eecf973099 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +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 b8188210871e1f6a8ce07b3d0541686d08b5378e Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 12 Oct 2023 13:42:38 +1100 Subject: [PATCH 10/40] Add release note for [GL #4334] (cherry picked from commit c4faf5c69f7eba3b23b8a932e66fc89ec3bee4a9) --- 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 a729f95ed3..24f3a3c331 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 a9af1ac5ae67d266a913ba8a7118ac6f47723d95 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/40] Limit isc_task_send() overhead for tree pruning Instead of issuing a separate isc_task_send() 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_task_send() 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. (cherry picked from commit 24381cc36d8528f5a4046fb2614451aeac4cdfc1) --- lib/dns/include/dns/rbt.h | 6 ++ lib/dns/rbt.c | 1 + lib/dns/rbtdb.c | 149 +++++++++++++++++++++++++------------- 3 files changed, 106 insertions(+), 50 deletions(-) diff --git a/lib/dns/include/dns/rbt.h b/lib/dns/include/dns/rbt.h index c144f83b49..40bf09ee77 100644 --- a/lib/dns/include/dns/rbt.h +++ b/lib/dns/include/dns/rbt.h @@ -124,6 +124,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 4d8c142fc2..29f19c895b 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -1576,6 +1576,7 @@ create_node(isc_mem_t *mctx, const dns_name_t *name, dns_rbtnode_t **nodep) { HASHVAL(node) = 0; ISC_LINK_INIT(node, deadlink); + ISC_LINK_INIT(node, prunelink); LOCKNUM(node) = 0; WILD(node) = 0; diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 7793be859e..b09d97ff64 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -494,6 +494,10 @@ struct dns_rbtdb { */ rbtnodelist_t *deadnodes; + /* List of nodes from which recursive tree pruning can be started from. + * Locked by tree_lock. */ + 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 @@ -1003,6 +1007,7 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log, isc_event_t *event) { unsigned int i; isc_result_t result; char buf[DNS_NAME_FORMATSIZE]; + dns_rbtnode_t *node = NULL; dns_rbt_t **treep; isc_time_t start; @@ -1028,8 +1033,6 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log, isc_event_t *event) { * the overhead of unlinking all nodes here should be negligible. */ for (i = 0; i < rbtdb->node_lock_count; i++) { - dns_rbtnode_t *node; - node = ISC_LIST_HEAD(rbtdb->deadnodes[i]); while (node != NULL) { ISC_LIST_UNLINK(rbtdb->deadnodes[i], node, deadlink); @@ -1037,6 +1040,12 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log, isc_event_t *event) { } } + node = ISC_LIST_HEAD(rbtdb->prunenodes); + while (node != NULL) { + ISC_LIST_UNLINK(rbtdb->prunenodes, node, prunelink); + node = ISC_LIST_HEAD(rbtdb->prunenodes); + } + if (event == NULL) { rbtdb->quantum = (rbtdb->task != NULL) ? 100 : 0; } @@ -1842,19 +1851,32 @@ 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) { - isc_event_t *ev; - dns_db_t *db; + bool pruning_queued = (ISC_LIST_HEAD(rbtdb->prunenodes) != NULL); + + INSIST(locktype == isc_rwlocktype_write); - ev = isc_event_allocate(rbtdb->common.mctx, NULL, DNS_EVENT_RBTPRUNE, - prune_tree, node, sizeof(isc_event_t)); new_reference(rbtdb, node, locktype); - db = NULL; - attach((dns_db_t *)rbtdb, &db); - ev->ev_sender = db; - isc_task_send(rbtdb->task, &ev); + INSIST(!ISC_LINK_LINKED(node, prunelink)); + ISC_LIST_APPEND(rbtdb->prunenodes, node, prunelink); + + if (!pruning_queued) { + isc_event_t *ev = NULL; + dns_db_t *db = NULL; + + attach((dns_db_t *)rbtdb, &db); + + ev = isc_event_allocate(rbtdb->common.mctx, NULL, + DNS_EVENT_RBTPRUNE, prune_tree, db, + sizeof(isc_event_t)); + isc_task_send(rbtdb->task, &ev); + } } /*% @@ -2129,17 +2151,26 @@ 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(isc_task_t *task, isc_event_t *event) { - dns_rbtdb_t *rbtdb = event->ev_sender; - dns_rbtnode_t *node = event->ev_arg; - dns_rbtnode_t *parent; + dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)event->ev_arg; + dns_rbtnode_t *node = NULL; + dns_rbtnode_t *parent = NULL; unsigned int locknum; UNUSED(task); @@ -2147,44 +2178,60 @@ prune_tree(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); RWLOCK(&rbtdb->tree_lock, isc_rwlocktype_write); - locknum = node->locknum; - NODE_LOCK(&rbtdb->node_locks[locknum].lock, isc_rwlocktype_write); - do { - parent = node->parent; - decrement_reference(rbtdb, node, 0, isc_rwlocktype_write, - isc_rwlocktype_write, true); - 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, - isc_rwlocktype_write); - locknum = parent->locknum; - NODE_LOCK(&rbtdb->node_locks[locknum].lock, - isc_rwlocktype_write); + while ((node = ISC_LIST_HEAD(rbtdb->prunenodes)) != NULL) { + locknum = node->locknum; + NODE_LOCK(&rbtdb->node_locks[locknum].lock, + isc_rwlocktype_write); + 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; + decrement_reference(rbtdb, node, 0, + isc_rwlocktype_write, + isc_rwlocktype_write, true); + + 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, + isc_rwlocktype_write); + locknum = parent->locknum; + NODE_LOCK( + &rbtdb->node_locks[locknum].lock, + isc_rwlocktype_write); + } + + /* + * 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); + } + new_reference(rbtdb, parent, + isc_rwlocktype_write); + } else { + parent = NULL; } - new_reference(rbtdb, parent, isc_rwlocktype_write); - } else { - parent = NULL; - } - node = parent; - } while (node != NULL); - NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, isc_rwlocktype_write); + node = parent; + } while (node != NULL); + NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, + isc_rwlocktype_write); + } RWUNLOCK(&rbtdb->tree_lock, isc_rwlocktype_write); detach((dns_db_t **)&rbtdb); @@ -8326,6 +8373,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++) { From 4ddf9fa5089635d62d84eb041548ca28a4ff6735 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/40] Add CHANGES entry for GL #4383 (cherry picked from commit 04df558d57105ccfb9853ba892021c9de01d30c9) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index eecf973099..749d50e2ba 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6319. [func] Limit isc_task_send() overhead for RBTDB tree pruning. + [GL #4383] + 6317. [security] Restore DNS64 state when handling a serve-stale timeout. (CVE-2023-5679) [GL #4334] From 8c875b2f1b0fa1e2ebc608b5628bef4ac0582589 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:55:58 +0100 Subject: [PATCH 13/40] Prepare release notes for BIND 9.18.22 --- doc/arm/notes.rst | 2 +- .../{notes-current.rst => notes-9.18.22.rst} | 15 --------------- 2 files changed, 1 insertion(+), 16 deletions(-) rename doc/notes/{notes-current.rst => notes-9.18.22.rst} (93%) diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index 1caec1b103..aa92b769d0 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -35,7 +35,7 @@ information about each release, and source code. .. include:: ../notes/notes-known-issues.rst -.. include:: ../notes/notes-current.rst +.. include:: ../notes/notes-9.18.22.rst .. include:: ../notes/notes-9.18.21.rst .. include:: ../notes/notes-9.18.20.rst .. include:: ../notes/notes-9.18.19.rst diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-9.18.22.rst similarity index 93% rename from doc/notes/notes-current.rst rename to doc/notes/notes-9.18.22.rst index 24f3a3c331..2695a10e37 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-9.18.22.rst @@ -32,21 +32,6 @@ Security Fixes features were enabled. This has been fixed. :cve:`2023-5679` :gl:`#4334` -New Features -~~~~~~~~~~~~ - -- None. - -Removed Features -~~~~~~~~~~~~~~~~ - -- None. - -Feature Changes -~~~~~~~~~~~~~~~ - -- None. - Bug Fixes ~~~~~~~~~ From 69014521177009257ff5de13195dc19aa56e47e3 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:55:58 +0100 Subject: [PATCH 14/40] Tweak and reword release notes --- doc/notes/notes-9.18.22.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/notes/notes-9.18.22.rst b/doc/notes/notes-9.18.22.rst index 2695a10e37..c2d3375b4f 100644 --- a/doc/notes/notes-9.18.22.rst +++ b/doc/notes/notes-9.18.22.rst @@ -35,9 +35,9 @@ Security Fixes 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 0eb83555ea5a22c52929eb26269b55fe551bee6c 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/40] 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. (cherry picked from commit 0fec404c64cf367b9c2d676535c50f228355b6b5) --- 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 791cdac4108864c06a5b63af05b8313b6dec5e65 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 5 Jan 2024 14:45:55 +0100 Subject: [PATCH 16/40] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 749d50e2ba..b37eb34e0c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.18.22 released --- + 6319. [func] Limit isc_task_send() overhead for RBTDB tree pruning. [GL #4383] From 0a6472be2910a9a0ea4800f2fd0687e30d41eacf Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 5 Jan 2024 14:46:13 +0100 Subject: [PATCH 17/40] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 43dde091b6..e87c598123 100644 --- a/configure.ac +++ b/configure.ac @@ -17,7 +17,7 @@ m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 18)dnl m4_define([bind_VERSION_PATCH], 22)dnl -m4_define([bind_VERSION_EXTRA], -dev)dnl +m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Extended Support Version)])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 7b390a7fb6ca10b60147b72acec0e951da251a7d Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 17 Jan 2024 15:40:56 +0200 Subject: [PATCH 18/40] Fix reading extra messages in TLS DNS in client mode When connecting to a remote party the TLS DNS code could process more than one message at a time despite the fact that it is expected that we should stop after every DNS message. Every DNS message is handled and consumed from the input buffer by isc__nm_process_sock_buffer(). However, as opposed to TCP DNS code, it can be called more than once when processing incoming data from a server (see tls_cycle_input()). That, in turn means that we can process more than one message at a time. Some higher level code might not expect that, as it breaks the contract. In particular, in the original report that happened during isc__nm_async_tlsdnsshutdown() call: when shutting down multiple calls to tls_cycle() are possible (each possibly leading to a isc__nm_process_sock_buffer()). If there are any non processed messages left, for any of the messages left the read callback will be called even when it is not expected as there were no preceding isc_nm_read(). To keep TCP DNS and TLS DNS code in sync, we make a similar change to it as well, although it should not matter. --- lib/isc/netmgr/tcpdns.c | 7 +++++++ lib/isc/netmgr/tlsdns.c | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index eda6aa62ce..cabc90533d 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -808,6 +808,13 @@ isc__nm_tcpdns_processbuffer(isc_nmsocket_t *sock) { return (ISC_R_CANCELED); } + if (sock->client && !sock->recv_read) { + /* + * We are not reading data - stop here. + */ + return (ISC_R_CANCELED); + } + req = isc__nm_get_read_req(sock, NULL); REQUIRE(VALID_UVREQ(req)); diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index d30e33fbfd..cfc62eb5e9 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1016,6 +1016,13 @@ isc__nm_tlsdns_processbuffer(isc_nmsocket_t *sock) { return (ISC_R_CANCELED); } + if (sock->client && !sock->recv_read) { + /* + * We are not reading data - stop here. + */ + return (ISC_R_CANCELED); + } + req = isc__nm_get_read_req(sock, NULL); REQUIRE(VALID_UVREQ(req)); From a15c5b168766f5b6de956fed50d9c153eb94a1b9 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 18 Jan 2024 22:51:42 +0200 Subject: [PATCH 19/40] Add a unit test which would fail on excessive reads This commit adds a unit tests which would fail/crash/abort if excessive reads were possible. See [GL #4487] --- tests/isc/netmgr_test.c | 172 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/tests/isc/netmgr_test.c b/tests/isc/netmgr_test.c index f75207a223..49e5e01722 100644 --- a/tests/isc/netmgr_test.c +++ b/tests/isc/netmgr_test.c @@ -2401,6 +2401,176 @@ ISC_RUN_TEST_IMPL(tlsdns_recv_one) { atomic_assert_int_eq(ssends, 0); } +static void +tlsdns_many_listen_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg) { + uint64_t magic = 0; + isc_nmhandle_t *sendhandle = NULL; + isc_buffer_t *send_data = (isc_buffer_t *)cbarg; + isc_region_t send_messages = { 0 }; + + assert_non_null(handle); + assert_non_null(send_data); + + F(); + + if (eresult != ISC_R_SUCCESS) { + goto unref; + } + + atomic_fetch_add(&sreads, 1); + + assert_true(region->length >= sizeof(magic)); + + memmove(&magic, region->base + sizeof(uint16_t), sizeof(magic)); + assert_true(magic == stop_magic || magic == send_magic); + + isc_nmhandle_attach(handle, &sendhandle); + isc_refcount_increment0(&active_ssends); + isc_nmhandle_setwritetimeout(sendhandle, T_IDLE); + /* send multiple DNS messages at once */ + isc_buffer_usedregion(send_data, &send_messages); + isc_nm_send(sendhandle, &send_messages, listen_send_cb, cbarg); +unref: + isc_refcount_decrement(&active_sreads); + isc_nmhandle_detach(&handle); +} + +static isc_result_t +tlsdns_many_listen_accept_cb(isc_nmhandle_t *handle, isc_result_t eresult, + void *cbarg) { + isc_nmhandle_t *readhandle = NULL; + + UNUSED(cbarg); + + F(); + + if (eresult != ISC_R_SUCCESS) { + return (eresult); + } + + atomic_fetch_add(&saccepts, 1); + + isc_refcount_increment0(&active_sreads); + isc_nmhandle_attach(handle, &readhandle); + isc_nm_read(handle, tlsdns_many_listen_read_cb, cbarg); + + return (ISC_R_SUCCESS); +} + +static void +tlsdns_many_connect_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg) { + isc_nmhandle_t *sendhandle = NULL; + uint64_t magic = 0; + + UNUSED(cbarg); + + assert_non_null(handle); + + F(); + + if (eresult != ISC_R_SUCCESS) { + goto unref; + } + + assert_true(region->length >= sizeof(magic)); + + atomic_fetch_add(&creads, 1); + + memmove(&magic, region->base, sizeof(magic)); + + assert_true(magic == stop_magic || magic == send_magic); + + isc_refcount_increment0(&active_csends); + isc_nmhandle_attach(handle, &sendhandle); + isc_nmhandle_setwritetimeout(handle, T_IDLE); + /* + * At this point the read is completed, so we should stop that - + * but the sending code will make a cycling through input + * attempt. When not properly handled, this situation will cause + * excessive reads. + */ + isc_nm_send(sendhandle, &send_msg, connect_send_cb, NULL); + +unref: + isc_refcount_decrement(&active_creads); + isc_nmhandle_detach(&handle); +} + +/* + * A unit test *VERY* specific to #4487 - it would crash the unit test + * suite without the related fix due to excessive/unexpected reads. + * + * The intention behind the test is to (needlessly ;-)) prove that the + * author of the fix is not fantasising and excessive reads are + * possible in principle. Also, it proves that there is more than one + * way to do that. + * + * It is *not* reproducing the situation from the bug report 1:1, as + * it is impossible to understand what exactly was going on with this + * custom/proprietary server without having access to it (and even in + * that case the bug was hard to reproduce to the point, where the + * reporters considered it to be fixed for a while). There are far too + * many things a play. + */ +ISC_RUN_TEST_IMPL(tlsdns_server_send_many_recv_one) { + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + uint8_t buf[512]; + isc_buffer_t server_send_buf = { 0 }; + + isc_buffer_init(&server_send_buf, buf, sizeof(buf)); + + /* + * Prepare a buffer with three "DNS" messages which we will send + * at once (our code does not normally do that do that). + */ + isc_buffer_putuint16(&server_send_buf, (uint16_t)send_msg.length); + isc_buffer_putmem(&server_send_buf, send_msg.base, send_msg.length); + isc_buffer_putuint16(&server_send_buf, (uint16_t)send_msg.length); + isc_buffer_putmem(&server_send_buf, send_msg.base, send_msg.length); + isc_buffer_putuint16(&server_send_buf, (uint16_t)send_msg.length); + isc_buffer_putmem(&server_send_buf, send_msg.base, send_msg.length); + + atomic_store(&nsends, 1); + + result = isc_nm_listentls( + listen_nm, &tcp_listen_addr, tlsdns_many_listen_accept_cb, + &server_send_buf, 0, 0, NULL, tcp_listen_tlsctx, &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + connect_readcb = tlsdns_many_connect_read_cb; + isc_refcount_increment0(&active_cconnects); + isc_nm_tlsdnsconnect(connect_nm, &tcp_connect_addr, &tcp_listen_addr, + connect_connect_cb, NULL, T_CONNECT, 0, + tcp_connect_tlsctx, tcp_tlsctx_client_sess_cache); + + WAIT_FOR_EQ(cconnects, 1); + WAIT_FOR_LE(nsends, 0); + WAIT_FOR_EQ(csends, 2); + WAIT_FOR_EQ(sreads, 1); + WAIT_FOR_EQ(ssends, 1); + WAIT_FOR_EQ(creads, 1); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc__netmgr_shutdown(connect_nm); + + X(cconnects); + X(csends); + X(creads); + X(sreads); + X(ssends); + + atomic_assert_int_eq(cconnects, 1); + atomic_assert_int_eq(csends, 2); + atomic_assert_int_eq(creads, 1); + atomic_assert_int_eq(sreads, 1); + atomic_assert_int_eq(ssends, 1); +} + ISC_RUN_TEST_IMPL(tlsdns_recv_two) { isc_result_t result = ISC_R_SUCCESS; isc_nmsocket_t *listen_sock = NULL; @@ -2879,6 +3049,8 @@ ISC_TEST_ENTRY_CUSTOM(tls_half_recv_half_send_quota_sendback, setup_test, /* TLSDNS */ ISC_TEST_ENTRY_CUSTOM(tlsdns_recv_one, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(tlsdns_server_send_many_recv_one, setup_test, + teardown_test) ISC_TEST_ENTRY_CUSTOM(tlsdns_recv_two, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(tlsdns_noop, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(tlsdns_noresponse, setup_test, teardown_test) From 6b9ccae5371be92f3e7e734a06e051853a1ec119 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 17 Jan 2024 16:01:08 +0200 Subject: [PATCH 20/40] Modify CHANGES [GL #4487] Mention that TLS DNS will not process more than one message at a time when that was not expected. --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index b37eb34e0c..6bccc42646 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6320. [bug] Under some circumstances, the DoT code in client + mode could process more than one message at a time when + that was not expected. That has been fixed. [GL #4487] + --- 9.18.22 released --- 6319. [func] Limit isc_task_send() overhead for RBTDB tree pruning. From 6d70ccd1289a747745f859d3255fda423fa297fa Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 16 Jan 2024 15:58:53 -0800 Subject: [PATCH 21/40] 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. (cherry picked from commit 4c19d35614f8cd80d8748156a5bad361e19abc28) --- lib/dns/message.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/dns/message.c b/lib/dns/message.c index cc42b01ae0..d09eb498dd 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -1164,7 +1164,9 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, cleanup: if (rdataset != NULL) { - INSIST(!dns_rdataset_isassociated(rdataset)); + if (dns_rdataset_isassociated(rdataset)) { + dns_rdataset_disassociate(rdataset); + } isc_mempool_put(msg->rdspool, rdataset); } if (free_name) { From cf34bccf4c0e75220edd9f5891aa25444fe6763f Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Fri, 19 Jan 2024 14:26:17 +0000 Subject: [PATCH 22/40] 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. (cherry picked from commit 510f1de8a6add516b842a55750366944701d3d9a) --- lib/dns/message.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/lib/dns/message.c b/lib/dns/message.c index d09eb498dd..8654e92ec3 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -1288,6 +1288,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; result = dns_message_gettempname(msg, &name); @@ -1435,10 +1436,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)) { @@ -1589,7 +1586,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); } @@ -1637,18 +1633,17 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, /* Free the rdataset we used as the key */ dns_rdataset_disassociate(rdataset); isc_mempool_put(msg->rdspool, 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); @@ -1693,7 +1688,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); @@ -1704,7 +1698,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; @@ -1714,9 +1707,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 |= DNS_NAMEATTR_NOCOMPRESS; - rdataset = NULL; free_name = false; } + rdataset = NULL; if (seen_problem) { if (free_name) { @@ -1725,8 +1718,6 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, free_name = false; } INSIST(!free_name); - - rdataset = NULL; } /* @@ -1748,6 +1739,10 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx, } cleanup: + if (rdataset != NULL && rdataset != found_rdataset) { + dns_rdataset_disassociate(rdataset); + isc_mempool_put(msg->rdspool, rdataset); + } if (free_name) { dns_message_puttempname(msg, &name); } From 48324b06d052b0746c85f0bb4047035a5e101a47 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 23/40] Add a CHANGES entry (cherry picked from commit 04ba284e1ace177d095599c8708126093a91336d) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 6bccc42646..c85c39a0b1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6321. [security] Change 6315 inadvertently introduced regressions that + could cause named to crash. [GL #4234] + 6320. [bug] Under some circumstances, the DoT code in client mode could process more than one message at a time when that was not expected. That has been fixed. [GL #4487] From 1b3b0cef224e7a9e8279c5cfe2f7e188e3777cc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 11 Jan 2024 12:03:24 +0100 Subject: [PATCH 24/40] Split fast and slow task queues Change the taskmgr (and thus netmgr) in a way that it supports fast and slow task queues. The fast queue is used for incoming DNS traffic and it will pass the processing to the slow queue for sending outgoing DNS messages and processing resolver messages. In the future, more tasks might get moved to the slow queues, so the cached and authoritative DNS traffic can be handled without being slowed down by operations that take longer time to process. --- lib/dns/resolver.c | 4 ++-- lib/isc/include/isc/netmgr.h | 3 +++ lib/isc/netmgr/http.c | 18 ++++++++--------- lib/isc/netmgr/netmgr-int.h | 1 + lib/isc/netmgr/netmgr.c | 38 +++++++++++++++++++++++------------- lib/isc/netmgr/tcp.c | 6 +++--- lib/isc/netmgr/tcpdns.c | 4 ++-- lib/isc/netmgr/tlsdns.c | 4 ++-- lib/isc/netmgr/tlsstream.c | 12 ++++++------ lib/isc/netmgr/udp.c | 6 +++--- 10 files changed, 55 insertions(+), 41 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 4b3d1c0b40..60cac293cb 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -10408,8 +10408,8 @@ dns_resolver_create(dns_view_t *view, isc_taskmgr_t *taskmgr, * Since we have a pool of tasks we bind them to task * queues to spread the load evenly */ - result = isc_task_create_bound(taskmgr, 0, - &res->buckets[i].task, i); + result = isc_task_create_bound( + taskmgr, 0, &res->buckets[i].task, ISC_NM_TASK_SLOW(i)); if (result != ISC_R_SUCCESS) { ntasks = i; isc_mutex_destroy(&res->buckets[i].lock); diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index eff33f6acb..d42cfe9a20 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -750,6 +750,9 @@ isc_nm_verify_tls_peer_result_string(const isc_nmhandle_t *handle); * \li 'handle' is a valid netmgr handle object. */ +#define ISC_NM_TASK_SLOW_OFFSET -2 +#define ISC_NM_TASK_SLOW(i) (ISC_NM_TASK_SLOW_OFFSET - 1 - i) + void isc_nm_task_enqueue(isc_nm_t *mgr, isc_task_t *task, int threadid); /*%< diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index d7a33d5abe..2220edf364 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -2969,7 +2969,7 @@ isc__nm_http_set_max_streams(isc_nmsocket_t *listener, void isc_nm_http_set_endpoints(isc_nmsocket_t *listener, isc_nm_http_endpoints_t *eps) { - size_t nworkers; + size_t nlisteners; REQUIRE(VALID_NMSOCK(listener)); REQUIRE(listener->type == isc_nm_httplistener); @@ -2977,8 +2977,8 @@ isc_nm_http_set_endpoints(isc_nmsocket_t *listener, atomic_store(&eps->in_use, true); - nworkers = (size_t)listener->mgr->nworkers; - for (size_t i = 0; i < nworkers; i++) { + nlisteners = (size_t)listener->mgr->nlisteners; + for (size_t i = 0; i < nlisteners; i++) { isc__netievent__http_eps_t *ievent = isc__nm_get_netievent_httpendpoints(listener->mgr, listener, eps); @@ -3003,20 +3003,20 @@ isc__nm_async_httpendpoints(isc__networker_t *worker, isc__netievent_t *ev0) { static void http_init_listener_endpoints(isc_nmsocket_t *listener, isc_nm_http_endpoints_t *epset) { - size_t nworkers; + size_t nlisteners; REQUIRE(VALID_NMSOCK(listener)); REQUIRE(VALID_NM(listener->mgr)); REQUIRE(VALID_HTTP_ENDPOINTS(epset)); - nworkers = (size_t)listener->mgr->nworkers; - INSIST(nworkers > 0); + nlisteners = (size_t)listener->mgr->nlisteners; + INSIST(nlisteners > 0); listener->h2.listener_endpoints = isc_mem_get(listener->mgr->mctx, - sizeof(isc_nm_http_endpoints_t *) * nworkers); - listener->h2.n_listener_endpoints = nworkers; - for (size_t i = 0; i < nworkers; i++) { + sizeof(isc_nm_http_endpoints_t *) * nlisteners); + listener->h2.n_listener_endpoints = nlisteners; + for (size_t i = 0; i < nlisteners; i++) { listener->h2.listener_endpoints[i] = NULL; isc_nm_http_endpoints_attach( epset, &listener->h2.listener_endpoints[i]); diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 364a933128..6aca9ab92c 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -776,6 +776,7 @@ struct isc_nm { isc_refcount_t references; isc_mem_t *mctx; int nworkers; + int nlisteners; isc_mutex_t lock; isc_condition_t wkstatecond; isc_condition_t wkpausecond; diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index b19d468820..2310b4b904 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -189,12 +189,12 @@ isc__nm_force_tid(int tid) { } static void -isc__nm_threadpool_initialize(uint32_t workers) { +isc__nm_threadpool_initialize(uint32_t nworkers) { char buf[11]; int r = uv_os_getenv("UV_THREADPOOL_SIZE", buf, &(size_t){ sizeof(buf) }); if (r == UV_ENOENT) { - snprintf(buf, sizeof(buf), "%" PRIu32, workers); + snprintf(buf, sizeof(buf), "%" PRIu32, nworkers); uv_os_setenv("UV_THREADPOOL_SIZE", buf); } } @@ -212,11 +212,11 @@ isc__nm_threadpool_initialize(uint32_t workers) { #endif void -isc__netmgr_create(isc_mem_t *mctx, uint32_t workers, isc_nm_t **netmgrp) { +isc__netmgr_create(isc_mem_t *mctx, uint32_t nworkers, isc_nm_t **netmgrp) { isc_nm_t *mgr = NULL; char name[32]; - REQUIRE(workers > 0); + REQUIRE(nworkers > 0); #ifdef MAXIMAL_UV_VERSION if (uv_version() > MAXIMAL_UV_VERSION) { @@ -234,10 +234,13 @@ isc__netmgr_create(isc_mem_t *mctx, uint32_t workers, isc_nm_t **netmgrp) { uv_version_string(), UV_VERSION_STRING); } - isc__nm_threadpool_initialize(workers); + isc__nm_threadpool_initialize(nworkers); mgr = isc_mem_get(mctx, sizeof(*mgr)); - *mgr = (isc_nm_t){ .nworkers = workers }; + *mgr = (isc_nm_t){ + .nworkers = nworkers * 2, + .nlisteners = nworkers, + }; isc_mem_attach(mctx, &mgr->mctx); isc_mutex_init(&mgr->lock); @@ -272,11 +275,12 @@ isc__netmgr_create(isc_mem_t *mctx, uint32_t workers, isc_nm_t **netmgrp) { atomic_init(&mgr->keepalive, 30000); atomic_init(&mgr->advertised, 30000); - isc_barrier_init(&mgr->pausing, workers); - isc_barrier_init(&mgr->resuming, workers); + isc_barrier_init(&mgr->pausing, mgr->nworkers); + isc_barrier_init(&mgr->resuming, mgr->nworkers); - mgr->workers = isc_mem_get(mctx, workers * sizeof(isc__networker_t)); - for (size_t i = 0; i < workers; i++) { + mgr->workers = isc_mem_get(mctx, + mgr->nworkers * sizeof(isc__networker_t)); + for (int i = 0; i < mgr->nworkers; i++) { isc__networker_t *worker = &mgr->workers[i]; int r; @@ -310,7 +314,7 @@ isc__netmgr_create(isc_mem_t *mctx, uint32_t workers, isc_nm_t **netmgrp) { mgr->workers_running++; isc_thread_create(nm_thread, &mgr->workers[i], &worker->thread); - snprintf(name, sizeof(name), "isc-net-%04zu", i); + snprintf(name, sizeof(name), "isc-net-%04d", i); isc_thread_setname(worker->thread, name); } @@ -817,9 +821,15 @@ isc_nm_task_enqueue(isc_nm_t *nm, isc_task_t *task, int threadid) { isc__networker_t *worker = NULL; if (threadid == -1) { - tid = (int)isc_random_uniform(nm->nworkers); + tid = (int)isc_random_uniform(nm->nlisteners); + } else if (threadid == ISC_NM_TASK_SLOW_OFFSET) { + tid = nm->nlisteners + + (int)isc_random_uniform(nm->nworkers - nm->nlisteners); + } else if (threadid < ISC_NM_TASK_SLOW_OFFSET) { + tid = nm->nlisteners + (ISC_NM_TASK_SLOW(threadid) % + (nm->nworkers - nm->nlisteners)); } else { - tid = threadid % nm->nworkers; + tid = threadid % nm->nlisteners; } worker = &nm->workers[tid]; @@ -3778,7 +3788,7 @@ isc__nm_async_settlsctx(isc__networker_t *worker, isc__netievent_t *ev0) { static void set_tlsctx_workers(isc_nmsocket_t *listener, isc_tlsctx_t *tlsctx) { /* Update the TLS context reference for every worker thread. */ - for (size_t i = 0; i < (size_t)listener->mgr->nworkers; i++) { + for (size_t i = 0; i < (size_t)listener->mgr->nlisteners; i++) { isc__netievent__tlsctx_t *ievent = isc__nm_get_netievent_settlsctx(listener->mgr, listener, tlsctx); diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 2a644fed3f..16b53cc579 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -341,7 +341,7 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc__nm_connectcb(sock, req, result, false); } else { isc__nmsocket_clearcb(sock); - sock->tid = isc_random_uniform(mgr->nworkers); + sock->tid = isc_random_uniform(mgr->nlisteners); isc__nm_connectcb(sock, req, result, true); } atomic_store(&sock->closed, true); @@ -362,7 +362,7 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc__nm_put_netievent_tcpconnect(mgr, ievent); } else { atomic_init(&sock->active, false); - sock->tid = isc_random_uniform(mgr->nworkers); + sock->tid = isc_random_uniform(mgr->nlisteners); isc__nm_enqueue_ievent(&mgr->workers[sock->tid], (isc__netievent_t *)ievent); } @@ -457,7 +457,7 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_sockaddr_t *iface, isc__nmsocket_init(sock, mgr, isc_nm_tcplistener, iface); atomic_init(&sock->rchildren, 0); - sock->nchildren = mgr->nworkers; + sock->nchildren = mgr->nlisteners; children_size = sock->nchildren * sizeof(sock->children[0]); sock->children = isc_mem_get(mgr->mctx, children_size); memset(sock->children, 0, children_size); diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index cabc90533d..b2a0b1016d 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -324,7 +324,7 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc__nm_put_netievent_tcpdnsconnect(mgr, ievent); } else { atomic_init(&sock->active, false); - sock->tid = isc_random_uniform(mgr->nworkers); + sock->tid = isc_random_uniform(mgr->nlisteners); isc__nm_enqueue_ievent(&mgr->workers[sock->tid], (isc__netievent_t *)ievent); } @@ -422,7 +422,7 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_sockaddr_t *iface, isc__nmsocket_init(sock, mgr, isc_nm_tcpdnslistener, iface); atomic_init(&sock->rchildren, 0); - sock->nchildren = mgr->nworkers; + sock->nchildren = mgr->nlisteners; children_size = sock->nchildren * sizeof(sock->children[0]); sock->children = isc_mem_get(mgr->mctx, children_size); memset(sock->children, 0, children_size); diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index cfc62eb5e9..feeb1a8d7d 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -419,7 +419,7 @@ isc_nm_tlsdnsconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc__nm_put_netievent_tlsdnsconnect(mgr, ievent); } else { atomic_init(&sock->active, false); - sock->tid = isc_random_uniform(mgr->nworkers); + sock->tid = isc_random_uniform(mgr->nlisteners); isc__nm_enqueue_ievent(&mgr->workers[sock->tid], (isc__netievent_t *)ievent); } @@ -532,7 +532,7 @@ isc_nm_listentlsdns(isc_nm_t *mgr, isc_sockaddr_t *iface, isc__nmsocket_init(sock, mgr, isc_nm_tlsdnslistener, iface); atomic_init(&sock->rchildren, 0); - sock->nchildren = mgr->nworkers; + sock->nchildren = mgr->nlisteners; children_size = sock->nchildren * sizeof(sock->children[0]); sock->children = isc_mem_get(mgr->mctx, children_size); memset(sock->children, 0, children_size); diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 7b490719bb..a3fc6d203c 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -1264,18 +1264,18 @@ isc__nm_tls_verify_tls_peer_result_string(const isc_nmhandle_t *handle) { static void tls_init_listener_tlsctx(isc_nmsocket_t *listener, isc_tlsctx_t *ctx) { - size_t nworkers; + size_t nlisteners; REQUIRE(VALID_NM(listener->mgr)); REQUIRE(ctx != NULL); - nworkers = (size_t)listener->mgr->nworkers; - INSIST(nworkers > 0); + nlisteners = (size_t)listener->mgr->nlisteners; + INSIST(nlisteners > 0); listener->tlsstream.listener_tls_ctx = isc_mem_get( - listener->mgr->mctx, sizeof(isc_tlsctx_t *) * nworkers); - listener->tlsstream.n_listener_tls_ctx = nworkers; - for (size_t i = 0; i < nworkers; i++) { + listener->mgr->mctx, sizeof(isc_tlsctx_t *) * nlisteners); + listener->tlsstream.n_listener_tls_ctx = nlisteners; + for (size_t i = 0; i < nlisteners; i++) { listener->tlsstream.listener_tls_ctx[i] = NULL; isc_tlsctx_attach(ctx, &listener->tlsstream.listener_tls_ctx[i]); diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index 476c7992f6..661de96ac6 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -157,14 +157,14 @@ isc_nm_listenudp(isc_nm_t *mgr, isc_sockaddr_t *iface, isc_nm_recv_cb_t cb, REQUIRE(VALID_NM(mgr)); /* - * We are creating mgr->nworkers duplicated sockets, one + * We are creating mgr->nlisteners duplicated sockets, one * socket for each worker thread. */ sock = isc_mem_get(mgr->mctx, sizeof(isc_nmsocket_t)); isc__nmsocket_init(sock, mgr, isc_nm_udplistener, iface); atomic_init(&sock->rchildren, 0); - sock->nchildren = mgr->nworkers; + sock->nchildren = mgr->nlisteners; children_size = sock->nchildren * sizeof(sock->children[0]); sock->children = isc_mem_get(mgr->mctx, children_size); memset(sock->children, 0, children_size); @@ -1037,7 +1037,7 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc__nm_put_netievent_udpconnect(mgr, event); } else { atomic_init(&sock->active, false); - sock->tid = isc_random_uniform(mgr->nworkers); + sock->tid = isc_random_uniform(mgr->nlisteners); isc__nm_enqueue_ievent(&mgr->workers[sock->tid], (isc__netievent_t *)event); } From 439e16e4de525599bbb5a31575211d06cc3e2fbb Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 22 Nov 2023 16:59:03 +1100 Subject: [PATCH 25/40] Skip revoked keys when selecting DNSKEY in the validation loop Don't select revoked keys when iterating through DNSKEYs in the DNSSEC validation routines. --- lib/dns/validator.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 56a0ced7b7..7ae0b3c49c 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1144,6 +1144,8 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { (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) { From 0add2934775dcfb05ea6ff9849c8c0c4a65fb009 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 22 Nov 2023 16:59:03 +1100 Subject: [PATCH 26/40] Fail processing incoming DNS message on first validation failure Stop processing the DNS validation when first validation failure occurs in the DNS message. --- lib/dns/include/dns/validator.h | 1 + lib/dns/validator.c | 21 +++++++-------------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 383dcb46e4..352a60a6a0 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -148,6 +148,7 @@ struct dns_validator { unsigned int depth; unsigned int authcount; unsigned int authfail; + bool failed; isc_stdtime_t start; }; diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 7ae0b3c49c..e0c3574040 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1591,20 +1591,9 @@ validate_answer(dns_validator_t *val, bool resume) { continue; } - do { - isc_result_t tresult; - vresult = verify(val, val->key, &rdata, - val->siginfo->keyid); - if (vresult == ISC_R_SUCCESS) { - break; - } - - tresult = select_signing_key(val, val->keyset); - if (tresult != ISC_R_SUCCESS) { - break; - } - } while (1); + vresult = verify(val, val->key, &rdata, val->siginfo->keyid); if (vresult != ISC_R_SUCCESS) { + val->failed = true; validator_log(val, ISC_LOG_DEBUG(3), "failed to verify rdataset"); } else { @@ -1641,9 +1630,13 @@ validate_answer(dns_validator_t *val, bool resume) { } else { validator_log(val, ISC_LOG_DEBUG(3), "verify failure: %s", - isc_result_totext(result)); + isc_result_totext(vresult)); resume = false; } + if (val->failed) { + result = ISC_R_NOMORE; + break; + } } if (result != ISC_R_NOMORE) { validator_log(val, ISC_LOG_DEBUG(3), From 21af5c9a97ed73345799dd4dfec493cc6785e40b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 11 Jan 2024 13:34:46 +0100 Subject: [PATCH 27/40] Don't iterate from start every time we select new signing key Remember the position in the iterator when selecting the next signing key. This should speed up processing for larger DNSKEY RRSets because we don't have to iterate from start over and over again. --- lib/dns/validator.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/lib/dns/validator.c b/lib/dns/validator.c index e0c3574040..4dd170604e 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1104,8 +1104,8 @@ create_validator(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, * 'rdataset'. If found, build a dst_key_t for it and point val->key at * it. * - * If val->key is already non-NULL, locate it in the rdataset and then - * search past it for the *next* key that could have signed 'siginfo', then + * If val->key is already non-NULL, start searching from the next position in + * 'rdataset' to find the *next* key that could have signed 'siginfo', then * set val->key to that. * * Returns ISC_R_SUCCESS if a possible matching key has been found, @@ -1118,19 +1118,18 @@ 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; 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); } - - result = dns_rdataset_first(rdataset); if (result != ISC_R_SUCCESS) { goto failure; } + do { dns_rdataset_current(rdataset, &rdata); @@ -1148,15 +1147,10 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { 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); - } + /* + * This is the key we're looking for. + */ + return (ISC_R_SUCCESS); } dst_key_free(&val->key); } @@ -1164,15 +1158,11 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { result = dns_rdataset_next(rdataset); } while (result == ISC_R_SUCCESS); +failure: if (result == ISC_R_NOMORE) { result = ISC_R_NOTFOUND; } -failure: - if (oldkey != NULL) { - dst_key_free(&oldkey); - } - return (result); } From f39cd17a26959e80b3ff4ce38e98936312395610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 11 Jan 2024 13:58:52 +0100 Subject: [PATCH 28/40] Optimize selecting the signing key Don't parse the crypto data before parsing and matching the id and the algorithm for consecutive DNSKEYs. This allows us to parse the RData only in case the other parameters match allowing us to skip keys that are of no interest to us, but still would consume precious CPU time by parsing possibly garbage with OpenSSL. --- lib/dns/dst_api.c | 27 +++++++++++++++++++-------- lib/dns/include/dst/dst.h | 4 ++++ lib/dns/validator.c | 24 ++++++++++++++++-------- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c index 4ffda8b358..0658c691a8 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); @@ -753,6 +754,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; @@ -783,7 +791,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); } @@ -804,7 +812,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); } @@ -2351,7 +2359,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; @@ -2376,10 +2385,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/dst/dst.h b/lib/dns/include/dst/dst.h index ca292b0ef0..f845e9bd2e 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/validator.c b/lib/dns/validator.c index 4dd170604e..47c48131fb 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1118,6 +1118,7 @@ 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 no_rdata = false; if (oldkey == NULL) { result = dns_rdataset_first(rdataset); @@ -1127,7 +1128,7 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { result = dns_rdataset_next(rdataset); } if (result != ISC_R_SUCCESS) { - goto failure; + goto done; } do { @@ -1136,8 +1137,9 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { 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) && @@ -1147,18 +1149,24 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { 0 && dst_key_iszonekey(val->key)) { - /* - * This is the key we're looking for. - */ - return (ISC_R_SUCCESS); + 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); -failure: +done: if (result == ISC_R_NOMORE) { result = ISC_R_NOTFOUND; } From c847092a17b98e14e1e1efea7ab06ba86595756e 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 29/40] 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 c85c39a0b1..3800e70930 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..dbc1c7bdec --- /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.18.23 +---------------------- + +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 4ac103a6b3665ff16381f1f4363be1318e5657e9 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:22:13 +0100 Subject: [PATCH 30/40] Prepare release notes for BIND 9.18.23 --- doc/arm/notes.rst | 1 + doc/notes/notes-9.18.22.rst | 36 +++--------------------- doc/notes/notes-9.18.23.rst | 56 +++++++++++++++++++++++++++++++++++++ doc/notes/notes-current.rst | 31 -------------------- 4 files changed, 61 insertions(+), 63 deletions(-) create mode 100644 doc/notes/notes-9.18.23.rst delete mode 100644 doc/notes/notes-current.rst diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index aa92b769d0..ce01edacb4 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -35,6 +35,7 @@ information about each release, and source code. .. include:: ../notes/notes-known-issues.rst +.. include:: ../notes/notes-9.18.23.rst .. include:: ../notes/notes-9.18.22.rst .. include:: ../notes/notes-9.18.21.rst .. include:: ../notes/notes-9.18.20.rst diff --git a/doc/notes/notes-9.18.22.rst b/doc/notes/notes-9.18.22.rst index c2d3375b4f..77f374c4cd 100644 --- a/doc/notes/notes-9.18.22.rst +++ b/doc/notes/notes-9.18.22.rst @@ -12,36 +12,8 @@ Notes for BIND 9.18.22 ---------------------- -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` - -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.18.22 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.18.23.rst b/doc/notes/notes-9.18.23.rst new file mode 100644 index 0000000000..e5f1a68cad --- /dev/null +++ b/doc/notes/notes-9.18.23.rst @@ -0,0 +1,56 @@ +.. 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.18.23 +---------------------- + +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` + +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 dbc1c7bdec..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.18.23 ----------------------- - -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 462a9af18227610cc0a9d84dad7ff135d1c4010c 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:22:13 +0100 Subject: [PATCH 31/40] Add release note for GL #4487 --- doc/notes/notes-9.18.23.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/notes/notes-9.18.23.rst b/doc/notes/notes-9.18.23.rst index e5f1a68cad..a74329ac54 100644 --- a/doc/notes/notes-9.18.23.rst +++ b/doc/notes/notes-9.18.23.rst @@ -41,6 +41,11 @@ Security Fixes features were enabled. This has been fixed. :cve:`2023-5679` :gl:`#4334` +- Under certain circumstances, the DNS-over-TLS client code incorrectly + attempted to process more than one DNS message at a time, which could + cause :iscman:`named` to crash with an assertion failure. This has + been fixed. :gl:`#4487` + Bug Fixes ~~~~~~~~~ From 671b8174c883edc470071cb56edec0bdc7adcdbf Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 2 Feb 2024 11:05:35 +0100 Subject: [PATCH 32/40] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 3800e70930..fc595882c0 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.18.23 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 cb49b26a34eee13ecf19d66f57a18540e89f0ab0 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 2 Feb 2024 11:05:55 +0100 Subject: [PATCH 33/40] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index e87c598123..58af49b84e 100644 --- a/configure.ac +++ b/configure.ac @@ -16,7 +16,7 @@ # m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 18)dnl -m4_define([bind_VERSION_PATCH], 22)dnl +m4_define([bind_VERSION_PATCH], 23)dnl m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Extended Support Version)])dnl m4_define([bind_SRCID], [m4_esyscmd_s([git rev-parse --short HEAD | cut -b1-7])])dnl From c462d65b2fd0db362947db4a18a87df78f8d8e5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sun, 11 Feb 2024 00:49:32 +0100 Subject: [PATCH 34/40] Fix case insensitive matching in isc_ht hash table implementation The case insensitive matching in isc_ht was basically completely broken as only the hashvalue computation was case insensitive, but the key comparison was always case sensitive. (cherry picked from commit ec11aa2836a87724104b3e29abcc57cb6b2b7ee8) --- lib/isc/ht.c | 55 +++++++++++++++++++++++++++++++++++++++++---- tests/isc/ht_test.c | 50 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 4 deletions(-) diff --git a/lib/isc/ht.c b/lib/isc/ht.c index eaf2b3c508..e11050fc93 100644 --- a/lib/isc/ht.c +++ b/lib/isc/ht.c @@ -93,11 +93,54 @@ maybe_rehash(isc_ht_t *ht, size_t newcount); static isc_result_t isc__ht_iter_next(isc_ht_iter_t *it); +static uint8_t maptolower[] = { + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, + 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, + 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, + 0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, + 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x3a, 0x3b, + 0x3c, 0x3d, 0x3e, 0x3f, 0x40, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, + 0x68, 0x69, 0x6a, 0x6b, 0x6c, 0x6d, 0x6e, 0x6f, 0x70, 0x71, 0x72, 0x73, + 0x74, 0x75, 0x76, 0x77, 0x78, 0x79, 0x7a, 0x5b, 0x5c, 0x5d, 0x5e, 0x5f, + 0x60, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, 0x68, 0x69, 0x6a, 0x6b, + 0x6c, 0x6d, 0x6e, 0x6f, 0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77, + 0x78, 0x79, 0x7a, 0x7b, 0x7c, 0x7d, 0x7e, 0x7f, 0x80, 0x81, 0x82, 0x83, + 0x84, 0x85, 0x86, 0x87, 0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f, + 0x90, 0x91, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97, 0x98, 0x99, 0x9a, 0x9b, + 0x9c, 0x9d, 0x9e, 0x9f, 0xa0, 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, + 0xa8, 0xa9, 0xaa, 0xab, 0xac, 0xad, 0xae, 0xaf, 0xb0, 0xb1, 0xb2, 0xb3, + 0xb4, 0xb5, 0xb6, 0xb7, 0xb8, 0xb9, 0xba, 0xbb, 0xbc, 0xbd, 0xbe, 0xbf, + 0xc0, 0xc1, 0xc2, 0xc3, 0xc4, 0xc5, 0xc6, 0xc7, 0xc8, 0xc9, 0xca, 0xcb, + 0xcc, 0xcd, 0xce, 0xcf, 0xd0, 0xd1, 0xd2, 0xd3, 0xd4, 0xd5, 0xd6, 0xd7, + 0xd8, 0xd9, 0xda, 0xdb, 0xdc, 0xdd, 0xde, 0xdf, 0xe0, 0xe1, 0xe2, 0xe3, + 0xe4, 0xe5, 0xe6, 0xe7, 0xe8, 0xe9, 0xea, 0xeb, 0xec, 0xed, 0xee, 0xef, + 0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, 0xf8, 0xf9, 0xfa, 0xfb, + 0xfc, 0xfd, 0xfe, 0xff +}; + +static int +memcasecmp(const void *vs1, const void *vs2, size_t len) { + uint8_t const *s1 = vs1; + uint8_t const *s2 = vs2; + for (size_t i = 0; i < len; i++) { + uint8_t u1 = s1[i]; + uint8_t u2 = s2[i]; + int U1 = maptolower[u1]; + int U2 = maptolower[u2]; + int diff = U1 - U2; + if (diff) { + return diff; + } + } + return 0; +} + static bool isc__ht_node_match(isc_ht_node_t *node, const uint32_t hashval, - const uint8_t *key, uint32_t keysize) { + const uint8_t *key, uint32_t keysize, bool case_sensitive) { return (node->hashval == hashval && node->keysize == keysize && - memcmp(node->key, key, keysize) == 0); + (case_sensitive ? (memcmp(node->key, key, keysize) == 0) + : (memcasecmp(node->key, key, keysize) == 0))); } static uint32_t @@ -341,7 +384,9 @@ nexttable: for (isc_ht_node_t *node = ht->table[findex][hash]; node != NULL; node = node->next) { - if (isc__ht_node_match(node, hashval, key, keysize)) { + if (isc__ht_node_match(node, hashval, key, keysize, + ht->case_sensitive)) + { return (node); } } @@ -390,7 +435,9 @@ isc__ht_delete(isc_ht_t *ht, const unsigned char *key, const uint32_t keysize, for (isc_ht_node_t *node = ht->table[idx][hash]; node != NULL; prev = node, node = node->next) { - if (isc__ht_node_match(node, hashval, key, keysize)) { + if (isc__ht_node_match(node, hashval, key, keysize, + ht->case_sensitive)) + { if (prev == NULL) { ht->table[idx][hash] = node->next; } else { diff --git a/tests/isc/ht_test.c b/tests/isc/ht_test.c index 89e18f37b5..64efa9d833 100644 --- a/tests/isc/ht_test.c +++ b/tests/isc/ht_test.c @@ -312,7 +312,57 @@ ISC_RUN_TEST_IMPL(isc_ht_iterator) { test_ht_iterator(); } +ISC_RUN_TEST_IMPL(isc_ht_case) { + isc_ht_t *ht = NULL; + void *f = NULL; + isc_result_t result = ISC_R_UNSET; + + unsigned char lower[16] = { "test case" }; + unsigned char same[16] = { "test case" }; + unsigned char upper[16] = { "TEST CASE" }; + unsigned char mixed[16] = { "tEsT CaSe" }; + + isc_ht_init(&ht, mctx, 8, ISC_HT_CASE_SENSITIVE); + assert_non_null(ht); + + result = isc_ht_add(ht, lower, 16, (void *)lower); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_ht_add(ht, same, 16, (void *)same); + assert_int_equal(result, ISC_R_EXISTS); + + result = isc_ht_add(ht, upper, 16, (void *)upper); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_ht_find(ht, mixed, 16, &f); + assert_int_equal(result, ISC_R_NOTFOUND); + assert_null(f); + + isc_ht_destroy(&ht); + assert_null(ht); + + isc_ht_init(&ht, mctx, 8, ISC_HT_CASE_INSENSITIVE); + assert_non_null(ht); + + result = isc_ht_add(ht, lower, 16, (void *)lower); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_ht_add(ht, same, 16, (void *)same); + assert_int_equal(result, ISC_R_EXISTS); + + result = isc_ht_add(ht, upper, 16, (void *)upper); + assert_int_equal(result, ISC_R_EXISTS); + + result = isc_ht_find(ht, mixed, 16, &f); + assert_int_equal(result, ISC_R_SUCCESS); + assert_ptr_equal(f, &lower); + + isc_ht_destroy(&ht); + assert_null(ht); +} + ISC_TEST_LIST_START +ISC_TEST_ENTRY(isc_ht_case) ISC_TEST_ENTRY(isc_ht_20) ISC_TEST_ENTRY(isc_ht_8) ISC_TEST_ENTRY(isc_ht_1) From c8b623d87f0fb8f9cba8dea5c6a4b600953895e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sun, 11 Feb 2024 09:13:43 +0100 Subject: [PATCH 35/40] Add a system test for mixed-case data for the same owner We were missing a test where a single owner name would have multiple types with a different case. The generated RRSIGs and NSEC records will then have different case than the signed records and message parser have to cope with that and treat everything as the same owner. (cherry picked from commit 14e435b8140ce850aa03233b2144b8997d95eaf7) --- bin/tests/system/dnssec/ns3/secure.example.db.in | 5 +++++ bin/tests/system/dnssec/ns3/sign.sh | 4 +++- bin/tests/system/dnssec/tests.sh | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/bin/tests/system/dnssec/ns3/secure.example.db.in b/bin/tests/system/dnssec/ns3/secure.example.db.in index ec39308e9a..883e06790b 100644 --- a/bin/tests/system/dnssec/ns3/secure.example.db.in +++ b/bin/tests/system/dnssec/ns3/secure.example.db.in @@ -47,3 +47,8 @@ rrsigonly A 10.0.0.29 cnameandkey CNAME @ cnamenokey CNAME @ dnameandkey DNAME @ + +mixedcase A 10.0.0.30 +mixedCASE TXT "mixed case" +MIXEDcase AAAA 2002:: +mIxEdCaSe LOC 37 52 56.788 N 121 54 55.02 W 1120m 10m 100m 10m diff --git a/bin/tests/system/dnssec/ns3/sign.sh b/bin/tests/system/dnssec/ns3/sign.sh index 2f3b0de923..14fc709bfb 100644 --- a/bin/tests/system/dnssec/ns3/sign.sh +++ b/bin/tests/system/dnssec/ns3/sign.sh @@ -87,7 +87,9 @@ keyname=$("$KEYGEN" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" -n zone "$zone cat "$infile" "$cnameandkey.key" "$dnameandkey.key" "$keyname.key" >"$zonefile" -"$SIGNER" -z -o "$zone" "$zonefile" >/dev/null +"$SIGNER" -z -D -o "$zone" "$zonefile" >/dev/null +cat "$zonefile" "$zonefile".signed >"$zonefile".tmp +mv "$zonefile".tmp "$zonefile".signed zone=bogus.example. infile=bogus.example.db.in diff --git a/bin/tests/system/dnssec/tests.sh b/bin/tests/system/dnssec/tests.sh index bdaac667e0..02040b914d 100644 --- a/bin/tests/system/dnssec/tests.sh +++ b/bin/tests/system/dnssec/tests.sh @@ -766,6 +766,21 @@ n=$((n + 1)) test "$ret" -eq 0 || echo_i "failed" status=$((status + ret)) +echo_i "checking mixed-case positive validation ($n)" +ret=0 +for type in a txt aaaa loc; do + dig_with_opts +noauth mixedcase.secure.example. \ + @10.53.0.3 $type >dig.out.$type.ns3.test$n || ret=1 + dig_with_opts +noauth mixedcase.secure.example. \ + @10.53.0.4 $type >dig.out.$type.ns4.test$n || ret=1 + digcomp --lc dig.out.$type.ns3.test$n dig.out.$type.ns4.test$n || ret=1 + grep "status: NOERROR" dig.out.$type.ns4.test$n >/dev/null || ret=1 + grep "flags:.*ad.*QUERY" dig.out.$type.ns4.test$n >/dev/null || ret=1 +done +n=$((n + 1)) +test "$ret" -eq 0 || echo_i "failed" +status=$((status + ret)) + echo_i "checking multi-stage positive validation NSEC/NSEC3 ($n)" ret=0 dig_with_opts +noauth a.nsec3.example. \ From 5ce386aa5c0d0f1ff219929e6b52b8372b83aeb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Sun, 11 Feb 2024 11:23:28 +0100 Subject: [PATCH 36/40] Prepare release notes for BIND 9.18.24 --- doc/arm/notes.rst | 1 + doc/notes/notes-9.18.23.rst | 51 +++---------------------------- doc/notes/notes-9.18.24.rst | 61 +++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 46 deletions(-) create mode 100644 doc/notes/notes-9.18.24.rst diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index ce01edacb4..a4a9754866 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -35,6 +35,7 @@ information about each release, and source code. .. include:: ../notes/notes-known-issues.rst +.. include:: ../notes/notes-9.18.24.rst .. include:: ../notes/notes-9.18.23.rst .. include:: ../notes/notes-9.18.22.rst .. include:: ../notes/notes-9.18.21.rst diff --git a/doc/notes/notes-9.18.23.rst b/doc/notes/notes-9.18.23.rst index a74329ac54..7f95b80131 100644 --- a/doc/notes/notes-9.18.23.rst +++ b/doc/notes/notes-9.18.23.rst @@ -12,50 +12,9 @@ Notes for BIND 9.18.23 ---------------------- -Security Fixes -~~~~~~~~~~~~~~ +.. note:: -- 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` - -- Under certain circumstances, the DNS-over-TLS client code incorrectly - attempted to process more than one DNS message at a time, which could - cause :iscman:`named` to crash with an assertion failure. This has - been fixed. :gl:`#4487` - -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.18.23 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 Vinzenz Vogel and Daniel + Stirnimann of SWITCH. diff --git a/doc/notes/notes-9.18.24.rst b/doc/notes/notes-9.18.24.rst new file mode 100644 index 0000000000..f6df887450 --- /dev/null +++ b/doc/notes/notes-9.18.24.rst @@ -0,0 +1,61 @@ +.. 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.18.24 +---------------------- + +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` + +- Under certain circumstances, the DNS-over-TLS client code incorrectly + attempted to process more than one DNS message at a time, which could + cause :iscman:`named` to crash with an assertion failure. This has + been fixed. :gl:`#4487` + +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. From bb1bf51f10bef666875a937c81b3a79cdfe78186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sun, 11 Feb 2024 00:59:30 +0100 Subject: [PATCH 37/40] Add CHANGES note for [GL #4568] (cherry picked from commit b7797adc4e4c5105d3277cf7b669be0106897ca7) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index fc595882c0..2b031ee3e9 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6343. [bug] Fix case insensitive setting for isc_ht hashtable. + [GL #4568] + --- 9.18.23 released --- 6322. [security] Specific DNS answers could cause a denial-of-service From 886793e8decb0c02095c1c923407a2e4a8e0a01f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Sun, 11 Feb 2024 11:23:28 +0100 Subject: [PATCH 38/40] Add release note for GL #4459 --- doc/notes/notes-9.18.24.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/notes/notes-9.18.24.rst b/doc/notes/notes-9.18.24.rst index f6df887450..3e3f1c2f83 100644 --- a/doc/notes/notes-9.18.24.rst +++ b/doc/notes/notes-9.18.24.rst @@ -24,6 +24,10 @@ Security Fixes Applied Cybersecurity ATHENE for bringing this vulnerability to our attention. :gl:`#4424` +- Preparing an NSEC3 closest encloser proof could cause excessive CPU + load, leading to a denial-of-service condition. This has been fixed. + :cve:`2023-50868` :gl:`#4459` + - Parsing DNS messages with many different names could cause excessive CPU load. This has been fixed. :cve:`2023-4408` From b4021910740b778abc8e814faa48c929ced11cef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Sun, 11 Feb 2024 11:39:40 +0100 Subject: [PATCH 39/40] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 2b031ee3e9..9bd4f51e7e 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.18.24 released --- + 6343. [bug] Fix case insensitive setting for isc_ht hashtable. [GL #4568] From 6d7674f8f2a27de4f760f1eb82d98d4b0cf85bd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Sun, 11 Feb 2024 11:39:40 +0100 Subject: [PATCH 40/40] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 58af49b84e..8aa7a90cb1 100644 --- a/configure.ac +++ b/configure.ac @@ -16,7 +16,7 @@ # m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 18)dnl -m4_define([bind_VERSION_PATCH], 23)dnl +m4_define([bind_VERSION_PATCH], 24)dnl m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Extended Support Version)])dnl m4_define([bind_SRCID], [m4_esyscmd_s([git rev-parse --short HEAD | cut -b1-7])])dnl