[v9_11] 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]

diff --git a/CHANGES b/CHANGES
index 8bfb51c0ef1e4d2db1570936d938d057923b8cbc..e515d197e77db87dee77ad1eb43eb626b77a1f7c 100644 (file)
--- 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 f41814c5ae01bf5fe637327a07a5b291ba34e076..4ba5e7c5aa854e6a5150af7229c188dead1d5679 100644 (file)
--- a/doc/arm/notes.xml
+++ b/doc/arm/notes.xml
@@ -123,6 +123,15 @@
          [RT #45181]
        </para>
       </listitem>
+      <listitem>
+       <para>
+         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]
+       </para>
+      </listitem>
     </itemizedlist>
   </section>
 
@@ -222,6 +231,15 @@
 
   <section xml:id="relnotes_bugs"><info><title>Bug Fixes</title></info>
     <itemizedlist>
+      <listitem>
+       <para>
+         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]
+       </para>
+      </listitem>
       <listitem>
        <para>
          When <command>named</command> was reconfigured, failure of some

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 8cfcb5b5c75b3f0a48a6242d254cb47a8c6de159..c5ecb4e054e990fc9dc0fe6e0a1d2b84da896461 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -831,7 +831,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",
@@ -839,7 +839,6 @@ fctx_stoptimer(fetchctx_t *fctx) {
        }
 }
 
-
 static inline isc_result_t
 fctx_startidletimer(fetchctx_t *fctx, isc_interval_t *interval) {
        /*
@@ -1116,7 +1115,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);
@@ -1132,7 +1132,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);
@@ -1148,7 +1149,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);
@@ -1163,7 +1165,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);
@@ -1171,16 +1174,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
@@ -1431,7 +1438,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);
 
@@ -4026,11 +4034,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 4e2faa988dc7486bd64b859bfc1d4adee8729d06..e7b0c5d45869ed1bb2f4843f28219b34989994ae 100644 (file)
--- a/lib/dns/validator.c
+++ b/lib/dns/validator.c
@@ -1099,7 +1099,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