From a3c2ad2ee509ae7d03bccdc8bed467f67da498ef Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 7 Aug 2020 18:00:41 +1000 Subject: [PATCH] Address use after free between view, resolver and nta. Hold a weak reference to the view so that it can't go away while nta is performing its lookups. Cancel nta timers once all external references to the view have gone to prevent them triggering new work. (cherry picked from commit 0b2555e8cfc187cb481080c06229a4eac416355d) --- CHANGES | 4 ++ lib/dns/include/dns/nta.h | 8 ++++ lib/dns/nta.c | 75 +++++++++++++++++++++++++++++-------- lib/dns/view.c | 3 ++ lib/dns/win32/libdns.def.in | 1 + 5 files changed, 76 insertions(+), 15 deletions(-) diff --git a/CHANGES b/CHANGES index 50390bb0c0..769e5676d3 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5488. [bug] nta needed to have a weak reference on view to prevent + the view being deleted while nta tests are being + performed. [GL #2067] + 5474. [bug] dns_rdata_hip_next() failed to return ISC_R_NOMORE when it should have. [GL !3880] diff --git a/lib/dns/include/dns/nta.h b/lib/dns/include/dns/nta.h index edbaa77ccf..59d4765f9f 100644 --- a/lib/dns/include/dns/nta.h +++ b/lib/dns/include/dns/nta.h @@ -52,6 +52,7 @@ struct dns_ntatable { /* Locked by rwlock. */ uint32_t references; dns_rbt_t *table; + bool shuttingdown; }; #define NTATABLE_MAGIC ISC_MAGIC('N', 'T', 'A', 't') @@ -199,6 +200,13 @@ dns_ntatable_save(dns_ntatable_t *ntatable, FILE *fp); /*%< * Save the NTA table to the file opened as 'fp', for later loading. */ + +void +dns_ntatable_shutdown(dns_ntatable_t *ntatable); +/*%< + * Cancel future checks to see if NTAs can be removed. + */ + ISC_LANG_ENDDECLS #endif /* DNS_NTA_H */ diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 7c790db229..fef1b2da0f 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -132,6 +132,7 @@ dns_ntatable_create(dns_view_t *view, if (result != ISC_R_SUCCESS) goto cleanup_rbt; + ntatable->shuttingdown = false; ntatable->timermgr = timermgr; ntatable->taskmgr = taskmgr; @@ -248,27 +249,31 @@ fetch_done(isc_task_t *task, isc_event_t *event) { (void) isc_timer_reset(nta->timer, isc_timertype_inactive, NULL, NULL, true); nta_detach(view->mctx, &nta); + dns_view_weakdetach(&view); } static void checkbogus(isc_task_t *task, isc_event_t *event) { dns_nta_t *nta = event->ev_arg; dns_ntatable_t *ntatable = nta->ntatable; - dns_view_t *view = ntatable->view; + dns_view_t *view = NULL; isc_result_t result; if (nta->fetch != NULL) { dns_resolver_cancelfetch(nta->fetch); nta->fetch = NULL; } - if (dns_rdataset_isassociated(&nta->rdataset)) + if (dns_rdataset_isassociated(&nta->rdataset)) { dns_rdataset_disassociate(&nta->rdataset); - if (dns_rdataset_isassociated(&nta->sigrdataset)) + } + if (dns_rdataset_isassociated(&nta->sigrdataset)) { dns_rdataset_disassociate(&nta->sigrdataset); + } isc_event_free(&event); nta_ref(nta); + dns_view_weakattach(ntatable->view, &view); result = dns_resolver_createfetch(view->resolver, nta->name, dns_rdatatype_nsec, NULL, NULL, NULL, @@ -277,8 +282,10 @@ checkbogus(isc_task_t *task, isc_event_t *event) { &nta->rdataset, &nta->sigrdataset, &nta->fetch); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { + dns_view_weakdetach(&view); nta_detach(view->mctx, &nta); + } } static isc_result_t @@ -342,11 +349,10 @@ nta_create(dns_ntatable_t *ntatable, dns_name_t *name, dns_nta_t **target) { } isc_result_t -dns_ntatable_add(dns_ntatable_t *ntatable, dns_name_t *name, - bool force, isc_stdtime_t now, - uint32_t lifetime) +dns_ntatable_add(dns_ntatable_t *ntatable, dns_name_t *name, bool force, + isc_stdtime_t now, uint32_t lifetime) { - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; dns_nta_t *nta = NULL; dns_rbtnode_t *node; dns_view_t *view; @@ -355,27 +361,34 @@ dns_ntatable_add(dns_ntatable_t *ntatable, dns_name_t *name, view = ntatable->view; + RWLOCK(&ntatable->rwlock, isc_rwlocktype_write); + + if (ntatable->shuttingdown) { + goto unlock; + } + result = nta_create(ntatable, name, &nta); - if (result != ISC_R_SUCCESS) - return (result); + if (result != ISC_R_SUCCESS) { + goto unlock; + } nta->expiry = now + lifetime; nta->forced = force; - RWLOCK(&ntatable->rwlock, isc_rwlocktype_write); - node = NULL; result = dns_rbt_addnode(ntatable->table, name, &node); if (result == ISC_R_SUCCESS) { - if (!force) + if (!force) { (void)settimer(ntatable, nta, lifetime); + } node->data = nta; nta = NULL; } else if (result == ISC_R_EXISTS) { dns_nta_t *n = node->data; if (n == NULL) { - if (!force) + if (!force) { (void)settimer(ntatable, nta, lifetime); + } node->data = nta; nta = NULL; } else { @@ -385,10 +398,12 @@ dns_ntatable_add(dns_ntatable_t *ntatable, dns_name_t *name, result = ISC_R_SUCCESS; } +unlock: RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_write); - if (nta != NULL) + if (nta != NULL) { nta_detach(view->mctx, &nta); + } return (result); } @@ -720,3 +735,33 @@ dns_ntatable_save(dns_ntatable_t *ntatable, FILE *fp) { else return (written ? ISC_R_SUCCESS : ISC_R_NOTFOUND); } + +void +dns_ntatable_shutdown(dns_ntatable_t *ntatable) { + isc_result_t result; + dns_rbtnode_t *node; + dns_rbtnodechain_t chain; + + REQUIRE(VALID_NTATABLE(ntatable)); + + RWLOCK(&ntatable->rwlock, isc_rwlocktype_write); + ntatable->shuttingdown = true; + + dns_rbtnodechain_init(&chain, ntatable->view->mctx); + result = dns_rbtnodechain_first(&chain, ntatable->table, NULL, NULL); + while (result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN) { + dns_rbtnodechain_current(&chain, NULL, NULL, &node); + if (node->data != NULL) { + dns_nta_t *nta = (dns_nta_t *)node->data; + if (nta->timer != NULL) { + (void)isc_timer_reset(nta->timer, + isc_timertype_inactive, + NULL, NULL, true); + } + } + result = dns_rbtnodechain_next(&chain, NULL, NULL); + } + + dns_rbtnodechain_invalidate(&chain); + RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_write); +} diff --git a/lib/dns/view.c b/lib/dns/view.c index 0fca1d934d..c1ece2e682 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -626,6 +626,9 @@ view_flushanddetach(dns_view_t **viewp, bool flush) { if (view->catzs != NULL) { dns_catz_catzs_detach(&view->catzs); } + if (view->ntatable_priv != NULL) { + dns_ntatable_shutdown(view->ntatable_priv); + } UNLOCK(&view->lock); /* Need to detach zones outside view lock */ diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 63be973457..b426f150dd 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -668,6 +668,7 @@ dns_ntatable_delete dns_ntatable_detach dns_ntatable_dump dns_ntatable_save +dns_ntatable_shutdown dns_ntatable_totext dns_opcode_totext dns_opcodestats_create