From 21373539fb61eabc2284ae3e0a12d7cd9f0e45ba Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 2 Feb 2025 18:33:18 +0100 Subject: [PATCH] Queries blocked upstream with a known blocking page IP should not be short-circuited locally as this will cause FTL to reply with the locally defined blcoking mode instead of the IP address of the upstream DNS server's blocking page Signed-off-by: DL6ER --- src/dnsmasq_interface.c | 37 ++++++++++++++++++++++++++----------- test/test_suite.bats | 26 ++++++++++++++++---------- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index c25fd6b40..0a3b413ea 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -1557,13 +1557,13 @@ static bool FTL_check_blocking(const unsigned int queryID, const unsigned int do force_next_DNS_reply = dns_cache->force_reply; query_blocked(query, domain, client, QUERY_SPECIAL_DOMAIN); return true; - break; case QUERY_EXTERNAL_BLOCKED_IP: case QUERY_EXTERNAL_BLOCKED_NULL: case QUERY_EXTERNAL_BLOCKED_NXRA: case QUERY_EXTERNAL_BLOCKED_EDE15: - + { + bool shortcircuit = true; switch(blocking_status) { case QUERY_UNKNOWN: @@ -1586,6 +1586,15 @@ static bool FTL_check_blocking(const unsigned int queryID, const unsigned int do break; case QUERY_EXTERNAL_BLOCKED_IP: blockingreason = "blocked upstream with known address"; + // We do not want to short-circuit this + // query as to get the address contained + // in the upstream reply being sent + // downstream to the client. + // Otherwise, Pi-hole's short-circuiting + // would reply to the client with the + // configured blocking mode (probably + // NULL) + shortcircuit = false; break; case QUERY_EXTERNAL_BLOCKED_NULL: blockingreason = "blocked upstream with NULL address"; @@ -1605,8 +1614,8 @@ static bool FTL_check_blocking(const unsigned int queryID, const unsigned int do force_next_DNS_reply = dns_cache->force_reply; query_blocked(query, domain, client, blocking_status); - return true; - break; + return shortcircuit; + } case QUERY_CACHE: case QUERY_FORWARDED: @@ -1626,7 +1635,6 @@ static bool FTL_check_blocking(const unsigned int queryID, const unsigned int do query->flags.allowed = true; return false; - break; } // Skip all checks and continue if we hit already at least one allowlist in the chain @@ -2387,10 +2395,12 @@ static void FTL_reply(const unsigned int flags, const char *name, const union al const double mean = upstream->rtime / upstream->responses; upstream->rtuncertainty += (mean - query->response)*(mean - query->response); - // Only proceed if query is not already known - // to have been blocked upstream - if(query->status == QUERY_EXTERNAL_BLOCKED_IP || - query->status == QUERY_EXTERNAL_BLOCKED_NULL || + // Only proceed if query is not already known to have been + // blocked upstream AND short-circuited. + // Note: The reply needs to be analyzed further in case of + // QUERY_EXTERNAL_BLOCKED_IP as this is a "normal" upstream + // reply and we need to process it further (DNSSEC status, etc.) + if(query->status == QUERY_EXTERNAL_BLOCKED_NULL || query->status == QUERY_EXTERNAL_BLOCKED_NXRA || query->status == QUERY_EXTERNAL_BLOCKED_EDE15) { @@ -2989,8 +2999,13 @@ int _FTL_check_reply(const unsigned int rcode, const unsigned short flags, { FTL_blocked_upstream_by_addr(new_qstatus, id, file, line); - // Query is blocked - return 1; + // Query is blocked upstream + + // Return true for any status except known blocking page + // IP address to short-circut the answer. In the latter case, + // we want to continue processing the query to get the correct + // reply downstream to the requesting client. + return new_qstatus != QUERY_EXTERNAL_BLOCKED_IP; } } diff --git a/test/test_suite.bats b/test/test_suite.bats index 6dcc79981..413c1d15b 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -529,9 +529,10 @@ before="$(grep -c ^ /var/log/pihole/FTL.log)" # Run test - run bash -c "dig A umbrella.ftl @127.0.0.1" + run bash -c "dig A umbrella.ftl +short @127.0.0.1" printf "%s\n" "${lines[@]}" - [[ ${lines[@]} == *"EDE: 15 (Blocked): (upstream IP)"* ]] + [[ ${lines[0]} == "146.112.61.104" ]] + [[ ${lines[1]} == "" ]] # Get number of lines in the log after the test after="$(grep -c ^ /var/log/pihole/FTL.log)" @@ -548,7 +549,6 @@ [[ ${lines[@]} == *"DEBUG_QUERIES: **** forwarded umbrella.ftl to 127.0.0.1#5555"* ]] [[ ${lines[@]} == *"DEBUG_QUERIES: blocked upstream with known address (IPv4)"* ]] [[ ${lines[@]} == *"DEBUG_QUERIES: DNS cache: A/127.0.0.1/umbrella.ftl -> EXTERNAL_BLOCKED_IP"* ]] - [[ ${lines[@]} == *"DEBUG_QUERIES: Adding RR: \"umbrella.ftl A 0.0.0.0\""* ]] } @test "Upstream blocked domain: IP is recognized (cached)" { @@ -556,9 +556,10 @@ before="$(grep -c ^ /var/log/pihole/FTL.log)" # Run test - run bash -c "dig A umbrella.ftl @127.0.0.1" + run bash -c "dig A umbrella.ftl +short @127.0.0.1" printf "%s\n" "${lines[@]}" - [[ ${lines[@]} == *"EDE: 15 (Blocked): (upstream IP)"* ]] + [[ ${lines[0]} == "146.112.61.104" ]] + [[ ${lines[1]} == "" ]] # Get number of lines in the log after the test after="$(grep -c ^ /var/log/pihole/FTL.log)" @@ -572,8 +573,10 @@ done <<< "${log}" printf "%s\n" "${lines[@]}" [[ ${lines[@]} == *"DEBUG_QUERIES: umbrella.ftl is known as blocked upstream with known address (expires in"* ]] + # Test for NOT forwarded ... [[ ${lines[@]} != *"DEBUG_QUERIES: **** forwarded umbrella.ftl to 127.0.0.1#5555"* ]] - [[ ${lines[@]} == *"DEBUG_QUERIES: Adding RR: \"umbrella.ftl A 0.0.0.0\""* ]] + # ... but cached + [[ ${lines[@]} == *"DEBUG_QUERIES: **** got cache reply: umbrella.ftl is 146.112.61.104"* ]] } @test "Upstream blocked domain: IP is recognized (IPv6)" { @@ -581,8 +584,10 @@ before="$(grep -c ^ /var/log/pihole/FTL.log)" # Run test - run bash -c "dig AAAA umbrella.ftl @127.0.0.1" + run bash -c "dig AAAA umbrella.ftl +short @127.0.0.1" printf "%s\n" "${lines[@]}" + [[ ${lines[0]} == "::ffff:146.112.61.104" ]] + [[ ${lines[1]} == "" ]] # Get number of lines in the log after the test after="$(grep -c ^ /var/log/pihole/FTL.log)" @@ -599,7 +604,6 @@ [[ ${lines[@]} == *"DEBUG_QUERIES: **** forwarded umbrella.ftl to 127.0.0.1#5555"* ]] [[ ${lines[@]} == *"DEBUG_QUERIES: blocked upstream with known address (IPv6)"* ]] [[ ${lines[@]} == *"DEBUG_QUERIES: DNS cache: AAAA/127.0.0.1/umbrella.ftl -> EXTERNAL_BLOCKED_IP"* ]] - [[ ${lines[@]} == *"DEBUG_QUERIES: Adding RR: \"umbrella.ftl AAAA ::\""* ]] } @test "Upstream blocked domain: IP is recognized (multi)" { @@ -607,8 +611,11 @@ before="$(grep -c ^ /var/log/pihole/FTL.log)" # Run test - run bash -c "dig A umbrella-multi.ftl @127.0.0.1" + run bash -c "dig A umbrella-multi.ftl +short @127.0.0.1" printf "%s\n" "${lines[@]}" + [[ "${lines[@]}" == *"146.112.61.104"* ]] + [[ "${lines[@]}" == *"8.8.8.8"* ]] + [[ "${lines[@]}" == *"1.2.3.4"* ]] # Get number of lines in the log after the test after="$(grep -c ^ /var/log/pihole/FTL.log)" @@ -624,7 +631,6 @@ [[ ${lines[@]} == *"DEBUG_QUERIES: DNS cache: A/127.0.0.1/umbrella-multi.ftl is not blocked (domainlist ID: -1)"* ]] [[ ${lines[@]} == *"DEBUG_QUERIES: **** forwarded umbrella-multi.ftl to 127.0.0.1#5555"* ]] [[ ${lines[@]} == *"DEBUG_QUERIES: DNS cache: A/127.0.0.1/umbrella-multi.ftl -> EXTERNAL_BLOCKED_IP"* ]] - [[ ${lines[@]} == *"DEBUG_QUERIES: Adding RR: \"umbrella-multi.ftl A 0.0.0.0\""* ]] } @test "Upstream blocked domain: EDE 15 is recognized" {