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 6ad65904b732f77bd73623f77570a17e820957d5..86b8dd76702b5a74403dd9f92a9bbf899d675e82 100644 (file)
--- 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 a65661670f1a0f9ce31d8c3c58c4e6b3b6ed54aa..0eab106e4b56378a6bf2cd5753f6daab3bda4729 100644 (file)
--- 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 df1abd6519866a03699079070736768e5bef6e75..cf5e41192457839a4e3c1b7561171175807f2a8e 100644 (file)
--- 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 ca93875157805bdc356d1ec80663171657d799a9..58a9f4adff2eea2ace04ac0242882a3e4f133df7 100644 (file)
--- 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