From d4a6f3a93e140fa816c38e2f9335dee04e011233 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Fri, 2 Feb 2024 21:36:56 +0000 Subject: [PATCH] Fix logic error in signed RR handling. In extract_addresses() the "secure" argument is only set if the whole reply is validated (ie the AD bit can be set). Even without that, some records may be validated, and should be marked as such in the cache. Related, the DNS doctor code has to update the flags for individual RRs as it works, not the global "secure" flag. --- src/dnssec.c | 2 +- src/forward.c | 48 +++++++++++++++++++++++++++++------------------- src/rfc1035.c | 30 ++++++++++++++---------------- 3 files changed, 44 insertions(+), 36 deletions(-) diff --git a/src/dnssec.c b/src/dnssec.c index 8a4f4fe..29a8e7a 100644 --- a/src/dnssec.c +++ b/src/dnssec.c @@ -1804,7 +1804,7 @@ static int zone_status(char *name, int class, char *keyname, time_t now) When validating replies to DS records, we're only interested in the NSEC{3} RRs in the auth section. Other RRs in that section missing sigs will not cause am INSECURE reply. We determine this mode - is the nons argument is non-NULL. + if the nons argument is non-NULL. */ int dnssec_validate_reply(time_t now, struct dns_header *header, size_t plen, char *name, char *keyname, int *class, int check_unsigned, int *neganswer, int *nons, int *nsec_ttl) diff --git a/src/forward.c b/src/forward.c index 60f3657..45542d7 100644 --- a/src/forward.c +++ b/src/forward.c @@ -814,28 +814,38 @@ static size_t process_reply(struct dns_header *header, time_t now, struct server } } - if (do_doctor(header, n)) - cache_secure = 0; - - switch (extract_addresses(header, n, daemon->namebuff, now, ipsets, nftsets, is_sign, check_rebind, no_cache, cache_secure)) + if (!bogusanswer) { - case 1: - my_syslog(LOG_WARNING, _("possible DNS-rebind attack detected: %s"), daemon->namebuff); - munged = 1; - cache_secure = 0; - ede = EDE_BLOCKED; - break; + if (daemon->doctors && !do_doctor(header, n)) + { + /* do_doctors found malformed answer. */ + munged = 1; + SET_RCODE(header, SERVFAIL); + cache_secure = 0; + ede = EDE_OTHER; + } - /* extract_addresses() found a malformed answer. */ - case 2: - munged = 1; - SET_RCODE(header, SERVFAIL); - cache_secure = 0; - ede = EDE_OTHER; - break; + if (RCODE(header) != SERVFAIL) + switch (extract_addresses(header, n, daemon->namebuff, now, ipsets, nftsets, is_sign, check_rebind, no_cache, cache_secure)) + { + case 1: + my_syslog(LOG_WARNING, _("possible DNS-rebind attack detected: %s"), daemon->namebuff); + munged = 1; + cache_secure = 0; + ede = EDE_BLOCKED; + break; + + /* extract_addresses() found a malformed answer. */ + case 2: + munged = 1; + SET_RCODE(header, SERVFAIL); + cache_secure = 0; + ede = EDE_OTHER; + break; + } } - - if (rcode == NOERROR && rrfilter(header, &n, RRFILTER_CONF) > 0) + + if (RCODE(header) == NOERROR && rrfilter(header, &n, RRFILTER_CONF) > 0) ede = EDE_FILTERED; } diff --git a/src/rfc1035.c b/src/rfc1035.c index e34b28f..0d0cdb8 100644 --- a/src/rfc1035.c +++ b/src/rfc1035.c @@ -388,14 +388,10 @@ int do_doctor(struct dns_header *header, size_t qlen) { unsigned char *p; int i, qtype, qclass, rdlen; - int doctored = 0; - - if (!daemon->doctors) - return 0; - + if (!(p = skip_questions(header, qlen))) return 0; - + for (i = 0; i < ntohs(header->ancount) + ntohs(header->arcount); i++) { /* Skip over auth section */ @@ -436,7 +432,11 @@ int do_doctor(struct dns_header *header, size_t qlen) addr.addr4.s_addr |= (doctor->out.s_addr & doctor->mask.s_addr); /* Since we munged the data, the server it came from is no longer authoritative */ header->hb3 &= ~HB3_AA; - doctored = 1; +#ifdef HAVE_DNSSEC + /* remove validated flag from this RR, since we changed it! */ + if (option_bool(OPT_DNSSEC_VALID) && i < ntohs(header->ancount)) + daemon->rr_status[i] = 0; +#endif memcpy(p, &addr.addr4, INADDRSZ); log_query(F_FORWARD | F_CONFIG | F_IPV4, daemon->workspacename, &addr, NULL, 0); break; @@ -447,14 +447,14 @@ int do_doctor(struct dns_header *header, size_t qlen) return 0; /* bad packet */ } - return doctored; + return 1; } /* Find SOA RR in auth section to get TTL for negative caching of name. Cache said SOA and return the difference in length between name and the name of the SOA RR so we can look it up again. */ -static int find_soa(struct dns_header *header, size_t qlen, char *name, int *substring, int no_cache, int secure, time_t now) +static int find_soa(struct dns_header *header, size_t qlen, char *name, int *substring, int no_cache, time_t now) { unsigned char *p, *psave; int qtype, qclass, rdlen; @@ -463,8 +463,6 @@ static int find_soa(struct dns_header *header, size_t qlen, char *name, int *sub size_t name_len, soa_len, len; union all_addr addr; - (void)secure; /* warning */ - /* first move to NS section and find TTL from SOA RR */ if (!(p = skip_questions(header, qlen)) || !(p = skip_section(p, ntohs(header->ancount), header, qlen))) @@ -547,7 +545,7 @@ static int find_soa(struct dns_header *header, size_t qlen, char *name, int *sub int secflag = 0; #ifdef HAVE_DNSSEC - if (option_bool(OPT_DNSSEC_VALID) && secure && daemon->rr_status[i + ntohs(header->ancount)] != 0) + if (option_bool(OPT_DNSSEC_VALID) && daemon->rr_status[i + ntohs(header->ancount)] != 0) { secflag = F_DNSSECOK; @@ -700,7 +698,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) && secure && daemon->rr_status[j] != 0) + if (option_bool(OPT_DNSSEC_VALID) && daemon->rr_status[j] != 0) { /* validated RR anywhere in CNAME chain, don't cache. */ if (cname_short || aqtype == T_CNAME) @@ -750,7 +748,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t if (!found && !option_bool(OPT_NO_NEG)) { /* don't cache SOAs for negative PTR records */ - ttl = find_soa(header, qlen, name, NULL, 1, 0, now); + ttl = find_soa(header, qlen, name, NULL, 1, now); flags |= F_NEG | (secure ? F_DNSSECOK : 0); if (name_encoding && ttl) @@ -815,7 +813,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t } #ifdef HAVE_DNSSEC - if (option_bool(OPT_DNSSEC_VALID) && secure && daemon->rr_status[j] != 0) + if (option_bool(OPT_DNSSEC_VALID) && daemon->rr_status[j] != 0) { secflag = F_DNSSECOK; @@ -1054,7 +1052,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t /* If there's no SOA to get the TTL from, but there is a CNAME pointing at this, inherit its TTL */ - if (insert && !option_bool(OPT_NO_NEG) && ((ttl = find_soa(header, qlen, name, &substring, no_cache_dnssec, secure, now)) || cpp)) + if (insert && !option_bool(OPT_NO_NEG) && ((ttl = find_soa(header, qlen, name, &substring, no_cache_dnssec, now)) || cpp)) { addr.rrdata.datalen = substring; addr.rrdata.rrtype = qtype;