From 0854f631149848b64cc193979d0b0edf39159330 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 2 Jun 2020 12:38:40 +1000 Subject: [PATCH 1/3] Remove INSIST from from new_reference RBTDB node can now appear on the deadnodes lists following the changes to decrement_reference in 176b23b6cd98e5b58f832902fdbe964ee5f762d0 to defer checking of node->down when the tree write lock is not held. The node should be unlinked instead. --- lib/dns/rbtdb.c | 173 ++++++++++++++++++++++++++++-------------------- 1 file changed, 100 insertions(+), 73 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index bfe3538a59..87fbdb317b 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1858,8 +1858,13 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { * Caller must be holding the node lock. */ static inline void -new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { - INSIST(!ISC_LINK_LINKED(node, deadlink)); +new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + isc_rwlocktype_t locktype) { + if (locktype == isc_rwlocktype_write && ISC_LINK_LINKED(node, deadlink)) + { + ISC_LIST_UNLINK(rbtdb->deadnodes[node->locknum], node, + deadlink); + } if (isc_refcount_increment0(&node->references) == 0) { /* this is the first reference to the node */ isc_refcount_increment0( @@ -1877,13 +1882,14 @@ is_leaf(dns_rbtnode_t *node) { } static inline void -send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { +send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + isc_rwlocktype_t locktype) { isc_event_t *ev; dns_db_t *db; ev = isc_event_allocate(rbtdb->common.mctx, NULL, DNS_EVENT_RBTPRUNE, prune_tree, node, sizeof(isc_event_t)); - new_reference(rbtdb, node); + new_reference(rbtdb, node, locktype); db = NULL; attach((dns_db_t *)rbtdb, &db); ev->ev_sender = db; @@ -1919,7 +1925,7 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) { node->data == NULL); if (is_leaf(node) && rbtdb->task != NULL) { - send_to_prune_tree(rbtdb, node); + send_to_prune_tree(rbtdb, node, isc_rwlocktype_write); } else if (node->down == NULL && node->data == NULL) { /* * Not a interior node and not needing to be @@ -1987,7 +1993,7 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, } } - new_reference(rbtdb, node); + new_reference(rbtdb, node, locktype); NODE_UNLOCK(nodelock, locktype); } @@ -2122,15 +2128,17 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * periodic walk-through). */ if (!pruning && is_leaf(node) && rbtdb->task != NULL) { - send_to_prune_tree(rbtdb, node); + send_to_prune_tree(rbtdb, node, isc_rwlocktype_write); no_reference = false; } else { delete_node(rbtdb, node); } } else { INSIST(node->data == NULL); - INSIST(!ISC_LINK_LINKED(node, deadlink)); - ISC_LIST_APPEND(rbtdb->deadnodes[bucket], node, deadlink); + if (!ISC_LINK_LINKED(node, deadlink)) { + ISC_LIST_APPEND(rbtdb->deadnodes[bucket], node, + deadlink); + } } restore_locks: @@ -2200,16 +2208,13 @@ prune_tree(isc_task_t *task, isc_event_t *event) { /* * We need to gain a reference to the node before - * decrementing it in the next iteration. In addition, - * if the node is in the dead-nodes list, extract it - * from the list beforehand as we do in - * reactivate_node(). + * decrementing it in the next iteration. */ if (ISC_LINK_LINKED(parent, deadlink)) { ISC_LIST_UNLINK(rbtdb->deadnodes[locknum], parent, deadlink); } - new_reference(rbtdb, parent); + new_reference(rbtdb, parent, isc_rwlocktype_write); } else { parent = NULL; } @@ -2976,7 +2981,7 @@ zone_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { * We increment the reference count on node to ensure that * search->zonecut_rdataset will still be valid later. */ - new_reference(search->rbtdb, node); + new_reference(search->rbtdb, node, isc_rwlocktype_read); search->zonecut = node; search->zonecut_rdataset = found; search->need_cleanup = true; @@ -3028,7 +3033,8 @@ zone_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { static inline void bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header, - isc_stdtime_t now, dns_rdataset_t *rdataset) { + isc_stdtime_t now, isc_rwlocktype_t locktype, + dns_rdataset_t *rdataset) { unsigned char *raw; /* RDATASLAB */ /* @@ -3043,7 +3049,7 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header, return; } - new_reference(rbtdb, node); + new_reference(rbtdb, node, locktype); INSIST(rdataset->methods == NULL); /* We must be disassociated. */ @@ -3148,12 +3154,12 @@ setup_delegation(rbtdb_search_t *search, dns_dbnode_t **nodep, NODE_LOCK(&(search->rbtdb->node_locks[node->locknum].lock), isc_rwlocktype_read); bind_rdataset(search->rbtdb, node, search->zonecut_rdataset, - search->now, rdataset); + search->now, isc_rwlocktype_read, rdataset); if (sigrdataset != NULL && search->zonecut_sigrdataset != NULL) { bind_rdataset(search->rbtdb, node, search->zonecut_sigrdataset, search->now, - sigrdataset); + isc_rwlocktype_read, sigrdataset); } NODE_UNLOCK(&(search->rbtdb->node_locks[node->locknum].lock), isc_rwlocktype_read); @@ -3818,18 +3824,21 @@ again: foundname, NULL); if (result == ISC_R_SUCCESS) { if (nodep != NULL) { - new_reference(search->rbtdb, - node); + new_reference( + search->rbtdb, node, + isc_rwlocktype_read); *nodep = node; } bind_rdataset(search->rbtdb, node, found, search->now, + isc_rwlocktype_read, rdataset); if (foundsig != NULL) { - bind_rdataset(search->rbtdb, - node, foundsig, - search->now, - sigrdataset); + bind_rdataset( + search->rbtdb, node, + foundsig, search->now, + isc_rwlocktype_read, + sigrdataset); } } } else if (found == NULL && foundsig == NULL) { @@ -4114,7 +4123,8 @@ found: * ensure that search->zonecut_rdataset will * still be valid later. */ - new_reference(search.rbtdb, node); + new_reference(search.rbtdb, node, + isc_rwlocktype_read); search.zonecut = node; search.zonecut_rdataset = header; search.zonecut_sigrdataset = NULL; @@ -4292,7 +4302,7 @@ found: goto node_exit; } if (nodep != NULL) { - new_reference(search.rbtdb, node); + new_reference(search.rbtdb, node, isc_rwlocktype_read); *nodep = node; } if ((search.rbtversion->secure == dns_db_secure && @@ -4300,10 +4310,10 @@ found: (search.options & DNS_DBFIND_FORCENSEC) != 0) { bind_rdataset(search.rbtdb, node, nsecheader, 0, - rdataset); + isc_rwlocktype_read, rdataset); if (nsecsig != NULL) { bind_rdataset(search.rbtdb, node, nsecsig, 0, - sigrdataset); + isc_rwlocktype_read, sigrdataset); } } if (wild) { @@ -4376,7 +4386,7 @@ found: if (nodep != NULL) { if (!at_zonecut) { - new_reference(search.rbtdb, node); + new_reference(search.rbtdb, node, isc_rwlocktype_read); } else { search.need_cleanup = false; } @@ -4384,10 +4394,11 @@ found: } if (type != dns_rdatatype_any) { - bind_rdataset(search.rbtdb, node, found, 0, rdataset); + bind_rdataset(search.rbtdb, node, found, 0, isc_rwlocktype_read, + rdataset); if (foundsig != NULL) { bind_rdataset(search.rbtdb, node, foundsig, 0, - sigrdataset); + isc_rwlocktype_read, sigrdataset); } } @@ -4570,8 +4581,7 @@ cache_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { * We increment the reference count on node to ensure that * search->zonecut_rdataset will still be valid later. */ - new_reference(search->rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search->rbtdb, node, locktype); search->zonecut = node; search->zonecut_rdataset = dname_header; search->zonecut_sigrdataset = sigdname_header; @@ -4679,14 +4689,15 @@ find_deepest_zonecut(rbtdb_search_t *search, dns_rbtnode_t *node, } result = DNS_R_DELEGATION; if (nodep != NULL) { - new_reference(search->rbtdb, node); + new_reference(search->rbtdb, node, locktype); *nodep = node; } bind_rdataset(search->rbtdb, node, found, search->now, - rdataset); + locktype, rdataset); if (foundsig != NULL) { bind_rdataset(search->rbtdb, node, foundsig, - search->now, sigrdataset); + search->now, locktype, + sigrdataset); } if (need_headerupdate(found, search->now) || (foundsig != NULL && @@ -4795,13 +4806,13 @@ find_coveringnsec(rbtdb_search_t *search, dns_dbnode_t **nodep, if (result != ISC_R_SUCCESS) { goto unlock_node; } - bind_rdataset(search->rbtdb, node, found, now, + bind_rdataset(search->rbtdb, node, found, now, locktype, rdataset); if (foundsig != NULL) { bind_rdataset(search->rbtdb, node, foundsig, - now, sigrdataset); + now, locktype, sigrdataset); } - new_reference(search->rbtdb, node); + new_reference(search->rbtdb, node, locktype); *nodep = node; result = DNS_R_COVERINGNSEC; } else if (!empty_node) { @@ -5026,18 +5037,18 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, if ((search.options & DNS_DBFIND_COVERINGNSEC) != 0 && nsecheader != NULL) { if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } bind_rdataset(search.rbtdb, node, nsecheader, - search.now, rdataset); + search.now, locktype, rdataset); if (need_headerupdate(nsecheader, search.now)) { update = nsecheader; } if (nsecsig != NULL) { bind_rdataset(search.rbtdb, node, nsecsig, - search.now, sigrdataset); + search.now, locktype, + sigrdataset); if (need_headerupdate(nsecsig, search.now)) { updatesig = nsecsig; } @@ -5052,18 +5063,18 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nsheader != NULL) { if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } bind_rdataset(search.rbtdb, node, nsheader, search.now, - rdataset); + locktype, rdataset); if (need_headerupdate(nsheader, search.now)) { update = nsheader; } if (nssig != NULL) { bind_rdataset(search.rbtdb, node, nssig, - search.now, sigrdataset); + search.now, locktype, + sigrdataset); if (need_headerupdate(nssig, search.now)) { updatesig = nssig; } @@ -5084,8 +5095,7 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } @@ -5117,13 +5127,14 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, if (type != dns_rdatatype_any || result == DNS_R_NCACHENXDOMAIN || result == DNS_R_NCACHENXRRSET) { - bind_rdataset(search.rbtdb, node, found, search.now, rdataset); + bind_rdataset(search.rbtdb, node, found, search.now, locktype, + rdataset); if (need_headerupdate(found, search.now)) { update = found; } if (!NEGATIVE(found) && foundsig != NULL) { bind_rdataset(search.rbtdb, node, foundsig, search.now, - sigrdataset); + locktype, sigrdataset); if (need_headerupdate(foundsig, search.now)) { updatesig = foundsig; } @@ -5282,15 +5293,15 @@ cache_findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, } if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } - bind_rdataset(search.rbtdb, node, found, search.now, rdataset); + bind_rdataset(search.rbtdb, node, found, search.now, locktype, + rdataset); if (foundsig != NULL) { bind_rdataset(search.rbtdb, node, foundsig, search.now, - sigrdataset); + locktype, sigrdataset); } if (need_headerupdate(found, search.now) || @@ -5653,10 +5664,11 @@ zone_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } } if (found != NULL) { - bind_rdataset(rbtdb, rbtnode, found, now, rdataset); + bind_rdataset(rbtdb, rbtnode, found, now, isc_rwlocktype_read, + rdataset); if (foundsig != NULL) { bind_rdataset(rbtdb, rbtnode, foundsig, now, - sigrdataset); + isc_rwlocktype_read, sigrdataset); } } @@ -5747,9 +5759,9 @@ cache_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } } if (found != NULL) { - bind_rdataset(rbtdb, rbtnode, found, now, rdataset); + bind_rdataset(rbtdb, rbtnode, found, now, locktype, rdataset); if (!NEGATIVE(found) && foundsig != NULL) { - bind_rdataset(rbtdb, rbtnode, foundsig, now, + bind_rdataset(rbtdb, rbtnode, foundsig, now, locktype, sigrdataset); } } @@ -5917,6 +5929,9 @@ resign_insert(dns_rbtdb_t *rbtdb, int idx, rdatasetheader_t *newheader) { return (result); } +/* + * node write lock must be held. + */ static void resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version, rdatasetheader_t *header) { @@ -5928,7 +5943,8 @@ resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version, header->heap_index); header->heap_index = 0; if (version != NULL) { - new_reference(rbtdb, header->node); + new_reference(rbtdb, header->node, + isc_rwlocktype_write); ISC_LIST_APPEND(version->resigned_list, header, link); } } @@ -5959,6 +5975,9 @@ update_recordsandxfrsize(bool add, rbtdb_version_t *rbtversion, RWUNLOCK(&rbtversion->rwlock, isc_rwlocktype_write); } +/* + * write lock on rbtnode must be held. + */ static isc_result_t add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, const dns_name_t *nodename, rbtdb_version_t *rbtversion, rdatasetheader_t *newheader, @@ -6085,9 +6104,11 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, const dns_name_t *nodename, free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) { - bind_rdataset(rbtdb, rbtnode, - topheader, now, - addedrdataset); + bind_rdataset( + rbtdb, rbtnode, + topheader, now, + isc_rwlocktype_write, + addedrdataset); } return (DNS_R_UNCHANGED); } @@ -6147,6 +6168,7 @@ find_header: free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) { bind_rdataset(rbtdb, rbtnode, header, now, + isc_rwlocktype_write, addedrdataset); } return (DNS_R_UNCHANGED); @@ -6258,6 +6280,7 @@ find_header: free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) { bind_rdataset(rbtdb, rbtnode, header, now, + isc_rwlocktype_write, addedrdataset); } return (ISC_R_SUCCESS); @@ -6307,6 +6330,7 @@ find_header: free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) { bind_rdataset(rbtdb, rbtnode, header, now, + isc_rwlocktype_write, addedrdataset); } return (ISC_R_SUCCESS); @@ -6504,7 +6528,8 @@ find_header: } if (addedrdataset != NULL) { - bind_rdataset(rbtdb, rbtnode, newheader, now, addedrdataset); + bind_rdataset(rbtdb, rbtnode, newheader, now, + isc_rwlocktype_write, addedrdataset); } return (ISC_R_SUCCESS); @@ -7045,13 +7070,15 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } if (result == ISC_R_SUCCESS && newrdataset != NULL) { - bind_rdataset(rbtdb, rbtnode, newheader, 0, newrdataset); + bind_rdataset(rbtdb, rbtnode, newheader, 0, + isc_rwlocktype_write, newrdataset); } if (result == DNS_R_NXRRSET && newrdataset != NULL && (options & DNS_DBSUB_WANTOLD) != 0) { - bind_rdataset(rbtdb, rbtnode, header, 0, newrdataset); + bind_rdataset(rbtdb, rbtnode, header, 0, isc_rwlocktype_write, + newrdataset); } unlock: @@ -7929,8 +7956,7 @@ getoriginnode(dns_db_t *db, dns_dbnode_t **nodep) { /* Note that the access to origin_node doesn't require a DB lock */ onode = (dns_rbtnode_t *)rbtdb->origin_node; if (onode != NULL) { - new_reference(rbtdb, onode); - + new_reference(rbtdb, onode, isc_rwlocktype_none); *nodep = rbtdb->origin_node; } else { INSIST(IS_CACHE(rbtdb)); @@ -8123,7 +8149,8 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, dns_name_t *foundname) { * Found something; pass back the answer and unlock * the bucket. */ - bind_rdataset(rbtdb, header->node, header, 0, rdataset); + bind_rdataset(rbtdb, header->node, header, 0, + isc_rwlocktype_read, rdataset); if (foundname != NULL) { dns_rbt_fullnamefromnode(header->node, foundname); @@ -9130,7 +9157,7 @@ rdatasetiter_current(dns_rdatasetiter_t *iterator, dns_rdataset_t *rdataset) { isc_rwlocktype_read); bind_rdataset(rbtdb, rbtnode, header, rbtiterator->common.now, - rdataset); + isc_rwlocktype_read, rdataset); NODE_UNLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, isc_rwlocktype_read); @@ -9585,7 +9612,7 @@ dbiterator_current(dns_dbiterator_t *iterator, dns_dbnode_t **nodep, result = ISC_R_SUCCESS; } - new_reference(rbtdb, node); + new_reference(rbtdb, node, isc_rwlocktype_none); *nodep = rbtdbiter->node; @@ -10498,7 +10525,7 @@ expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, bool tree_locked, * We first need to gain a new reference to the node to meet a * requirement of decrement_reference(). */ - new_reference(rbtdb, header->node); + new_reference(rbtdb, header->node, isc_rwlocktype_write); decrement_reference(rbtdb, header->node, 0, isc_rwlocktype_write, tree_locked ? isc_rwlocktype_write From 8e88a6f0ad64e2a2f9513ea1b1a19d5b6daa509e Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 2 Jun 2020 15:11:05 +1000 Subject: [PATCH 2/3] Add release note for #1718 --- 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 ae1bf3785b..6fc84ba751 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -28,6 +28,10 @@ Security Fixes - It was possible to trigger an assertion when attempting to fill an oversized TCP buffer. This was disclosed in CVE-2020-8618. [GL #1850] +- It was possible to trigger an INSIST failure when a zone with an + interior wildcard label was queried in a certain pattern. This was + disclosed in CVE-2020-8619. [GL #1111] [GL #1718] + Known Issues ~~~~~~~~~~~~ From 7f928f4afb52dc7d753a6111918d1cebf2675a5c Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 2 Jun 2020 13:50:10 +1000 Subject: [PATCH 3/3] Add CHANGES entry for #1718 --- CHANGES | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index a6ff574b03..036f2e826b 100644 --- a/CHANGES +++ b/CHANGES @@ -13,7 +13,10 @@ 5435. [placeholder] -5434. [placeholder] +5434. [security] It was possible to trigger an INSIST in + lib/dns/rbtdb.c:new_reference() with a particular zone + content and query patterns. (CVE-2020-8619) [GL #1111] + [GL #1718] 5433. [placeholder]