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

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]

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

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);
 

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