From e9d5219fca9f6b819d953990b369d6acfb4e952b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 30 May 2023 08:46:17 +0200 Subject: [PATCH 01/11] Improve RBT overmem cache cleaning When cache memory usage is over the configured cache size (overmem) and we are cleaning unused entries, it might not be enough to clean just two entries if the entries to be expired are smaller than the newly added rdata. This could be abused by an attacker to cause a remote Denial of Service by possibly running out of the operating system memory. Currently, the addrdataset() tries to do a single TTL-based cleaning considering the serve-stale TTL and then optionally moves to overmem cleaning if we are in that condition. Then the overmem_purge() tries to do another single TTL based cleaning from the TTL heap and then continue with LRU-based cleaning up to 2 entries cleaned. Squash the TTL-cleaning mechanism into single call from addrdataset(), but ignore the serve-stale TTL if we are currently overmem. Then instead of having a fixed number of entries to clean, pass the size of newly added rdatasetheader to the overmem_purge() function and cleanup at least the size of the newly added data. This prevents the cache going over the configured memory limit (`max-cache-size`). Additionally, refactor the overmem_purge() function to reduce for-loop nesting for readability. --- lib/dns/rbtdb.c | 106 +++++++++++++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 41 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index bebef1006a..5d364667eb 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -561,7 +561,7 @@ static void expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, bool tree_locked, expire_t reason); static void -overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, +overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, size_t purgesize, bool tree_locked); static void resign_insert(dns_rbtdb_t *rbtdb, int idx, rdatasetheader_t *newheader); @@ -6795,6 +6795,16 @@ cleanup: static dns_dbmethods_t zone_methods; +static size_t +rdataset_size(rdatasetheader_t *header) { + if (!NONEXISTENT(header)) { + return (dns_rdataslab_size((unsigned char *)header, + sizeof(*header))); + } + + return (sizeof(*header)); +} + static isc_result_t addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, isc_stdtime_t now, dns_rdataset_t *rdataset, unsigned int options, @@ -6959,7 +6969,8 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } if (cache_is_overmem) { - overmem_purge(rbtdb, rbtnode->locknum, now, tree_locked); + overmem_purge(rbtdb, rbtnode->locknum, rdataset_size(newheader), + tree_locked); } NODE_LOCK(&rbtdb->node_locks[rbtnode->locknum].lock, @@ -6978,11 +6989,18 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } header = isc_heap_element(rbtdb->heaps[rbtnode->locknum], 1); - if (header != NULL && - header->rdh_ttl + STALE_TTL(header, rbtdb) < - now - RBTDB_VIRTUAL) - { - expire_header(rbtdb, header, tree_locked, expire_ttl); + if (header != NULL) { + dns_ttl_t rdh_ttl = header->rdh_ttl; + + /* Only account for stale TTL if cache is not overmem */ + if (!cache_is_overmem) { + rdh_ttl += STALE_TTL(header, rbtdb); + } + + if (rdh_ttl < now - RBTDB_VIRTUAL) { + expire_header(rbtdb, header, tree_locked, + expire_ttl); + } } /* @@ -10122,52 +10140,58 @@ update_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, isc_stdtime_t now) { ISC_LIST_PREPEND(rbtdb->rdatasets[header->node->locknum], header, link); } +static size_t +expire_lru_headers(dns_rbtdb_t *rbtdb, unsigned int locknum, size_t purgesize, + bool tree_locked) { + rdatasetheader_t *header, *header_prev; + size_t purged = 0; + + for (header = ISC_LIST_TAIL(rbtdb->rdatasets[locknum]); + header != NULL && purged <= purgesize; header = header_prev) + { + header_prev = ISC_LIST_PREV(header, link); + /* + * Unlink the entry at this point to avoid checking it + * again even if it's currently used someone else and + * cannot be purged at this moment. This entry won't be + * referenced any more (so unlinking is safe) since the + * TTL was reset to 0. + */ + ISC_LIST_UNLINK(rbtdb->rdatasets[locknum], header, link); + size_t header_size = rdataset_size(header); + expire_header(rbtdb, header, tree_locked, expire_lru); + purged += header_size; + } + + return (purged); +} + /*% - * Purge some expired and/or stale (i.e. unused for some period) cache entries - * under an overmem condition. To recover from this condition quickly, up to - * 2 entries will be purged. This process is triggered while adding a new - * entry, and we specifically avoid purging entries in the same LRU bucket as - * the one to which the new entry will belong. Otherwise, we might purge - * entries of the same name of different RR types while adding RRsets from a - * single response (consider the case where we're adding A and AAAA glue records - * of the same NS name). + * Purge some stale (i.e. unused for some period - LRU based cleaning) cache + * entries under the overmem condition. To recover from this condition quickly, + * we cleanup entries up to the size of newly added rdata (passed as purgesize). + * + * This process is triggered while adding a new entry, and we specifically avoid + * purging entries in the same LRU bucket as the one to which the new entry will + * belong. Otherwise, we might purge entries of the same name of different RR + * types while adding RRsets from a single response (consider the case where + * we're adding A and AAAA glue records of the same NS name). */ static void -overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, +overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, size_t purgesize, bool tree_locked) { - rdatasetheader_t *header, *header_prev; unsigned int locknum; - int purgecount = 2; + size_t purged = 0; for (locknum = (locknum_start + 1) % rbtdb->node_lock_count; - locknum != locknum_start && purgecount > 0; + locknum != locknum_start && purged <= purgesize; locknum = (locknum + 1) % rbtdb->node_lock_count) { NODE_LOCK(&rbtdb->node_locks[locknum].lock, isc_rwlocktype_write); - header = isc_heap_element(rbtdb->heaps[locknum], 1); - if (header && header->rdh_ttl < now - RBTDB_VIRTUAL) { - expire_header(rbtdb, header, tree_locked, expire_ttl); - purgecount--; - } - - for (header = ISC_LIST_TAIL(rbtdb->rdatasets[locknum]); - header != NULL && purgecount > 0; header = header_prev) - { - header_prev = ISC_LIST_PREV(header, link); - /* - * Unlink the entry at this point to avoid checking it - * again even if it's currently used someone else and - * cannot be purged at this moment. This entry won't be - * referenced any more (so unlinking is safe) since the - * TTL was reset to 0. - */ - ISC_LIST_UNLINK(rbtdb->rdatasets[locknum], header, - link); - expire_header(rbtdb, header, tree_locked, expire_lru); - purgecount--; - } + purged += expire_lru_headers(rbtdb, locknum, purgesize - purged, + tree_locked); NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, isc_rwlocktype_write); From 09fcd8f88a988a94bc8ea0759b27f1a1b652d481 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 1 Jun 2023 15:46:23 +0200 Subject: [PATCH 02/11] Add CHANGES and release note for [GL #4055] --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 9 ++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 3955f7f618..b34478bf5c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6190. [security] Improve the overmem cleaning process to prevent the + cache going over the configured limit. (CVE-2023-2828) + [GL #4055] + 6188. [performance] Reduce memory consumption by allocating properly sized send buffers for stream-based transports. [GL #4038] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 5ae9d306f8..78c3c048e2 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -15,7 +15,14 @@ Notes for BIND 9.18.16 Security Fixes ~~~~~~~~~~~~~~ -- None. +- The overmem cleaning process has been improved, to prevent the cache from + significantly exceeding the configured :any:`max-cache-size` limit. + (CVE-2023-2828) + + 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:`#4055` New Features ~~~~~~~~~~~~ From ec72e11ee46c4d2059ab641d69acd25999ee7b72 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Wed, 7 Jun 2023 14:03:01 +0200 Subject: [PATCH 03/11] Set max-cache-size expectations for low values --- doc/arm/reference.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 25a13fa70a..3de2effb72 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -3932,6 +3932,11 @@ system. default value of that option (90% of physical memory for each individual cache) may lead to memory exhaustion over time. + .. note:: + + :any:`max-cache-size` does not work reliably for the maximum + amount of memory of 100 MB or lower. + Upon startup and reconfiguration, caches with a limited size preallocate a small amount of memory (less than 1% of :any:`max-cache-size` for a given view). This preallocation serves as an From 240caa32b9cab90a38ab863fd64e6becf5d1393c Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 25 May 2023 23:53:50 -0700 Subject: [PATCH 04/11] Stale answer lookups could loop when over recursion quota When a query was aborted because of the recursion quota being exceeded, but triggered a stale answer response and a stale data refresh query, it could cause named to loop back where we are iterating and following a delegation. Having no good answer in cache, we would fall back to using serve-stale again, use the stale data, try to refresh the RRset, and loop back again, without ever terminating until crashing due to stack overflow. This happens because in the functions 'query_notfound()' and 'query_delegation_recurse()', we check whether we can fall back to serving stale data. We shouldn't do so if we are already refreshing an RRset due to having prioritized stale data in cache. In other words, we need to add an extra check to 'query_usestale()' to disallow serving stale data if we are currently refreshing a stale RRset. As an additional mitigation to prevent looping, we now use the result code ISC_R_ALREADYRUNNING rather than ISC_R_FAILURE when a recursion loop is encountered, and we check for that condition in 'query_usestale()' as well. --- lib/ns/query.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 1fab263a35..9fa8ee0f9a 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6533,7 +6533,7 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, if (recparam_match(&client->query.recparam, qtype, qname, qdomain)) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_QUERY, ISC_LOG_INFO, "recursion loop detected"); - return (ISC_R_FAILURE); + return (ISC_R_ALREADYRUNNING); } recparam_update(&client->query.recparam, qtype, qname, qdomain); @@ -7650,10 +7650,21 @@ query_usestale(query_ctx_t *qctx, isc_result_t result) { return (false); } - if (result == DNS_R_DUPLICATE || result == DNS_R_DROP) { + if (qctx->refresh_rrset) { + /* + * This is a refreshing query, we have already prioritized + * stale data, so don't enable serve-stale again. + */ + return (false); + } + + if (result == DNS_R_DUPLICATE || result == DNS_R_DROP || + result == ISC_R_ALREADYRUNNING) + { /* * Don't enable serve-stale if the result signals a duplicate - * query or query that is being dropped. + * query or a query that is being dropped or can't proceed + * because of a recursion loop. */ return (false); } From ff5bacf17c2451e9d48c78a5ef96ec0c376ff33d Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 1 Jun 2023 10:03:48 +0200 Subject: [PATCH 05/11] Fix serve-stale hang at shutdown The 'refresh_rrset' variable is used to determine if we can detach from the client. This can cause a hang on shutdown. To fix this, move setting of the 'nodetach' variable up to where 'refresh_rrset' is set (in query_lookup(), and thus not in ns_query_done()), and set it to false when actually refreshing the RRset, so that when this lookup is completed, the client will be detached. --- lib/ns/query.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 9fa8ee0f9a..64e9278307 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -5827,6 +5827,7 @@ query_refresh_rrset(query_ctx_t *orig_qctx) { qctx.client->query.dboptions &= ~(DNS_DBFIND_STALETIMEOUT | DNS_DBFIND_STALEOK | DNS_DBFIND_STALEENABLED); + qctx.client->nodetach = false; /* * We'll need some resources... @@ -6100,7 +6101,14 @@ query_lookup(query_ctx_t *qctx) { "%s stale answer used, an attempt to " "refresh the RRset will still be made", namebuf); + qctx->refresh_rrset = STALE(qctx->rdataset); + /* + * If we are refreshing the RRSet, we must not + * detach from the client in query_send(). + */ + qctx->client->nodetach = qctx->refresh_rrset; + if (stale_found) { ns_client_extendederror( qctx->client, ede, @@ -11961,12 +11969,7 @@ ns_query_done(query_ctx_t *qctx) { /* * Client may have been detached after query_send(), so * we test and store the flag state here, for safety. - * If we are refreshing the RRSet, we must not detach from the client - * in the query_send(), so we need to override the flag. */ - if (qctx->refresh_rrset) { - qctx->client->nodetach = true; - } nodetach = qctx->client->nodetach; query_send(qctx->client); From 10ac503a94ee5384f68325d50dd90eaf03d41912 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 31 May 2023 12:47:31 -0700 Subject: [PATCH 06/11] CHANGES and release notes for [GL #4089] --- CHANGES | 7 +++++++ doc/notes/notes-current.rst | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/CHANGES b/CHANGES index b34478bf5c..9bcfe6270f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,10 @@ +6192. [security] A query that prioritizes stale data over lookup + triggers a fetch to refresh the stale data in cache. + If the fetch is aborted for exceeding the recursion + quota, it was possible for 'named' to enter an infinite + callback loop and crash due to stack overflow. This has + been fixed. (CVE-2023-2911) [GL #4089] + 6190. [security] Improve the overmem cleaning process to prevent the cache going over the configured limit. (CVE-2023-2828) [GL #4055] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 78c3c048e2..3c706ebe98 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -24,6 +24,12 @@ Security Fixes and Yuval Shavitt from Tel-Aviv University for bringing this vulnerability to our attention. :gl:`#4055` +- A query that prioritizes stale data over lookup triggers a fetch to refresh + the stale data in cache. If the fetch is aborted for exceeding the recursion + quota, it was possible for :iscman:`named` to enter an infinite callback + loop and crash due to stack overflow. This has been fixed. (CVE-2023-2911) + :gl:`#4089` + New Features ~~~~~~~~~~~~ From d3fa7e19302c5b3573b59723cc94daebdc188eca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 2 Jun 2023 12:17:16 +0200 Subject: [PATCH 07/11] Re-add a code comment to the "hooks" system test Commit da74157440e02bd52e0abe6036e4e9ec7316ebd2 removed a useful code comment from the "hooks" system test. Add it back to prevent confusion. --- bin/tests/system/hooks/tests_async_plugin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/tests/system/hooks/tests_async_plugin.py b/bin/tests/system/hooks/tests_async_plugin.py index 48f9feefbd..2f42e27379 100644 --- a/bin/tests/system/hooks/tests_async_plugin.py +++ b/bin/tests/system/hooks/tests_async_plugin.py @@ -23,4 +23,5 @@ def test_async_hook(named_port): "A", ) ans = dns.query.udp(msg, "10.53.0.1", timeout=10, port=named_port) + # the test-async plugin changes the status of any positive answer to NOTIMP assert ans.rcode() == dns.rcode.NOTIMP From ddfa5450225b621aa258c870a33c5f6dd6b98d0c Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 9 Jun 2023 12:49:08 +0200 Subject: [PATCH 08/11] Prepare release notes for BIND 9.18.16 --- doc/arm/notes.rst | 2 +- doc/notes/{notes-current.rst => notes-9.18.16.rst} | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) rename doc/notes/{notes-current.rst => notes-9.18.16.rst} (98%) diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index 3dc0196180..4af203f336 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.16.rst .. include:: ../notes/notes-9.18.15.rst .. include:: ../notes/notes-9.18.14.rst .. include:: ../notes/notes-9.18.13.rst diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-9.18.16.rst similarity index 98% rename from doc/notes/notes-current.rst rename to doc/notes/notes-9.18.16.rst index 3c706ebe98..f13ba434b6 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-9.18.16.rst @@ -33,8 +33,6 @@ Security Fixes New Features ~~~~~~~~~~~~ -- None. - - The system test suite can now be executed with pytest (along with pytest-xdist for parallel execution). :gl:`#3978` @@ -45,11 +43,6 @@ Removed Features will be removed in a future release. A warning will be logged when the ``tkey-dhkey`` option is used in ``named.conf``. :gl:`#3905` -Feature Changes -~~~~~~~~~~~~~~~ - -- None. - Bug Fixes ~~~~~~~~~ From 7e2c9d2747a2558493e46099b73022f54c25ea36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 2 Jun 2023 12:28:23 +0200 Subject: [PATCH 09/11] Tweak and reword release notes --- doc/notes/notes-9.18.16.rst | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/notes/notes-9.18.16.rst b/doc/notes/notes-9.18.16.rst index f13ba434b6..9ed090ca9c 100644 --- a/doc/notes/notes-9.18.16.rst +++ b/doc/notes/notes-9.18.16.rst @@ -41,28 +41,28 @@ Removed Features - TKEY mode 2 (Diffie-Hellman Exchanged Keying) is now deprecated, and will be removed in a future release. A warning will be logged when - the ``tkey-dhkey`` option is used in ``named.conf``. :gl:`#3905` + the :any:`tkey-dhkey` option is used in ``named.conf``. :gl:`#3905` Bug Fixes ~~~~~~~~~ -- BIND could get stuck on reconfiguration when a `listen` statement - for HTTP is removed from the configuration. That has been fixed. - :gl:`#4071` +- BIND could get stuck on reconfiguration when a :any:`listen-on` + statement for HTTP is removed from the configuration. That has been + fixed. :gl:`#4071` -- It could happen that after the :any:`stale-answer-client-timeout` duration, - a delegation from cache was returned to the client. This has now been fixed. - :gl:`#3950` +- Previously, it was possible for a delegation from cache to be returned + to the client after the :any:`stale-answer-client-timeout` duration. + This has been fixed. :gl:`#3950` - BIND could allocate too big buffers when sending data via stream-based DNS transports, leading to increased memory usage. This has been fixed. :gl:`#4038` - When the :any:`stale-answer-enable` option was enabled and the - :any:`stale-answer-client-timeout` option was enabled and larger than 0, - ``named`` was taking two places from the :any:`clients-per-query` limit for - each client and was failing to gradually auto-tune its value, as configured. - This has been fixed. :gl:`#4074` + :any:`stale-answer-client-timeout` option was enabled and larger than + 0, :iscman:`named` previously allocated two slots from the + :any:`clients-per-query` limit for each client and failed to gradually + auto-tune its value, as configured. This has been fixed. :gl:`#4074` Known Issues ~~~~~~~~~~~~ From 7488850de1b81f061da29daaba7ee8c34d3990a0 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 9 Jun 2023 15:38:05 +0200 Subject: [PATCH 10/11] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 9bcfe6270f..d0b15b8c40 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.18.16 released --- + 6192. [security] A query that prioritizes stale data over lookup triggers a fetch to refresh the stale data in cache. If the fetch is aborted for exceeding the recursion From 8193c9b8628580df04320a98c69adcb9e761d214 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 9 Jun 2023 15:38:48 +0200 Subject: [PATCH 11/11] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 4fa9517e4f..c8a3ba95ff 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], 16)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