From 053b51c4dbd28f6e4de71ce4268a6f606025d76d Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 3 Jan 2018 19:11:18 -0800 Subject: [PATCH] [master] block validator deadlock and prevent use-after-free 4859. [bug] A loop was possible when attempting to validate unsigned CNAME responses from secure zones; this caused a delay in returning SERVFAIL and also increased the chances of encountering CVE-2017-3145. [RT #46839] 4858. [security] Addresses could be referenced after being freed in resolver.c, causing an assertion failure. (CVE-2017-3145) [RT #46839] --- CHANGES | 10 ++++++++++ doc/arm/notes.xml | 12 ++++++++++-- lib/dns/resolver.c | 39 ++++++++++++++++++++++++--------------- lib/dns/validator.c | 3 ++- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/CHANGES b/CHANGES index 5d308d22bc..077a0ff07c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,13 @@ +4859. [bug] A loop was possible when attempting to validate + unsigned CNAME responses from secure zones; + this caused a delay in returning SERVFAIL and + also increased the chances of encountering + CVE-2017-3145. [RT #46839] + +4858. [security] Addresses could be referenced after being freed + in resolver.c, causing an assertion failure. + (CVE-2017-3145) [RT #46839] + 4857. [bug] Maintain attach/detach semantics for event->db, event->node, event->rdataset and event->sigrdataset in query.c. [RT #46891] diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml index 7f2a702ee8..ad0e3ccdf6 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -40,7 +40,11 @@ - None. + Addresses could be referenced after being freed during resolver + processing, causing an assertion failure. The chances of this + happening were remote, but the introduction of a delay in + resolution increasred them. This bug is disclosed in + CVE-2017-3145. [RT #46839] @@ -73,7 +77,11 @@ - None. + Attempting to validate improperly unsigned CNAME responses + from secure zones could cause a validator loop. This caused + a delay in returning SERVFAIL and also increased the chances + of encountering the crash bug described in CVE-2017-3145. + [RT #46839] diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index e5899d0a26..77312a85b0 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -1051,7 +1051,7 @@ fctx_stoptimer(fetchctx_t *fctx) { * cannot fail in that case. */ result = isc_timer_reset(fctx->timer, isc_timertype_inactive, - NULL, NULL, ISC_TRUE); + NULL, NULL, ISC_TRUE); if (result != ISC_R_SUCCESS) { UNEXPECTED_ERROR(__FILE__, __LINE__, "isc_timer_reset(): %s", @@ -1059,7 +1059,6 @@ fctx_stoptimer(fetchctx_t *fctx) { } } - static inline isc_result_t fctx_startidletimer(fetchctx_t *fctx, isc_interval_t *interval) { /* @@ -1336,7 +1335,8 @@ fctx_cleanupfinds(fetchctx_t *fctx) { for (find = ISC_LIST_HEAD(fctx->finds); find != NULL; - find = next_find) { + find = next_find) + { next_find = ISC_LIST_NEXT(find, publink); ISC_LIST_UNLINK(fctx->finds, find, publink); dns_adb_destroyfind(&find); @@ -1352,7 +1352,8 @@ fctx_cleanupaltfinds(fetchctx_t *fctx) { for (find = ISC_LIST_HEAD(fctx->altfinds); find != NULL; - find = next_find) { + find = next_find) + { next_find = ISC_LIST_NEXT(find, publink); ISC_LIST_UNLINK(fctx->altfinds, find, publink); dns_adb_destroyfind(&find); @@ -1368,7 +1369,8 @@ fctx_cleanupforwaddrs(fetchctx_t *fctx) { for (addr = ISC_LIST_HEAD(fctx->forwaddrs); addr != NULL; - addr = next_addr) { + addr = next_addr) + { next_addr = ISC_LIST_NEXT(addr, publink); ISC_LIST_UNLINK(fctx->forwaddrs, addr, publink); dns_adb_freeaddrinfo(fctx->adb, &addr); @@ -1383,7 +1385,8 @@ fctx_cleanupaltaddrs(fetchctx_t *fctx) { for (addr = ISC_LIST_HEAD(fctx->altaddrs); addr != NULL; - addr = next_addr) { + addr = next_addr) + { next_addr = ISC_LIST_NEXT(addr, publink); ISC_LIST_UNLINK(fctx->altaddrs, addr, publink); dns_adb_freeaddrinfo(fctx->adb, &addr); @@ -1391,16 +1394,20 @@ fctx_cleanupaltaddrs(fetchctx_t *fctx) { } static inline void -fctx_stopeverything(fetchctx_t *fctx, isc_boolean_t no_response, - isc_boolean_t age_untried) +fctx_stopqueries(fetchctx_t *fctx, isc_boolean_t no_response, + isc_boolean_t age_untried) { - FCTXTRACE("stopeverything"); + FCTXTRACE("stopqueries"); fctx_cancelqueries(fctx, no_response, age_untried); + fctx_stoptimer(fctx); +} + +static inline void +fctx_cleanupall(fetchctx_t *fctx) { fctx_cleanupfinds(fctx); fctx_cleanupaltfinds(fctx); fctx_cleanupforwaddrs(fctx); fctx_cleanupaltaddrs(fctx); - fctx_stoptimer(fctx); } static void @@ -1651,7 +1658,8 @@ fctx_done(fetchctx_t *fctx, isc_result_t result, int line) { age_untried = ISC_TRUE; fctx->reason = NULL; - fctx_stopeverything(fctx, no_response, age_untried); + + fctx_stopqueries(fctx, no_response, age_untried); LOCK(&res->buckets[fctx->bucketnum].lock); @@ -4271,11 +4279,12 @@ fctx_doshutdown(isc_task_t *task, isc_event_t *event) { dns_resolver_cancelfetch(fctx->nsfetch); /* - * Shut down anything that is still running on behalf of this - * fetch. To avoid deadlock with the ADB, we must do this - * before we lock the bucket lock. + * Shut down anything still running on behalf of this + * fetch, and clean up finds and addresses. To avoid deadlock + * with the ADB, we must do this before we lock the bucket lock. */ - fctx_stopeverything(fctx, ISC_FALSE, ISC_FALSE); + fctx_stopqueries(fctx, ISC_FALSE, ISC_FALSE); + fctx_cleanupall(fctx); LOCK(&res->buckets[bucketnum].lock); diff --git a/lib/dns/validator.c b/lib/dns/validator.c index db9d4baf0d..463edb1ce2 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1100,7 +1100,8 @@ check_deadlock(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, for (parent = val; parent != NULL; parent = parent->parent) { if (parent->event != NULL && - parent->event->type == type && + (parent->event->type == type || + parent->event->type == dns_rdatatype_cname) && dns_name_equal(parent->event->name, name) && /* * As NSEC3 records are meta data you sometimes