From be9b63e5845bf69609bbc4e23ae93fb951928fb8 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 2c5c50ab38..29828698b2 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -371,6 +371,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. @@ -3539,6 +3541,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)) { @@ -4708,6 +4711,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); @@ -4752,6 +4756,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) { @@ -6760,6 +6765,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) { @@ -6786,7 +6800,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; @@ -8453,6 +8467,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. @@ -8768,13 +8789,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; /* @@ -9161,8 +9180,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 ecf5580ec2ccd0cfa4a065fc61bfc211aa676f4b 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 29828698b2..efe4a438b5 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6768,7 +6768,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 41158c284d4728a25648f23697e8081f13e9e849 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 efe4a438b5..fed923025f 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6765,37 +6765,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 @@ -6824,7 +6861,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; @@ -8494,7 +8531,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; } @@ -8817,7 +8854,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 78f9691095a4680455181d8116669918947f50d7 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 fed923025f..bcdf902714 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 @@ -6768,38 +6769,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'. */ @@ -6819,16 +6853,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 2818968af2657e2d193958e65679517fd303ab42 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, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 47b854433c..03c863d5b1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +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. [GL #3172] From 9d13d3c6d6d118efa8a18bc195b744538998deb2 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 3cff9db120..4bac1c8706 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 ~~~~~~~~~~~~