From 391fac1fc8d2e470287b5cc4344b3adb90c6f54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 27 Apr 2018 09:13:26 +0200 Subject: [PATCH 1/3] Prevent check_stale_header() from leaking rdataset headers check_stale_header() fails to update the pointer to the previous header while processing rdataset headers eligible for serve-stale, thus enabling rdataset headers to be leaked (i.e. disassociated from a node and left on the relevant TTL heap) while iterating through a node. This can lead to several different assertion failures. Add the missing pointer update. --- lib/dns/rbtdb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 64bd7a93bf..b64350c62d 100644 --- a/lib/dns/rbtdb.c +++ b/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); } From 46bb4dd124ed031d4c219d1e37a3c6322092e30c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Fri, 27 Apr 2018 09:13:26 +0200 Subject: [PATCH 2/3] Detect recursion loops during query processing Interrupt query processing when query_recurse() attempts to ask the same name servers for the same QNAME/QTYPE tuple for two times in a row as this indicates that query processing may be stuck for an indeterminate period of time, e.g. due to interactions between features able to restart query_lookup(). --- lib/ns/include/ns/query.h | 15 +++++++++ lib/ns/query.c | 66 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index 365cb8c754..c9d0cfb43c 100644 --- a/lib/ns/include/ns/query.h +++ b/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; diff --git a/lib/ns/query.c b/lib/ns/query.c index d6e9a4dffd..ec54bfbcc1 100644 --- a/lib/ns/query.c +++ b/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(¶m->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(¶m->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); From 4b67376e420c4fd239dd983c77b231729e241ce2 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 21 May 2018 09:51:20 -0700 Subject: [PATCH 3/3] update CHANGES --- CHANGES | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGES b/CHANGES index dd038c4fd8..61ca53953f 100644 --- a/CHANGES +++ b/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]