From: Ondřej Surý Date: Sun, 17 May 2026 15:01:54 +0000 (+0200) Subject: Fix use-after-free in concurrent dns_tsigkey_delete() X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=5c8dcd4419693320817e44ad144002cd7c648acc;p=thirdparty%2Fbind9.git Fix use-after-free in concurrent dns_tsigkey_delete() Two TSIG-authenticated TKEY DELETE queries for the same dynamic key, arriving on different worker loops, could each enter dns_tsigkey_delete() and cause over-decrementing the key refcount. This has been fixed by making dns_tsigkey_delete() idempotent. --- diff --git a/lib/dns/include/dns/tsig.h b/lib/dns/include/dns/tsig.h index 6ad65904b73..86b8dd76702 100644 --- a/lib/dns/include/dns/tsig.h +++ b/lib/dns/include/dns/tsig.h @@ -82,17 +82,16 @@ struct dns_tsigkey { isc_mem_t *mctx; dst_key_t *key; /*%< Key */ dns_fixedname_t fn; - dns_name_t *name; /*%< Key name */ - dst_algorithm_t alg; /*< Algorithm */ - dns_name_t algname; /*< Algorithm name, only used if - algorithm is DST_ALG_UNKNOWN */ - dns_name_t *creator; /*%< name that created secret */ - bool generated : 1; /*%< key was auto-generated */ - bool restored : 1; /*%< key was restored at startup */ - isc_stdtime_t inception; /*%< start of validity period */ - isc_stdtime_t expire; /*%< end of validity period */ - dns_tsigkeyring_t *ring; /*%< the enclosing keyring */ - isc_refcount_t references; /*%< reference counter */ + dns_name_t *name; /*%< Key name */ + dst_algorithm_t alg; /*< Algorithm */ + dns_name_t algname; /*< Algorithm name, only used if + algorithm is DST_ALG_UNKNOWN */ + dns_name_t *creator; /*%< name that created secret */ + bool generated : 1; /*%< key was auto-generated */ + bool restored : 1; /*%< key was restored at startup */ + isc_stdtime_t inception; /*%< start of validity period */ + isc_stdtime_t expire; /*%< end of validity period */ + isc_refcount_t references; /*%< reference counter */ ISC_LINK(dns_tsigkey_t) link; }; @@ -163,13 +162,14 @@ dns_tsigkey_createfromkey(const dns_name_t *name, dst_algorithm_t algorithm, */ void -dns_tsigkey_delete(dns_tsigkey_t *key); +dns_tsigkey_delete(dns_tsigkeyring_t *ring, dns_tsigkey_t *key); /*%< * Prevents this key from being used again. It will be deleted when * no references exist. * * Requires: - *\li 'key' is a valid TSIG key on a keyring + *\li 'ring' is a valid TSIG keyring + *\li 'key' is a valid TSIG key on a keyring 'ring' */ isc_result_t diff --git a/lib/dns/tkey.c b/lib/dns/tkey.c index a65661670f1..0eab106e4b5 100644 --- a/lib/dns/tkey.c +++ b/lib/dns/tkey.c @@ -316,7 +316,7 @@ process_deletetkey(dns_name_t *signer, dns_name_t *name, * was not generated with TKEY and is in the config file, it may be * reloaded later. */ - dns_tsigkey_delete(tsigkey); + dns_tsigkey_delete(ring, tsigkey); /* Release the reference */ dns_tsigkey_detach(&tsigkey); @@ -444,7 +444,7 @@ dns_tkey_processquery(dns_message_t *msg, dns_tkeyctx_t *tctx, if (tsigkey->inception != tsigkey->expire && tsigkey->expire <= isc_stdtime_now()) { - dns_tsigkey_delete(tsigkey); + dns_tsigkey_delete(ring, tsigkey); dns_tsigkey_detach(&tsigkey); result = ISC_R_NOTFOUND; } else { diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c index df1abd65198..cf5e4119245 100644 --- a/lib/dns/tsig.c +++ b/lib/dns/tsig.c @@ -151,41 +151,18 @@ match_ptr(void *node, const void *key) { } static void -rm_hashmap(dns_tsigkey_t *tkey) { - REQUIRE(VALID_TSIGKEY(tkey)); - REQUIRE(VALID_TSIGKEYRING(tkey->ring)); - - (void)isc_hashmap_delete(tkey->ring->keys, dns_name_hash(tkey->name), - match_ptr, tkey); - dns_tsigkey_detach(&tkey); -} - -static void -rm_lru(dns_tsigkey_t *tkey) { - REQUIRE(VALID_TSIGKEY(tkey)); - REQUIRE(VALID_TSIGKEYRING(tkey->ring)); - - if (tkey->generated && ISC_LINK_LINKED(tkey, link)) { - ISC_LIST_UNLINK(tkey->ring->lru, tkey, link); - tkey->ring->generated--; - dns_tsigkey_unref(tkey); - } -} - -static void -adjust_lru(dns_tsigkey_t *tkey) { +adjust_lru(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) { if (tkey->generated) { - RWLOCK(&tkey->ring->lock, isc_rwlocktype_write); + RWLOCK(&ring->lock, isc_rwlocktype_write); /* * We may have been removed from the LRU list between * removing the read lock and acquiring the write lock. */ - if (ISC_LINK_LINKED(tkey, link) && tkey->ring->lru.tail != tkey) - { - ISC_LIST_UNLINK(tkey->ring->lru, tkey, link); - ISC_LIST_APPEND(tkey->ring->lru, tkey, link); + if (ISC_LINK_LINKED(tkey, link) && ring->lru.tail != tkey) { + ISC_LIST_UNLINK(ring->lru, tkey, link); + ISC_LIST_APPEND(ring->lru, tkey, link); } - RWUNLOCK(&tkey->ring->lock, isc_rwlocktype_write); + RWUNLOCK(&ring->lock, isc_rwlocktype_write); } } @@ -273,6 +250,14 @@ cleanup_name: return result; } +static void +dns__tsigkey_deletelru(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) { + if (tkey->generated && ISC_LINK_LINKED(tkey, link)) { + ISC_LIST_UNLINK(ring->lru, tkey, link); + ring->generated--; + } +} + static void destroyring(dns_tsigkeyring_t *ring) { isc_result_t result; @@ -285,7 +270,8 @@ destroyring(dns_tsigkeyring_t *ring) { { dns_tsigkey_t *tkey = NULL; isc_hashmap_iter_current(it, (void **)&tkey); - rm_lru(tkey); + + dns__tsigkey_deletelru(ring, tkey); dns_tsigkey_detach(&tkey); } isc_hashmap_iter_destroy(&it); @@ -507,14 +493,24 @@ ISC_REFCOUNT_TRACE_IMPL(dns_tsigkey, destroy_tsigkey); ISC_REFCOUNT_IMPL(dns_tsigkey, destroy_tsigkey); #endif +static void +dns__tsigkey_delete(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) { + isc_result_t result = isc_hashmap_delete( + ring->keys, dns_name_hash(tkey->name), match_ptr, tkey); + if (result == ISC_R_SUCCESS) { + dns__tsigkey_deletelru(ring, tkey); + dns_tsigkey_detach(&tkey); + } +} + void -dns_tsigkey_delete(dns_tsigkey_t *key) { - REQUIRE(VALID_TSIGKEY(key)); +dns_tsigkey_delete(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) { + REQUIRE(VALID_TSIGKEY(tkey)); + REQUIRE(VALID_TSIGKEYRING(ring)); - RWLOCK(&key->ring->lock, isc_rwlocktype_write); - rm_lru(key); - rm_hashmap(key); - RWUNLOCK(&key->ring->lock, isc_rwlocktype_write); + RWLOCK(&ring->lock, isc_rwlocktype_write); + dns__tsigkey_delete(ring, tkey); + RWUNLOCK(&ring->lock, isc_rwlocktype_write); } isc_result_t @@ -1489,14 +1485,13 @@ again: key = NULL; goto again; } - rm_lru(key); - rm_hashmap(key); + dns__tsigkey_delete(ring, key); RWUNLOCK(&ring->lock, locktype); return ISC_R_NOTFOUND; } dns_tsigkey_ref(key); RWUNLOCK(&ring->lock, locktype); - adjust_lru(key); + adjust_lru(ring, key); *tsigkey = key; return ISC_R_SUCCESS; } @@ -1561,14 +1556,12 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) { REQUIRE(VALID_TSIGKEY(tkey)); REQUIRE(VALID_TSIGKEYRING(ring)); - REQUIRE(tkey->ring == NULL); RWLOCK(&ring->lock, isc_rwlocktype_write); result = isc_hashmap_add(ring->keys, dns_name_hash(tkey->name), tkey_match, tkey->name, tkey, NULL); if (result == ISC_R_SUCCESS) { dns_tsigkey_ref(tkey); - tkey->ring = ring; /* * If this is a TKEY-generated key, add it to the LRU list, @@ -1578,15 +1571,11 @@ dns_tsigkeyring_add(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) { */ if (tkey->generated) { ISC_LIST_APPEND(ring->lru, tkey, link); - dns_tsigkey_ref(tkey); if (++ring->generated > DNS_TSIG_MAXGENERATEDKEYS) { dns_tsigkey_t *key = ISC_LIST_HEAD(ring->lru); - rm_lru(key); - rm_hashmap(key); + dns__tsigkey_delete(ring, key); } } - - tkey->ring = ring; } RWUNLOCK(&ring->lock, isc_rwlocktype_write); diff --git a/tests/dns/tsig_test.c b/tests/dns/tsig_test.c index ca938751578..58a9f4adff2 100644 --- a/tests/dns/tsig_test.c +++ b/tests/dns/tsig_test.c @@ -483,6 +483,45 @@ ISC_RUN_TEST_IMPL(tsig_badsig) { tsig_tcp(isc_stdtime_now(), DNS_R_TSIGVERIFYFAILURE, true); } +/* + * dns_tsigkey_delete() must be idempotent: a second call on a key + * that has already been removed from the keyring is a no-op and must + * not touch the key's refcount. + */ +static void +tsig_delete(bool generated) { + dns_fixedname_t fkeyname; + dns_name_t *keyname = dns_fixedname_initname(&fkeyname); + + isc_result_t result = dns_name_fromstring(keyname, "tsig-key", + dns_rootname, 0, NULL); + assert_int_equal(result, ISC_R_SUCCESS); + + dns_tsigkeyring_t *ring = NULL; + dns_tsigkeyring_create(isc_g_mctx, &ring); + assert_non_null(ring); + + dns_tsigkey_t *key = NULL; + result = dns_tsigkey_createfromkey(keyname, DST_ALG_HMACSHA256, NULL, + generated, false, NULL, 0, 0, + isc_g_mctx, &key); + assert_int_equal(result, ISC_R_SUCCESS); + + result = dns_tsigkeyring_add(ring, key); + assert_int_equal(result, ISC_R_SUCCESS); + + dns_tsigkey_delete(ring, key); + dns_tsigkey_delete(ring, key); + + dns_tsigkey_detach(&key); + dns_tsigkeyring_detach(&ring); +} + +ISC_RUN_TEST_IMPL(tsig_delete) { + tsig_delete(false); + tsig_delete(true); +} + /* Tests the dns__tsig_algvalid function */ ISC_RUN_TEST_IMPL(algvalid) { UNUSED(state); @@ -502,6 +541,7 @@ ISC_TEST_LIST_START ISC_TEST_ENTRY(algvalid) ISC_TEST_ENTRY(tsig_badsig) ISC_TEST_ENTRY(tsig_badtime) +ISC_TEST_ENTRY(tsig_delete) ISC_TEST_ENTRY(tsig_tcp) ISC_TEST_LIST_END