From 3b74df4f55b1107b051183d6b628eb02933cb00a Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Tue, 10 Dec 2024 14:51:24 +0000 Subject: [PATCH] Fix erroneous "DNSSEC validated" state with non-DNSSEC upstream servers. When DNSEC validation is enabled, but a query is not validated because it gets forwarded to a non-DNSEC-capable upstream server, the rr_status array is not correctly cleared, with the effect that the answer may be maked as DNSSEC validated if the immediately preceding query was DNS signed and validated. --- CHANGELOG | 5 ++- src/forward.c | 91 +++++++++++++++++++++++++++++---------------------- src/rfc1035.c | 4 +-- 3 files changed, 57 insertions(+), 43 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 60f8cd7..8982a93 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -72,7 +72,10 @@ version 2.91 a DHCP relay. This change allows dnsmasq to act as both a PXE proxy-DHCP server AND a DHCP relay for the same network. - + Fix erroneous "DNSSEC validated" state with non-DNSSEC + upstream servers. Thanks to Dominik Derigs for the bug report. + + version 2.90 Fix reversion in --rev-server introduced in 2.88 which caused breakage if the prefix length is not exactly divisible diff --git a/src/forward.c b/src/forward.c index 5c704d1..20c10b0 100644 --- a/src/forward.c +++ b/src/forward.c @@ -906,7 +906,7 @@ static void dnssec_validate(struct frec *forward, struct dns_header *header, else status = dnssec_validate_reply(now, header, plen, daemon->namebuff, daemon->keyname, &forward->class, !option_bool(OPT_DNSSEC_IGN_NS) && (forward->sentto->flags & SERV_DO_DNSSEC), - NULL, NULL, NULL, &orig->validate_counter); + NULL, NULL, NULL, &orig->validate_counter); if (STAT_ISEQUAL(status, STAT_ABANDONED)) log_resource = 1; @@ -1220,10 +1220,15 @@ void reply_query(int fd, time_t now) #ifdef HAVE_DNSSEC - if ((forward->sentto->flags & SERV_DO_DNSSEC) && - option_bool(OPT_DNSSEC_VALID) && - !(forward->flags & FREC_CHECKING_DISABLED)) - dnssec_validate(forward, header, n, STAT_OK, now); + if (option_bool(OPT_DNSSEC_VALID)) + { + /* Clear this in case we don't call dnssec_validate() below */ + memset(daemon->rr_status, 0, sizeof(*daemon->rr_status) * daemon->rr_status_sz); + + if ((forward->sentto->flags & SERV_DO_DNSSEC) && + !(forward->flags & FREC_CHECKING_DISABLED)) + dnssec_validate(forward, header, n, STAT_OK, now); + } else #endif return_reply(now, forward, header, n, STAT_OK); @@ -2491,46 +2496,52 @@ unsigned char *tcp_request(int confd, time_t now, log_query_mysockaddr(F_SERVER | F_FORWARD, daemon->namebuff, &serv->addr, NULL, 0); #ifdef HAVE_DNSSEC - if (option_bool(OPT_DNSSEC_VALID) && !checking_disabled && (master->flags & SERV_DO_DNSSEC)) + if (option_bool(OPT_DNSSEC_VALID)) { - int keycount = daemon->limit[LIMIT_WORK]; /* Limit to number of DNSSEC questions, to catch loops and avoid filling cache. */ - int validatecount = daemon->limit[LIMIT_CRYPTO]; - int status = tcp_key_recurse(now, STAT_OK, header, m, 0, daemon->namebuff, daemon->keyname, - serv, have_mark, mark, &keycount, &validatecount); - char *result, *domain = "result"; + /* Clear this in case we don't call tcp_key_recurse() below */ + memset(daemon->rr_status, 0, sizeof(*daemon->rr_status) * daemon->rr_status_sz); - union all_addr a; - a.log.ede = ede = errflags_to_ede(status); - - if (STAT_ISEQUAL(status, STAT_ABANDONED)) + if (!checking_disabled && (master->flags & SERV_DO_DNSSEC)) { - result = "ABANDONED"; - status = STAT_BOGUS; - } - else - result = (STAT_ISEQUAL(status, STAT_SECURE) ? "SECURE" : (STAT_ISEQUAL(status, STAT_INSECURE) ? "INSECURE" : "BOGUS")); - - if (STAT_ISEQUAL(status, STAT_SECURE)) - cache_secure = 1; - else if (STAT_ISEQUAL(status, STAT_BOGUS)) - { - no_cache_dnssec = 1; - bogusanswer = 1; + int keycount = daemon->limit[LIMIT_WORK]; /* Limit to number of DNSSEC questions, to catch loops and avoid filling cache. */ + int validatecount = daemon->limit[LIMIT_CRYPTO]; + int status = tcp_key_recurse(now, STAT_OK, header, m, 0, daemon->namebuff, daemon->keyname, + serv, have_mark, mark, &keycount, &validatecount); + char *result, *domain = "result"; - if (extract_request(header, m, daemon->namebuff, NULL)) - domain = daemon->namebuff; + union all_addr a; + a.log.ede = ede = errflags_to_ede(status); + + if (STAT_ISEQUAL(status, STAT_ABANDONED)) + { + result = "ABANDONED"; + status = STAT_BOGUS; + } + else + result = (STAT_ISEQUAL(status, STAT_SECURE) ? "SECURE" : (STAT_ISEQUAL(status, STAT_INSECURE) ? "INSECURE" : "BOGUS")); + + if (STAT_ISEQUAL(status, STAT_SECURE)) + cache_secure = 1; + else if (STAT_ISEQUAL(status, STAT_BOGUS)) + { + no_cache_dnssec = 1; + bogusanswer = 1; + + if (extract_request(header, m, daemon->namebuff, NULL)) + domain = daemon->namebuff; + } + + log_query(F_SECSTAT, domain, &a, result, 0); + + if ((daemon->limit[LIMIT_CRYPTO] - validatecount) > (int)daemon->metrics[METRIC_CRYPTO_HWM]) + daemon->metrics[METRIC_CRYPTO_HWM] = daemon->limit[LIMIT_CRYPTO] - validatecount; + + if ((daemon->limit[LIMIT_WORK] - keycount) > (int)daemon->metrics[METRIC_WORK_HWM]) + daemon->metrics[METRIC_WORK_HWM] = daemon->limit[LIMIT_WORK] - keycount; + + /* include DNSSEC queries in the limit for a connection. */ + query_count += daemon->limit[LIMIT_WORK] - keycount; } - - log_query(F_SECSTAT, domain, &a, result, 0); - - if ((daemon->limit[LIMIT_CRYPTO] - validatecount) > (int)daemon->metrics[METRIC_CRYPTO_HWM]) - daemon->metrics[METRIC_CRYPTO_HWM] = daemon->limit[LIMIT_CRYPTO] - validatecount; - - if ((daemon->limit[LIMIT_WORK] - keycount) > (int)daemon->metrics[METRIC_WORK_HWM]) - daemon->metrics[METRIC_WORK_HWM] = daemon->limit[LIMIT_WORK] - keycount; - - /* include DNSSEC queries in the limit for a connection. */ - query_count += daemon->limit[LIMIT_WORK] - keycount; } #endif diff --git a/src/rfc1035.c b/src/rfc1035.c index ce7f5c1..8dbe652 100644 --- a/src/rfc1035.c +++ b/src/rfc1035.c @@ -706,7 +706,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t if (aqclass == C_IN && res != 2 && (aqtype == T_CNAME || aqtype == T_PTR)) { #ifdef HAVE_DNSSEC - if (option_bool(OPT_DNSSEC_VALID) && daemon->rr_status[j] != 0) + if (option_bool(OPT_DNSSEC_VALID) && j < daemon->rr_status_sz && daemon->rr_status[j] != 0) { /* validated RR anywhere in CNAME chain, don't cache. */ if (cname_short || aqtype == T_CNAME) @@ -825,7 +825,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t } #ifdef HAVE_DNSSEC - if (option_bool(OPT_DNSSEC_VALID) && daemon->rr_status[j] != 0) + if (option_bool(OPT_DNSSEC_VALID) && j < daemon->rr_status_sz && daemon->rr_status[j] != 0) { secflag = F_DNSSECOK;