From fa153f791f9324bf84abf8d259e11c0531fe6e25 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 10 Jul 2025 09:37:36 +1000 Subject: [PATCH 1/3] Tighten restrictions on caching NS RRsets in authority section To prevent certain spoofing attacks, a new check has been added to the existing rules for whether NS data can be cached: the owner name of the NS RRset must be an ancestor of the name being queried. --- lib/dns/resolver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 61ec6191f6..85b8aafadd 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -8435,8 +8435,8 @@ rctx_answer_dname(respctx_t *rctx) { * section to be subdomains of the domain being queried; any that are * not are skipped. We expect to find only *one* owner name; any names * after the first one processed are ignored. We expect to find only - * rdatasets of type NS, RRSIG, or SIG; all others are ignored. Whatever - * remains can be cached at trust level authauthority or additional + * rdatasets of type NS; all others are ignored. Whatever remains can + * be cached at trust level authauthority or additional * (depending on whether the AA bit was set on the answer). */ static void @@ -8445,7 +8445,9 @@ rctx_authority_positive(respctx_t *rctx) { dns_message_t *msg = rctx->query->rmessage; MSG_SECTION_FOREACH(msg, DNS_SECTION_AUTHORITY, name) { - if (!name_external(name, dns_rdatatype_ns, fctx)) { + if (!name_external(name, dns_rdatatype_ns, fctx) && + dns_name_issubdomain(fctx->name, name)) + { /* * We expect to find NS or SIG NS rdatasets, and * nothing else. From a41054e9e606a61f1b3c8bc0c54e2f1059347165 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 14 Aug 2025 14:35:46 +1000 Subject: [PATCH 2/3] Further restrict addresses that are cached when processing referrals Use the owner name of the NS record as the bailwick apex name when determining which additional records to cache, rather than the name of the delegating zone (or a parent thereof). --- lib/dns/resolver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 85b8aafadd..7d2823e129 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6359,7 +6359,8 @@ mark_related(dns_name_t *name, dns_rdataset_t *rdataset, bool external, * locally served zone. */ static inline bool -name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { +name_external(const dns_name_t *name, dns_rdatatype_t type, respctx_t *rctx) { + fetchctx_t *fctx = rctx->fctx; isc_result_t result; dns_forwarders_t *forwarders = NULL; dns_name_t *apex = NULL; @@ -6369,7 +6370,7 @@ name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { dns_namereln_t rel; apex = (ISDUALSTACK(fctx->addrinfo) || !ISFORWARDER(fctx->addrinfo)) - ? fctx->domain + ? rctx->ns_name != NULL ? rctx->ns_name : fctx->domain : fctx->fwdname; /* @@ -6466,7 +6467,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, type, fctx); + external = name_external(name, type, rctx); if (type == dns_rdatatype_a) { ISC_LIST_FOREACH(name->list, rdataset, link) { if (dns_rdatatype_issig(rdataset->type)) { @@ -8152,7 +8153,7 @@ rctx_answer_scan(respctx_t *rctx) { /* * Don't accept DNAME from parent namespace. */ - if (name_external(name, dns_rdatatype_dname, fctx)) { + if (name_external(name, dns_rdatatype_dname, rctx)) { continue; } @@ -8445,7 +8446,7 @@ rctx_authority_positive(respctx_t *rctx) { dns_message_t *msg = rctx->query->rmessage; MSG_SECTION_FOREACH(msg, DNS_SECTION_AUTHORITY, name) { - if (!name_external(name, dns_rdatatype_ns, fctx) && + if (!name_external(name, dns_rdatatype_ns, rctx) && dns_name_issubdomain(fctx->name, name)) { /* From 2e40705c06831988106335ed77db3cf924d431f6 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 13 Aug 2025 13:56:01 +1000 Subject: [PATCH 3/3] Retry lookups with unsigned DNAME over TCP To prevent spoofed unsigned DNAME responses being accepted retry response with unsigned DNAMEs over TCP if the response is not TSIG signed or there isn't a good DNS CLIENT COOKIE. --- doc/arm/reference.rst | 2 + lib/dns/include/dns/message.h | 8 +++ lib/dns/message.c | 12 +++++ lib/dns/resolver.c | 99 ++++++++++++++++++++++++++++------- 4 files changed, 103 insertions(+), 18 deletions(-) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 3a761cfd1b..7d78c93adb 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -2207,6 +2207,8 @@ Boolean Options autodetection of DNS COOKIE support to determine when to retry a request over TCP. + For DNAME lookups the default is ``yes`` and it is enforced. Servers + serving DNAME must correctly support DNS over TCP. .. note:: If a UDP response is signed using TSIG, :iscman:`named` accepts it even if diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index 557a745bbc..bfda3fbb56 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -261,6 +261,7 @@ struct dns_message { unsigned int rdclass_set : 1; /* 14 */ unsigned int fuzzing : 1; /* 15 */ unsigned int free_pools : 1; /* 16 */ + unsigned int has_dname : 1; /* 17 */ unsigned int : 0; unsigned int opt_reserved; @@ -1456,3 +1457,10 @@ dns_message_createpools(isc_mem_t *mctx, isc_mempool_t **namepoolp, isc_mempool_t **rdspoolp); void dns_message_destroypools(isc_mempool_t **namepoolp, isc_mempool_t **rdspoolp); + +bool +dns_message_hasdname(dns_message_t *msg); +/*%< + * Return whether a DNAME was detected in the ANSWER section of a QUERY + * message when it was parsed. + */ diff --git a/lib/dns/message.c b/lib/dns/message.c index e773a419d4..eadc99d015 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -442,6 +442,7 @@ msginit(dns_message_t *m) { m->cc_bad = 0; m->tkey = 0; m->rdclass_set = 0; + m->has_dname = 0; m->querytsig = NULL; m->indent.string = "\t"; m->indent.count = 0; @@ -1494,6 +1495,11 @@ getsection(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx, */ msg->tsigname->attributes.nocompress = true; free_name = false; + } else if (rdtype == dns_rdatatype_dname && + sectionid == DNS_SECTION_ANSWER && + msg->opcode == dns_opcode_query) + { + msg->has_dname = 1; } rdataset = NULL; @@ -5083,3 +5089,9 @@ dns_message_destroypools(isc_mempool_t **namepoolp, isc_mempool_t **rdspoolp) { isc_mempool_destroy(rdspoolp); isc_mempool_destroy(namepoolp); } + +bool +dns_message_hasdname(dns_message_t *msg) { + REQUIRE(DNS_MESSAGE_VALID(msg)); + return msg->has_dname; +} diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 7d2823e129..30537e418e 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -854,6 +854,7 @@ typedef struct respctx { bool get_nameservers; /* get a new NS rrset at * zone cut? */ bool resend; /* resend this query? */ + bool secured; /* message was signed or had a valid cookie */ bool nextitem; /* invalid response; keep * listening for the correct one */ bool truncated; /* response was truncated */ @@ -7278,13 +7279,10 @@ rctx_cookiecheck(respctx_t *rctx) { resquery_t *query = rctx->query; /* - * If TSIG signed, sent via TCP, or cookie present, - * no need to continue. + * If the message was secured or TCP is already in the + * retry flags, no need to continue. */ - if (dns_message_gettsig(query->rmessage, NULL) != NULL || - query->rmessage->cc_ok || query->rmessage->cc_bad || - (rctx->retryopts & DNS_FETCHOPT_TCP) != 0) - { + if (rctx->secured || (rctx->retryopts & DNS_FETCHOPT_TCP) != 0) { return ISC_R_SUCCESS; } @@ -7347,6 +7345,47 @@ rctx_cookiecheck(respctx_t *rctx) { return ISC_R_SUCCESS; } +static bool +rctx_need_tcpretry(respctx_t *rctx) { + resquery_t *query = rctx->query; + if ((rctx->retryopts & DNS_FETCHOPT_TCP) != 0) { + /* TCP is already in the retry flags */ + return false; + } + + /* + * If the message was secured, no need to continue. + */ + if (rctx->secured) { + return false; + } + + /* + * Currently the only extra reason why we might need to + * retry a UDP response over TCP is a DNAME in the message. + */ + if (dns_message_hasdname(query->rmessage)) { + return true; + } + + return false; +} + +static isc_result_t +rctx_tcpretry(respctx_t *rctx) { + /* + * Do we need to retry a UDP response over TCP? + */ + if (rctx_need_tcpretry(rctx)) { + rctx->retryopts |= DNS_FETCHOPT_TCP; + rctx->resend = true; + rctx_done(rctx, ISC_R_SUCCESS); + return ISC_R_COMPLETE; + } + + return ISC_R_SUCCESS; +} + static void resquery_response_continue(void *arg, isc_result_t result) { respctx_t *rctx = arg; @@ -7366,6 +7405,17 @@ resquery_response_continue(void *arg, isc_result_t result) { goto cleanup; } + /* + * Remember whether this message was signed or had a + * valid client cookie; if not, we may need to retry over + * TCP later. + */ + if (query->rmessage->cc_ok || query->rmessage->tsig != NULL || + query->rmessage->sig0 != NULL) + { + rctx->secured = true; + } + /* * The dispatcher should ensure we only get responses with QR * set. @@ -7373,13 +7423,21 @@ resquery_response_continue(void *arg, isc_result_t result) { INSIST((query->rmessage->flags & DNS_MESSAGEFLAG_QR) != 0); /* - * Check for cookie issues. + * Check for cookie issues; if found, maybe retry over TCP. */ result = rctx_cookiecheck(rctx); if (result == ISC_R_COMPLETE) { goto cleanup; } + /* + * Check whether we need to retry over TCP for some other reason. + */ + result = rctx_tcpretry(rctx); + if (result == ISC_R_COMPLETE) { + goto cleanup; + } + /* * Check for EDNS issues. */ @@ -8092,8 +8150,8 @@ rctx_answer_positive(respctx_t *rctx) { } /* - * Cache records in the authority section, if - * there are any suitable for caching. + * Cache records in the authority section, if there are + * any suitable for caching. */ rctx_authority_positive(rctx); @@ -8431,20 +8489,25 @@ rctx_answer_dname(respctx_t *rctx) { /* * rctx_authority_positive(): - * Examine the records in the authority section (if there are any) for a - * positive answer. We expect the names for all rdatasets in this - * section to be subdomains of the domain being queried; any that are - * not are skipped. We expect to find only *one* owner name; any names - * after the first one processed are ignored. We expect to find only - * rdatasets of type NS; all others are ignored. Whatever remains can - * be cached at trust level authauthority or additional - * (depending on whether the AA bit was set on the answer). + * If a positive answer was received over TCP or secured with a cookie + * or TSIG, examine the authority section. We expect names for all + * rdatasets in this section to be subdomains of the domain being queried; + * any that are not are skipped. We expect to find only *one* owner name; + * any names after the first one processed are ignored. We expect to find + * only rdatasets of type NS; all others are ignored. Whatever remains can + * be cached at trust level authauthority or additional (depending on + * whether the AA bit was set on the answer). */ static void rctx_authority_positive(respctx_t *rctx) { fetchctx_t *fctx = rctx->fctx; - dns_message_t *msg = rctx->query->rmessage; + + /* If it's spoofable, don't cache it. */ + if (!rctx->secured && (rctx->query->options & DNS_FETCHOPT_TCP) == 0) { + return; + } + MSG_SECTION_FOREACH(msg, DNS_SECTION_AUTHORITY, name) { if (!name_external(name, dns_rdatatype_ns, rctx) && dns_name_issubdomain(fctx->name, name))