chg: usr: named could crash on concurrent TKEY DELETE for the same key

On a server configured with tkey-gssapi-keytab (or tkey-gssapi-credential),
an authenticated peer could crash named by sending two TKEY DELETE requests
for the same dynamic key in rapid succession.  This has been fixed.

Closes #6001

Merge branch '6001-tsig-tkey-delete-uaf' into 'main'

See merge request isc-projects/bind9!12041

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

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 {

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
 
-void
-dns_tsigkey_delete(dns_tsigkey_t *key) {
-	REQUIRE(VALID_TSIGKEY(key));
+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);
+	}
+}
 
-	RWLOCK(&key->ring->lock, isc_rwlocktype_write);
-	rm_lru(key);
-	rm_hashmap(key);
-	RWUNLOCK(&key->ring->lock, isc_rwlocktype_write);
+void
+dns_tsigkey_delete(dns_tsigkeyring_t *ring, dns_tsigkey_t *tkey) {
+	REQUIRE(VALID_TSIGKEY(tkey));
+	REQUIRE(VALID_TSIGKEYRING(ring));
+
+	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);
 

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