Merge branch '185-public-fix' into 'master'

Resolve "[CVE-2018-5737] serve-stale crash"

Closes #185

See merge request isc-projects/bind9!302

CHANGES

@@ -49,7 +49,10 @@
 4935.	[func]		Add support for LibreSSL >= 2.7.0 (some OpenSSL 1.1.0
 			call were added). [GL #191]
 
-4934.	[placeholder]
+4934.	[security]	Simultaneous use of stale cache records and NSEC
+			aggressive negative caching could trigger a recursion
+			loop. (CVE-2018-5737) [GL #185]
+
 
 4933.	[bug]		Not creating signing keys for an inline signed zone
 			prevented changes applied to the raw zone from being
@@ -121,10 +124,10 @@
 			like dig without IDN support.  libidn2 version 2.0
 			or higher is needed for +idnout enabled by default.
 
-4914.	[bug]		A bug in zone database reference counting could lead to
+4914.	[security]	A bug in zone database reference counting could lead to
 			a crash when multiple versions of a slave zone were
 			transferred from a master in close succession.
-			[GL #134]
+			(CVE-2018-5736) [GL #134]
 
 4913.	[test]		Re-implemented older unit tests in bin/tests as ATF,
 			removed the lib/tests unit testing library. [GL #115]

lib/dns/rbtdb.c

@@ -4494,6 +4494,7 @@ check_stale_header(dns_rbtnode_t *node, rdatasetheader_t *header,
 		 */
 		if (KEEPSTALE(search->rbtdb) && stale > search->now) {
 			header->attributes |= RDATASET_ATTR_STALE;
+			*header_prev = header;
 			return ((search->options & DNS_DBFIND_STALEOK) == 0);
 		}
 

lib/ns/include/ns/query.h

@@ -34,6 +34,18 @@ typedef struct ns_dbversion {
 	ISC_LINK(struct ns_dbversion)	link;
 } ns_dbversion_t;
 
+/*%
+ * nameserver recursion parameters, to uniquely identify a recursion
+ * query; this is used to detect a recursion loop
+ */
+typedef struct ns_query_recparam {
+	dns_rdatatype_t			qtype;
+	dns_name_t *			qname;
+	dns_fixedname_t			fqname;
+	dns_name_t *			qdomain;
+	dns_fixedname_t			fqdomain;
+} ns_query_recparam_t;
+
 /*% nameserver query structure */
 struct ns_query {
 	unsigned int			attributes;
@@ -62,6 +74,7 @@ struct ns_query {
 	unsigned int			dns64_aaaaoklen;
 	unsigned int			dns64_options;
 	unsigned int			dns64_ttl;
+
 	struct {
 		dns_db_t *      	db;
 		dns_zone_t *      	zone;
@@ -76,6 +89,8 @@ struct ns_query {
 		isc_boolean_t		is_zone;
 	} redirect;
 
+	ns_query_recparam_t		recparam;
+
 	dns_keytag_t root_key_sentinel_keyid;
 	isc_boolean_t root_key_sentinel_is_ta;
 	isc_boolean_t root_key_sentinel_not_ta;

lib/ns/query.c

@@ -347,6 +347,10 @@ query_lookup(query_ctx_t *qctx);
 static void
 fetch_callback(isc_task_t *task, isc_event_t *event);
 
+static void
+recparam_update(ns_query_recparam_t *param, dns_rdatatype_t qtype,
+		const dns_name_t *qname, const dns_name_t *qdomain);
+
 static isc_result_t
 query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
 	      dns_name_t *qdomain, dns_rdataset_t *nameservers,
@@ -701,6 +705,7 @@ query_reset(ns_client_t *client, isc_boolean_t everything) {
 	client->query.isreferral = ISC_FALSE;
 	client->query.dns64_options = 0;
 	client->query.dns64_ttl = ISC_UINT32_MAX;
+	recparam_update(&client->query.recparam, 0, NULL, NULL);
 	client->query.root_key_sentinel_keyid = 0;
 	client->query.root_key_sentinel_is_ta = ISC_FALSE;
 	client->query.root_key_sentinel_not_ta = ISC_FALSE;
@@ -5593,6 +5598,54 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 	dns_resolver_destroyfetch(&fetch);
 }
 
+/*%
+ * Check whether the recursion parameters in 'param' match the current query's
+ * recursion parameters provided in 'qtype', 'qname', and 'qdomain'.
+ */
+static isc_boolean_t
+recparam_match(const ns_query_recparam_t *param, dns_rdatatype_t qtype,
+	       const dns_name_t *qname, const dns_name_t *qdomain)
+{
+	REQUIRE(param != NULL);
+
+	return (ISC_TF(param->qtype == qtype &&
+		       param->qname != NULL && qname != NULL &&
+		       param->qdomain != NULL && qdomain != NULL &&
+		       dns_name_equal(param->qname, qname) &&
+		       dns_name_equal(param->qdomain, qdomain)));
+}
+
+/*%
+ * Update 'param' with current query's recursion parameters provided in
+ * 'qtype', 'qname', and 'qdomain'.
+ */
+static void
+recparam_update(ns_query_recparam_t *param, dns_rdatatype_t qtype,
+		const dns_name_t *qname, const dns_name_t *qdomain)
+{
+	isc_result_t result;
+
+	REQUIRE(param != NULL);
+
+	param->qtype = qtype;
+
+	if (qname == NULL) {
+		param->qname = NULL;
+	} else {
+		param->qname = dns_fixedname_initname(&param->fqname);
+		result = dns_name_copy(qname, param->qname, NULL);
+		RUNTIME_CHECK(result == ISC_R_SUCCESS);
+	}
+
+	if (qdomain == NULL) {
+		param->qdomain = NULL;
+	} else {
+		param->qdomain = dns_fixedname_initname(&param->fqdomain);
+		result = dns_name_copy(qdomain, param->qdomain, NULL);
+		RUNTIME_CHECK(result == ISC_R_SUCCESS);
+	}
+}
+
 /*%
  * Prepare client for recursion, then create a resolver fetch, with
  * the event callback set to fetch_callback(). Afterward we terminate
@@ -5610,6 +5663,19 @@ query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
 
 	CTRACE(ISC_LOG_DEBUG(3), "query_recurse");
 
+	/*
+	 * Check recursion parameters from the previous query to see if they
+	 * match.  If not, update recursion parameters and proceed.
+	 */
+	if (recparam_match(&client->query.recparam, qtype, qname, qdomain)) {
+		ns_client_log(client, NS_LOGCATEGORY_CLIENT,
+			      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
+			      "recursion loop detected");
+		return (ISC_R_FAILURE);
+	}
+
+	recparam_update(&client->query.recparam, qtype, qname, qdomain);
+
 	if (!resuming)
 		inc_stats(client, ns_statscounter_recursion);