From 6b695c4b33ba1d9f9d322fe449fffa9d66289a60 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Mon, 13 Jan 2025 20:22:42 +0000 Subject: [PATCH 1/8] Tweak handling of duplicate DNS answers via UDP. If we get a duplicate answer for a query via UDP which we have either already received and started DNSSEC validation, or was truncated and we've passed to TCP, then just ignore it. The code was already in place, but had evolved wonky and only worked for error replies which would otherwise prompt a retransmit. Signed-off-by: DL6ER --- src/dnsmasq/forward.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/dnsmasq/forward.c b/src/dnsmasq/forward.c index 0819ea26e..1000f3676 100644 --- a/src/dnsmasq/forward.c +++ b/src/dnsmasq/forward.c @@ -884,10 +884,6 @@ static void dnssec_validate(struct frec *forward, struct dns_header *header, daemon->log_display_id = forward->frec_src.log_id; - /* We've had a reply already, which we're validating. Ignore this duplicate */ - if (forward->blocking_query || (forward->flags & FREC_GONE_TO_TCP)) - return; - /* Find the original query that started it all.... */ for (orig = forward; orig->dependent; orig = orig->dependent); @@ -1216,18 +1212,20 @@ void reply_query(int fd, time_t now) if (daemon->ignore_addr && RCODE(header) == NOERROR && check_for_ignored_address(header, n)) return; - - if ((RCODE(header) == REFUSED || RCODE(header) == SERVFAIL) && forward->forwardall == 0) - /* for broken servers, attempt to send to another one. */ - { + #ifdef HAVE_DNSSEC /* The query MAY have got a good answer, and be awaiting the results of further queries, in which case - The Stash contains something else and we don't need to retry anyway. */ + the stash contains something else and we don't need to retry anyway. + We may also have already got a truncated reply, and be in the process + of doing the query by TCP so can ignore further, probably truncated, UDP answers. */ if (forward->blocking_query || (forward->flags & FREC_GONE_TO_TCP)) return; #endif + if ((RCODE(header) == REFUSED || RCODE(header) == SERVFAIL) && forward->forwardall == 0) + /* for broken servers, attempt to send to another one. */ + { /* Get the saved query back. */ blockdata_retrieve(forward->stash, forward->stash_len, (void *)header); From 993b30144c9edb1d968a94b3e27cf6c6cd265ab7 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Fri, 17 Jan 2025 16:48:08 +0000 Subject: [PATCH 2/8] Extend build fingerprinting to include CFLAGS. If the value of CFLAGS is changed between builds, the makefile will rebuid, in the same way as for COPTS. Signed-off-by: DL6ER --- src/dnsmasq/config.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/dnsmasq/config.h b/src/dnsmasq/config.h index 82ded47ee..6cd4bedf5 100644 --- a/src/dnsmasq/config.h +++ b/src/dnsmasq/config.h @@ -375,6 +375,11 @@ HAVE_SOCKADDR_SA_LEN #define HAVE_INOTIFY #endif +/* This never compiles code, it's only used by the makefile to fingerprint builds. */ +#ifdef DNSMASQ_COMPILE_FLAGS +static char *compile_flags = DNSMASQ_COMPILE_FLAGS; +#endif + /* Define a string indicating which options are in use. DNSMASQ_COMPILE_OPTS is only defined in dnsmasq.c */ From dc4e9dc99d2910b3c0c2baf6b6fbf9116dddbe9e Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Fri, 17 Jan 2025 17:49:29 +0000 Subject: [PATCH 3/8] Handle DS queries to auth zones. When dnsmasq is configured to act as an authoritative server and has an authoritative zone configured, and recieves a query for that zone _as_forwarder_ it answers the query directly rather than forwarding it. This doesn't affect the answer, but it saves dnsmasq forwarding the query to the recusor upstream, whch then bounces it back to dnsmasq in auth mode. The exception should be when the query is for the root of zone, for a DS RR. The answer to that has to come from the parent, via the recursor, and will typically be a proof-of-nonexistence since dnsmasq doesn't support signed zones. This patch suppresses local answers and forces forwarding to the upstream recursor for such queries. It stops breakage when a DNSSEC validating client makes queries to dnsmasq acting as forwarder for a zone for which it is authoritative. Signed-off-by: DL6ER --- src/dnsmasq/forward.c | 52 +++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/src/dnsmasq/forward.c b/src/dnsmasq/forward.c index 1000f3676..a0d4842d1 100644 --- a/src/dnsmasq/forward.c +++ b/src/dnsmasq/forward.c @@ -1774,15 +1774,27 @@ void receive_query(struct listener *listen, time_t now) &source_addr, auth_dns ? "auth" : "query", type, daemon->log_display_id, UDP); #ifdef HAVE_AUTH - /* find queries for zones we're authoritative for, and answer them directly */ + /* Find queries for zones we're authoritative for, and answer them directly. + The exception to this is DS queries for the zone route. They + have to come from the parent zone. Since dnsmasq's auth server + can't do DNSSEC, the zone will be unsigned, and anything using + dnsmasq as a forwarder and doing validation will be expecting to + see the proof of non-existence from the parent. */ if (!auth_dns && !option_bool(OPT_LOCALISE)) for (zone = daemon->auth_zones; zone; zone = zone->next) - if (in_zone(zone, daemon->namebuff, NULL)) - { - auth_dns = 1; - local_auth = 1; - break; - } + { + char *cut; + + if (in_zone(zone, daemon->namebuff, &cut)) + { + if (type != T_DS || cut) + { + auth_dns = 1; + local_auth = 1; + } + break; + } + } #endif #ifdef HAVE_LOOP @@ -2462,15 +2474,27 @@ unsigned char *tcp_request(int confd, time_t now, &peer_addr, auth_dns ? "auth" : "query", qtype, daemon->log_display_id, TCP); #ifdef HAVE_AUTH - /* find queries for zones we're authoritative for, and answer them directly */ + /* Find queries for zones we're authoritative for, and answer them directly. + The exception to this is DS queries for the zone route. They + have to come from the parent zone. Since dnsmasq's auth server + can't do DNSSEC, the zone will be unsigned, and anything using + dnsmasq as a forwarder and doing validation will be expecting to + see the proof of non-existence from the parent. */ if (!auth_dns && !option_bool(OPT_LOCALISE)) for (zone = daemon->auth_zones; zone; zone = zone->next) - if (in_zone(zone, daemon->namebuff, NULL)) - { - auth_dns = 1; - local_auth = 1; - break; - } + { + char *cut; + + if (in_zone(zone, daemon->namebuff, &cut)) + { + if (qtype != T_DS || cut) + { + auth_dns = 1; + local_auth = 1; + } + break; + } + } #endif norebind = domain_no_rebind(daemon->namebuff); From 6f774d2a4e665a9be753ef481540ddd0511285ee Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Sat, 18 Jan 2025 22:16:29 +0000 Subject: [PATCH 4/8] Fix fubar that could return unsigned NODATA response when do bit set. Signed-off-by: DL6ER --- src/dnsmasq/rfc1035.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/dnsmasq/rfc1035.c b/src/dnsmasq/rfc1035.c index ee95ecf36..14c3dc4c8 100644 --- a/src/dnsmasq/rfc1035.c +++ b/src/dnsmasq/rfc1035.c @@ -1594,6 +1594,8 @@ static unsigned long crec_ttl(struct crec *crecp, time_t now) static int cache_validated(const struct crec *crecp) { + /* return 0; */ + return (option_bool(OPT_DNSSEC_VALID) && !(crecp->flags & F_DNSSECOK)); } @@ -2280,14 +2282,15 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, } - if (qtype != T_ANY && !ans && rr_on_list(daemon->filter_rr, qtype)) + if (qtype != T_ANY && !ans && rr_on_list(daemon->filter_rr, qtype) && !do_bit) { /* We don't have a cached answer and when we get an answer from upstream we're going to filter it anyway. If we have a cached answer for the domain for another RRtype then that may be enough to tell us if the answer should be NODATA and save the round trip. Cached NXDOMAIN has already been handled, so here we look for any record for the domain, since its existence allows us to return a NODATA answer. Note that we never set the AD flag, - since we didn't authenticate the record. */ + since we didn't authenticate the record; this doesn't work if we want auth data, so + don't use this shortcut in that case. */ if (cache_find_by_name(NULL, name, now, F_IPV4 | F_IPV6 | F_RR | F_CNAME)) { From b55f56ffba43223428bae6254d7dd7e5b5c62636 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Sat, 18 Jan 2025 22:40:30 +0000 Subject: [PATCH 5/8] Fix subtle bug in arbitrary-RR caching. If the client asks for DNSSEC RRs via the do bit, and we have an answer cached, we can only return the cached answer if the RR was not validated. This is because we don't the extra info (RRSIGS, NSECs) for a complete validated answer. In that case we have to forward again. This bug was that the "is the cache entry validated" test was in an outer loop rather than an inner one. A cache hit on a different RRtype that wasn't validated would satify the condition to use the cache, even if the cache entry for the required RRtype didn't. The only time when there can be a mix of validated and non validated cache entries for the same domain is when most are not validated, but one is a negative cache for a DS record. This bug took a long time to find. Signed-off-by: DL6ER --- src/dnsmasq/rfc1035.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/dnsmasq/rfc1035.c b/src/dnsmasq/rfc1035.c index 14c3dc4c8..32b28e5c0 100644 --- a/src/dnsmasq/rfc1035.c +++ b/src/dnsmasq/rfc1035.c @@ -1594,8 +1594,6 @@ static unsigned long crec_ttl(struct crec *crecp, time_t now) static int cache_validated(const struct crec *crecp) { - /* return 0; */ - return (option_bool(OPT_DNSSEC_VALID) && !(crecp->flags & F_DNSSECOK)); } @@ -2207,19 +2205,19 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, if (!ans) { - if ((crecp = cache_find_by_name(NULL, name, now, F_RR | F_NXDOMAIN)) && - rd_bit && (!do_bit || cache_validated(crecp))) + if ((crecp = cache_find_by_name(NULL, name, now, F_RR | F_NXDOMAIN)) && rd_bit) do { int flags = crecp->flags; unsigned short rrtype; - + if (flags & F_KEYTAG) rrtype = crecp->addr.rrblock.rrtype; else rrtype = crecp->addr.rrdata.rrtype; - if ((flags & F_NXDOMAIN) || rrtype == qtype) + if (((flags & F_NXDOMAIN) || rrtype == qtype) && + (!do_bit || cache_validated(crecp))) { char *rrdata = NULL; unsigned short rrlen = 0; From 0acbfe8ecaf993a1e2e970d0917e99dcecfe6e98 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Sat, 18 Jan 2025 23:26:06 +0000 Subject: [PATCH 6/8] Rename cache_validated() to cache_not_validated(). Let's give the poor programmers a chance. Signed-off-by: DL6ER --- src/dnsmasq/rfc1035.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/dnsmasq/rfc1035.c b/src/dnsmasq/rfc1035.c index 32b28e5c0..59d644fc3 100644 --- a/src/dnsmasq/rfc1035.c +++ b/src/dnsmasq/rfc1035.c @@ -1592,7 +1592,7 @@ static unsigned long crec_ttl(struct crec *crecp, time_t now) return daemon->max_ttl; } -static int cache_validated(const struct crec *crecp) +static int cache_not_validated(const struct crec *crecp) { return (option_bool(OPT_DNSSEC_VALID) && !(crecp->flags & F_DNSSECOK)); } @@ -1693,7 +1693,7 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, /* If the client asked for DNSSEC don't use cached data. */ if ((crecp->flags & (F_HOSTS | F_DHCP | F_CONFIG)) || - (rd_bit && (!do_bit || cache_validated(crecp)))) + (rd_bit && (!do_bit || cache_not_validated(crecp)))) { if (crecp->flags & F_CONFIG || qtype == T_CNAME) ans = 1; @@ -1852,7 +1852,7 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, the zone is unsigned, which implies that we're doing validation. */ if ((crecp->flags & (F_HOSTS | F_DHCP | F_CONFIG)) || - (rd_bit && (!do_bit || cache_validated(crecp)) )) + (rd_bit && (!do_bit || cache_not_validated(crecp)) )) { do { @@ -2008,7 +2008,7 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, /* If the client asked for DNSSEC don't use cached data. */ if ((crecp->flags & (F_HOSTS | F_DHCP | F_CONFIG)) || - (rd_bit && (!do_bit || cache_validated(crecp)) )) + (rd_bit && (!do_bit || cache_not_validated(crecp)) )) do { int stale_flag = 0; @@ -2217,7 +2217,7 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, rrtype = crecp->addr.rrdata.rrtype; if (((flags & F_NXDOMAIN) || rrtype == qtype) && - (!do_bit || cache_validated(crecp))) + (!do_bit || cache_not_validated(crecp))) { char *rrdata = NULL; unsigned short rrlen = 0; From 1ae7b655b79342f38427ac25cb415c4ccdf86ab8 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Sat, 18 Jan 2025 23:56:23 +0000 Subject: [PATCH 7/8] Fix log message fields in wrong order in some auth replies. Signed-off-by: DL6ER --- src/dnsmasq/auth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dnsmasq/auth.c b/src/dnsmasq/auth.c index d4556aba8..efe838555 100644 --- a/src/dnsmasq/auth.c +++ b/src/dnsmasq/auth.c @@ -513,7 +513,7 @@ size_t answer_auth(struct dns_header *header, char *limit, size_t qlen, time_t n nxdomain = 0; if ((crecp->flags & flag) && (local_query || filter_zone(zone, flag, &(crecp->addr)))) { - log_query(crecp->flags, name, &crecp->addr, record_source(crecp->uid), 0); + log_query(crecp->flags & ~F_REVERSE, name, &crecp->addr, record_source(crecp->uid), 0); if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, daemon->auth_ttl, NULL, qtype, C_IN, qtype == T_A ? "4" : "6", &crecp->addr)) From 62010ec9c4a721db2a56a6f5fdc07ef4d06de984 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 19 Jan 2025 07:00:12 +0100 Subject: [PATCH 8/8] Update dnsmasq version to most recent tag v2.91test8 Signed-off-by: DL6ER --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6c55f2b30..ee816d6c4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -16,6 +16,6 @@ set(CMAKE_C_STANDARD 17) project(PIHOLE_FTL C) -set(DNSMASQ_VERSION pi-hole-v2.91test7) +set(DNSMASQ_VERSION pi-hole-v2.91test8) add_subdirectory(src)