Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix blocked upstream (IP) handling #2176

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Fix blocked upstream (IP) handling #2176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions src/dnsmasq_interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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";
Expand All @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
}
}

Expand Down
26 changes: 16 additions & 10 deletions test/test_suite.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand All @@ -548,17 +549,17 @@
[[ ${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)" {
# Get number of lines in the log before the test
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)"
Expand All @@ -572,17 +573,21 @@
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)" {
# Get number of lines in the log before the test
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)"
Expand All @@ -599,16 +604,18 @@
[[ ${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)" {
# Get number of lines in the log before the test
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)"
Expand All @@ -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" {
Expand Down
Loading