From: Michał Kępień <michal@isc.org>
Date: Thu, 19 Jul 2018 15:43:58 +0000 (+0200)
Subject: Fix handling of TAT sending failures
X-Git-Tag: v9.13.3~101^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8666f8d28f58db5ea37cd5848b7c3c0ea5d07bd9;p=thirdparty%2Fbind9.git

Fix handling of TAT sending failures

dns_view_zonecut() may associate the dns_rdataset_t structure passed to
it even if it returns a result different then ISC_R_SUCCESS.  Not
handling this properly may cause a reference leak.  Fix by ensuring
'nameservers' is cleaned up in all relevant failure modes.
---

diff --git a/bin/named/server.c b/bin/named/server.c
index a21ea705979..97b7b8dd7e3 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -6646,25 +6646,32 @@ dotat(dns_keytable_t *keytable, dns_keynode_t *keynode, void *arg) {
 	dns_rdataset_init(&nameservers);
 	result = dns_view_findzonecut(view, origin, domain, 0, 0, ISC_TRUE,
 				      ISC_TRUE, &nameservers, NULL);
-	if (result != ISC_R_SUCCESS) {
-		goto done;
+	if (result == ISC_R_SUCCESS) {
+		result = dns_resolver_createfetch(view->resolver, tatname,
+						  dns_rdatatype_null, domain,
+						  &nameservers, NULL, NULL, 0,
+						  0, 0, NULL, tat->task,
+						  tat_done, tat,
+						  &tat->rdataset,
+						  &tat->sigrdataset,
+						  &tat->fetch);
 	}
 
-	result = dns_resolver_createfetch(view->resolver, tatname,
-					  dns_rdatatype_null, domain,
-					  &nameservers, NULL, NULL, 0, 0, 0,
-					  NULL, tat->task, tat_done, tat,
-					  &tat->rdataset, &tat->sigrdataset,
-					  &tat->fetch);
-
 	/*
-	 * dns_resolver_createfetch() creates its own copies of 'domain' and
-	 * 'nameservers'; clean up the latter (the former points into a
-	 * dst_key_t structure and thus must not be freed).
+	 * 'domain' holds the dns_name_t pointer inside a dst_key_t structure.
+	 * dns_resolver_createfetch() creates its own copy of 'domain' if it
+	 * succeeds.  Thus, 'domain' is not freed here.
+	 *
+	 * Even if dns_view_findzonecut() returned something else than
+	 * ISC_R_SUCCESS, it still could have associated 'nameservers'.
+	 * dns_resolver_createfetch() creates its own copy of 'nameservers' if
+	 * it succeeds.  Thus, we need to check whether 'nameservers' is
+	 * associated and release it if it is.
 	 */
-	dns_rdataset_disassociate(&nameservers);
+	if (dns_rdataset_isassociated(&nameservers)) {
+		dns_rdataset_disassociate(&nameservers);
+	}
 
- done:
 	if (result != ISC_R_SUCCESS) {
 		isc_task_detach(&tat->task);
 		isc_mem_putanddetach(&tat->mctx, tat, sizeof(*tat));
diff --git a/bin/tests/system/mirror/ns1/sign.sh b/bin/tests/system/mirror/ns1/sign.sh
index 4a09a8619bd..0382585541a 100644
--- a/bin/tests/system/mirror/ns1/sign.sh
+++ b/bin/tests/system/mirror/ns1/sign.sh
@@ -27,4 +27,10 @@ cat $infile $keyname1.key $keyname2.key > $zonefile
 
 $SIGNER -P -g -o $zone $zonefile > /dev/null
 
-keyfile_to_trusted_keys $keyname1 > trusted.conf
+# Add a trust anchor for a name whose non-existence can be securely proved
+# without recursing when the root zone is mirrored.  This will exercise code
+# attempting to send TAT queries for such names (in ns3).  Key data is
+# irrelevant here, so just reuse the root zone key generated above.
+sed "s/^\./nonexistent./;" $keyname1.key > $keyname1.modified.key
+
+keyfile_to_trusted_keys $keyname1 $keyname1.modified > trusted.conf