From 5dc3b25d03d2d435bb34da7155cf565c49fabad7 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 19 Jan 2022 17:38:18 +1100 Subject: [PATCH 1/6] Add additional name checks when using a forwarder When using a forwarder, check that the owner name of response records are within the bailiwick of the forwarded name space. --- lib/dns/resolver.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 96e8a0cf99..4abed04905 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -370,6 +370,8 @@ struct fetchctx { dns_rdataset_t qminrrset; dns_fixedname_t qmindcfname; dns_name_t *qmindcname; + dns_fixedname_t fwdfname; + dns_name_t *fwdname; /*% * The number of events we're waiting for. @@ -3552,6 +3554,7 @@ fctx_getaddresses(fetchctx_t *fctx, bool badcache) { if (result == ISC_R_SUCCESS) { fwd = ISC_LIST_HEAD(forwarders->fwdrs); fctx->fwdpolicy = forwarders->fwdpolicy; + dns_name_copy(domain, fctx->fwdname); if (fctx->fwdpolicy == dns_fwdpolicy_only && isstrictsubdomain(domain, fctx->domain)) { @@ -4728,6 +4731,7 @@ fctx_create(dns_resolver_t *res, isc_task_t *task, const dns_name_t *name, fctx->domain = dns_fixedname_initname(&fctx->dfname); fctx->qminname = dns_fixedname_initname(&fctx->qminfname); fctx->qmindcname = dns_fixedname_initname(&fctx->qmindcfname); + fctx->fwdname = dns_fixedname_initname(&fctx->fwdfname); dns_name_copy(name, fctx->name); dns_name_copy(name, fctx->qminname); @@ -4772,6 +4776,7 @@ fctx_create(dns_resolver_t *res, isc_task_t *task, const dns_name_t *name, fname, &forwarders); if (result == ISC_R_SUCCESS) { fctx->fwdpolicy = forwarders->fwdpolicy; + dns_name_copy(fname, fctx->fwdname); } if (fctx->fwdpolicy != dns_fwdpolicy_only) { @@ -6757,6 +6762,15 @@ mark_related(dns_name_t *name, dns_rdataset_t *rdataset, bool external, } } +static inline bool +name_external(const dns_name_t *name, fetchctx_t *fctx) { + if (ISFORWARDER(fctx->addrinfo)) { + return (!dns_name_issubdomain(name, fctx->fwdname)); + } + + return (!dns_name_issubdomain(name, fctx->domain)); +} + static isc_result_t check_section(void *arg, const dns_name_t *addname, dns_rdatatype_t type, dns_rdataset_t *found, dns_section_t section) { @@ -6783,7 +6797,7 @@ check_section(void *arg, const dns_name_t *addname, dns_rdatatype_t type, result = dns_message_findname(rctx->query->rmessage, section, addname, dns_rdatatype_any, 0, &name, NULL); if (result == ISC_R_SUCCESS) { - external = !dns_name_issubdomain(name, fctx->domain); + external = name_external(name, fctx); if (type == dns_rdatatype_a) { for (rdataset = ISC_LIST_HEAD(name->list); rdataset != NULL; @@ -8450,6 +8464,13 @@ rctx_answer_scan(respctx_t *rctx) { break; case dns_namereln_subdomain: + /* + * Don't accept DNAME from parent namespace. + */ + if (name_external(name, fctx)) { + continue; + } + /* * In-scope DNAME records must have at least * as many labels as the domain being queried. @@ -8765,13 +8786,11 @@ rctx_authority_positive(respctx_t *rctx) { DNS_SECTION_AUTHORITY); while (!done && result == ISC_R_SUCCESS) { dns_name_t *name = NULL; - bool external; dns_message_currentname(rctx->query->rmessage, DNS_SECTION_AUTHORITY, &name); - external = !dns_name_issubdomain(name, fctx->domain); - if (!external) { + if (!name_external(name, fctx)) { dns_rdataset_t *rdataset = NULL; /* @@ -9158,8 +9177,10 @@ rctx_authority_dnssec(respctx_t *rctx) { } if (!dns_name_issubdomain(name, fctx->domain)) { - /* Invalid name found; preserve it for logging - * later */ + /* + * Invalid name found; preserve it for logging + * later. + */ rctx->found_name = name; rctx->found_type = ISC_LIST_HEAD(name->list)->type; continue; From 7e37b5e379368370d6e52a8db6a66dc6fa667d8e Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 21 Jan 2022 10:52:02 +1100 Subject: [PATCH 2/6] Check that the forward declaration is unchanged and not overridden If we are using a fowarder, in addition to checking that names to be cached are subdomains of the forwarded namespace, we must also check that there are no subsidiary forwarded namespaces which would take precedence. To be safe, we don't cache any responses if the forwarding configuration has changed since the query was sent. --- lib/dns/resolver.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 4abed04905..76b0c48b4f 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6765,7 +6765,31 @@ mark_related(dns_name_t *name, dns_rdataset_t *rdataset, bool external, static inline bool name_external(const dns_name_t *name, fetchctx_t *fctx) { if (ISFORWARDER(fctx->addrinfo)) { - return (!dns_name_issubdomain(name, fctx->fwdname)); + isc_result_t result; + dns_fixedname_t fixed; + dns_forwarders_t *forwarders = NULL; + dns_name_t *fname; + + if (!dns_name_issubdomain(name, fctx->fwdname)) { + return (true); + } + + /* + * Is there a child forwarder declaration that is better? + * This lookup should always succeed if the configuration + * has not changed. + */ + fname = dns_fixedname_initname(&fixed); + result = dns_fwdtable_find(fctx->res->view->fwdtable, name, fname, + &forwarders); + if (result == ISC_R_SUCCESS) { + return (!dns_name_equal(fname, fctx->fwdname)); + } + + /* + * Play it safe if the configuration has changed. + */ + return (true); } return (!dns_name_issubdomain(name, fctx->domain)); From c289913e5cd8558bb2ba8f01be2448c9ef7aaae4 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 21 Jan 2022 15:36:48 +1100 Subject: [PATCH 3/6] Check cached names for possible "forward only" clause When caching additional and glue data *not* from a forwarder, we must check that there is no "forward only" clause covering the owner name that would take precedence. Such names would normally be allowed by baliwick rules, but a "forward only" zone introduces a new baliwick scope. --- lib/dns/resolver.c | 79 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 21 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 76b0c48b4f..bee1fd103f 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6762,37 +6762,74 @@ mark_related(dns_name_t *name, dns_rdataset_t *rdataset, bool external, } } +/* + * Returns true if 'name' is external to the namespace for which + * the server being queried can answer, either because it's not a + * subdomain or because it's below a forward declaration. + */ static inline bool -name_external(const dns_name_t *name, fetchctx_t *fctx) { +name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { + isc_result_t result; + dns_forwarders_t *forwarders = NULL; + dns_fixedname_t fixed; + dns_name_t *fname = dns_fixedname_initname(&fixed); + dns_name_t suffix; + unsigned int labels; + + /* + * The name is outside the queried namespace. + */ + if (!dns_name_issubdomain(name, ISFORWARDER(fctx->addrinfo) + ? fctx->fwdname + : fctx->domain)) + { + return (true); + } + + /* + * If the record lives in the parent zone, adjust the name so we + * look for the correct forward clause. + */ + labels = dns_name_countlabels(name); + if (dns_rdatatype_atparent(type) && labels > 1U) { + dns_name_init(&suffix, NULL); + dns_name_getlabelsequence(name, 1, labels - 1, &suffix); + name = &suffix; + } + + /* + * Look for a forward declaration below 'name'. + */ + result = dns_fwdtable_find(fctx->res->view->fwdtable, name, fname, + &forwarders); + if (ISFORWARDER(fctx->addrinfo)) { - isc_result_t result; - dns_fixedname_t fixed; - dns_forwarders_t *forwarders = NULL; - dns_name_t *fname; - - if (!dns_name_issubdomain(name, fctx->fwdname)) { - return (true); - } - /* - * Is there a child forwarder declaration that is better? - * This lookup should always succeed if the configuration - * has not changed. + * See if the forwarder declaration is better. */ - fname = dns_fixedname_initname(&fixed); - result = dns_fwdtable_find(fctx->res->view->fwdtable, name, fname, - &forwarders); if (result == ISC_R_SUCCESS) { return (!dns_name_equal(fname, fctx->fwdname)); } /* - * Play it safe if the configuration has changed. + * If the lookup failed, the configuration must have + * changed: play it safe and don't cache. */ return (true); } - return (!dns_name_issubdomain(name, fctx->domain)); + /* + * If name is covered by 'forward only' then we can't + * cache this repsonse. + */ + if (result == ISC_R_SUCCESS && + forwarders->fwdpolicy == dns_fwdpolicy_only && + !ISC_LIST_EMPTY(forwarders->fwdrs)) + { + return (true); + } + + return (false); } static isc_result_t @@ -6821,7 +6858,7 @@ check_section(void *arg, const dns_name_t *addname, dns_rdatatype_t type, result = dns_message_findname(rctx->query->rmessage, section, addname, dns_rdatatype_any, 0, &name, NULL); if (result == ISC_R_SUCCESS) { - external = name_external(name, fctx); + external = name_external(name, type, fctx); if (type == dns_rdatatype_a) { for (rdataset = ISC_LIST_HEAD(name->list); rdataset != NULL; @@ -8491,7 +8528,7 @@ rctx_answer_scan(respctx_t *rctx) { /* * Don't accept DNAME from parent namespace. */ - if (name_external(name, fctx)) { + if (name_external(name, dns_rdatatype_dname, fctx)) { continue; } @@ -8814,7 +8851,7 @@ rctx_authority_positive(respctx_t *rctx) { dns_message_currentname(rctx->query->rmessage, DNS_SECTION_AUTHORITY, &name); - if (!name_external(name, fctx)) { + if (!name_external(name, dns_rdatatype_ns, fctx)) { dns_rdataset_t *rdataset = NULL; /* From fe1bbba2596572d4a04cd773e86ff111f13aa358 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 2 Feb 2022 15:13:39 +1100 Subject: [PATCH 4/6] Look for zones deeper than the current domain or forward name When caching glue, we need to ensure that there is no closer source of truth for the name. If the owner name for the glue record would be answered by a locally configured zone, do not cache. --- lib/dns/resolver.c | 64 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 16 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index bee1fd103f..b57c5308bb 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -63,6 +63,7 @@ #include #include #include +#include /* Detailed logging of fctx attach/detach */ #ifndef FCTX_TRACE @@ -6765,38 +6766,71 @@ mark_related(dns_name_t *name, dns_rdataset_t *rdataset, bool external, /* * Returns true if 'name' is external to the namespace for which * the server being queried can answer, either because it's not a - * subdomain or because it's below a forward declaration. + * subdomain or because it's below a forward declaration or a + * locally served zone. */ static inline bool name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { isc_result_t result; dns_forwarders_t *forwarders = NULL; - dns_fixedname_t fixed; + dns_fixedname_t fixed, zfixed; dns_name_t *fname = dns_fixedname_initname(&fixed); + dns_name_t *zfname = dns_fixedname_initname(&zfixed); + dns_name_t *apex = NULL; dns_name_t suffix; + dns_zone_t *zone = NULL; unsigned int labels; + dns_namereln_t rel; + + apex = ISFORWARDER(fctx->addrinfo) ? fctx->fwdname : fctx->domain; /* * The name is outside the queried namespace. */ - if (!dns_name_issubdomain(name, ISFORWARDER(fctx->addrinfo) - ? fctx->fwdname - : fctx->domain)) - { + rel = dns_name_fullcompare(name, apex, &(int){ 0 }, + &(unsigned int){ 0U }); + if (rel != dns_namereln_subdomain && rel != dns_namereln_equal) { return (true); } /* * If the record lives in the parent zone, adjust the name so we - * look for the correct forward clause. + * look for the correct zone or forward clause. */ labels = dns_name_countlabels(name); if (dns_rdatatype_atparent(type) && labels > 1U) { dns_name_init(&suffix, NULL); dns_name_getlabelsequence(name, 1, labels - 1, &suffix); name = &suffix; + } else if (rel == dns_namereln_equal) { + /* If 'name' is 'apex', no further checking is needed. */ + return (false); } + /* + * If there is a locally served zone between 'apex' and 'name' + * then don't cache. + */ + LOCK(&fctx->res->view->lock); + if (fctx->res->view->zonetable != NULL) { + unsigned int options = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_MIRROR; + result = dns_zt_find(fctx->res->view->zonetable, name, options, + zfname, &zone); + if (zone != NULL) { + dns_zone_detach(&zone); + } + if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { + if (dns_name_fullcompare(zfname, apex, &(int){ 0 }, + &(unsigned int){ 0U }) == + dns_namereln_subdomain) + { + UNLOCK(&fctx->res->view->lock); + return (true); + } + } + } + UNLOCK(&fctx->res->view->lock); + /* * Look for a forward declaration below 'name'. */ @@ -6816,16 +6850,14 @@ name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { * changed: play it safe and don't cache. */ return (true); - } - - /* - * If name is covered by 'forward only' then we can't - * cache this repsonse. - */ - if (result == ISC_R_SUCCESS && - forwarders->fwdpolicy == dns_fwdpolicy_only && - !ISC_LIST_EMPTY(forwarders->fwdrs)) + } else if (result == ISC_R_SUCCESS && + forwarders->fwdpolicy == dns_fwdpolicy_only && + !ISC_LIST_EMPTY(forwarders->fwdrs)) { + /* + * If 'name' is covered by a 'forward only' clause then we + * can't cache this repsonse. + */ return (true); } From 612f2778779565586dc1badca765c3a7740dc5d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Fri, 25 Feb 2022 15:14:10 +0100 Subject: [PATCH 5/6] Add CHANGES note for [GL #2950] --- CHANGES | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 0224b17784..d05a7c67d0 100644 --- a/CHANGES +++ b/CHANGES @@ -41,7 +41,10 @@ 5818. [placeholder] -5817. [placeholder] +5817. [security] The rules for acceptance of records into the cache + have been tightened to prevent the possibility of + poisoning if forwarders send records outside + the configured bailiwick. (CVE-2021-25220) [GL #2950] 5816. [bug] Make BIND compile with LibreSSL 3.5.0, as it was using not very accurate pre-processor checks for using shims. From 51546e88920829d9da74bff378eb88ac897f807e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Fri, 25 Feb 2022 15:14:23 +0100 Subject: [PATCH 6/6] Add Release Note for [GL #2950] --- doc/notes/notes-current.rst | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index a66b6eb57d..0b45bfb281 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -15,7 +15,14 @@ Notes for BIND 9.17.23 Security Fixes ~~~~~~~~~~~~~~ -- None. +- The rules for acceptance of records into the cache have been tightened + to prevent the possibility of poisoning if forwarders send records + outside the configured bailiwick. (CVE-2021-25220) + + ISC would like to thank Xiang Li, Baojun Liu, and Chaoyi Lu from + Network and Information Security Lab, Tsinghua University, and + Changgen Zou from Qi An Xin Group Corp. for bringing this + vulnerability to our attention. :gl:`#2950` Known Issues ~~~~~~~~~~~~