Validate internal PTR DNS responses

[brealorg]

What does this implement/fix?

The internal PTR resolver reuses one UDP socket across sequential lookups. If a lookup times out, a delayed response from that lookup can remain queued and be consumed while the resolver is waiting for the next lookup.

This change correlates received UDP DNS datagrams with the active PTR request before parsing them. It:

• validates the response source address and port;
• validates QR, opcode, transaction ID and QDCOUNT;
• decodes and compares the echoed QNAME, QTYPE and QCLASS;
• discards unrelated datagrams while keeping one absolute CLOCK_MONOTONIC deadline;
• uses the actual recvfrom() byte count as the UDP parser boundary;
• validates into a local DNS header and only copies it out after the response is accepted;
• preserves the secure query ID introduced by Fix non-reentrant libc calls reachable from FTL's threads #3030 and derives request_id from the final dns.id;
• reads the TCP response body until the announced DNS message length has actually arrived and bounds parsing by the received length;
• adds a deterministic regression harness under test/ for the stale-response sequence.

The independent return NULL; → return false; cleanup identified during review was split into #3027 and has already landed separately. The three remaining cases in this bool resolver function were corrected here during the rebase as requested.

How to test the change during review

The regression harness deliberately delays response A past the resolver timeout, waits for request B on the same UDP socket, then sends the stale A response before the correct B response.

On the pre-rebase baseline used to demonstrate the regression (a17efdd3307a233172d2f3b0d1e2ead5f9c85302):

FIRST_LOOKUP_TIMEOUT=PASS
SECOND_LOOKUP_RESULT=late-a.example
SERVER_SEQUENCE=PASS
PTR_RESPONSE_REGRESSION=FAIL

With this change applied:

FIRST_LOOKUP_TIMEOUT=PASS
SECOND_LOOKUP_RESULT=answer-b.example
SERVER_SEQUENCE=PASS
PTR_RESPONSE_REGRESSION=PASS

The harness is integrated into the normal test suite as PTR stale-response regression harness.

The exact rebased change, based on development at 1945de4f22e0af14ad2b323c3437bda4cb3210f3, was also built and tested in the current ghcr.io/pi-hole/ftl-build:v2.25 environment:

193 BATS tests, 0 failures
151 pytest API tests passed
12 dotdoh client tests, 0 failures
25 dotdoh server tests, 0 failures
9 final log-validation tests, 0 failures
TLS terminator test: PASS

The earlier full-daemon reproduction documented on #2978 remains independent corroboration of the same stale-response mechanism.

Additional information

Related issue or feature (if applicable): Refs #2978

Pull request in docs with documentation (if applicable): N/A

---

By submitting this pull request, I confirm the following:

1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
2. I have commented my proposed changes within the code.
3. I am willing to help maintain this change if there are issues with it later.
4. It is compatible with the EUPL 1.2 license
5. I have squashed any insignificant commits.
6. My change does not modify src/dnsmasq/.

Checklist:

• The code change is tested and works locally.
• I based my code and PRs against the repository's development branch.
• I signed off all commits (DCO).
• I cryptographically signed all commits.
• I have read the above and my PR is ready for review.

[brealorg]

A new production recurrence has been captured on the unpatched FTL v6.7 installation.

The full timestamped evidence and interpretation are recorded on issue #2978:

#2978 (comment)

Key observation: two IPv6 PTR timeouts were immediately followed by the IPv4 PTR lookup for 192.168.1.78 being reported with residual ip6.arpa question/answer data, while embedded dnsmasq simultaneously returned the correct Stormtab.localdomain PTR.

There is no packet capture from that exact production event, so it is documented as strong independent corroboration rather than standalone wire-level proof. No change to the draft patch is requested solely from this recurrence.

[DL6ER]

Confirmed the mechanism on #2978, so this is the right layer to fix it.

Using recvfrom()'s length as the parser boundary is a second fix in its own right: the existing bufend = buf + sizeof(buf) let the answer parser walk past the datagram into stale buffer content. The TCP path carries the same over-read and prefix is known there, so please use buf + prefix instead of leaving that path out of scope.

Smaller things:

• Timeouts log through strsockerr(EAGAIN), an errno string for an errno nothing returned. "Timed out after 2000 ms" is clearer.
• validate_udp_ptr_response() fills the caller's dns before any check passes, so a rejected packet clobbers the header copy. Harmless while the loop continues, but validating into a local and copying out on success is cleaner.
• A peer sending unrelated datagrams keeps the loop spinning until the deadline. Fine on loopback, worth a comment saying so.
• source_len < sizeof(source) compares socklen_t against size_t.
• The four return NULL to return false corrections are independent of the feature. Split them into their own PR and I will merge that one right away.

Drop the PoC wording from the comments before this goes ready - the bug story belongs in the PR and the commit message, not in the code.

You wrote on #2978 that the deterministic harness can be supplied separately. I would rather have it in test/ as a regression test than kept aside, if you are willing to adapt it.

Worth deciding separately: connect()ing the socket would let the kernel drop foreign sources for free, but not a stale reply from the right source, so the validation is needed either way.

[brealorg]

Split the four independent bool-return cleanups into #3027 as requested.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[DL6ER]

The conflict in src/resolve.c is my doing - #3030 landed this morning and changed the query ID in the same function. When you rebase, dns.id is no longer htons(random()), so request_id should pick up whatever the ID ends up being rather than reintroducing the old call. The green checks here ran against the base from before that merge, so they do not say anything about the rebased state yet.

One thing left in the TCP path. prefix is the length the peer announces, but the recv() below reads into sizeof(buf) and its return value is discarded, so nothing checks that prefix bytes actually arrived. On a stream socket a short read is normal, and bufend = buf + prefix then covers bytes that were never received - the same over-read you fixed for UDP by using the real recvfrom() length. Reading exactly prefix bytes in a loop and bounding by what came back would close it.

Three return NULL are also left in this bool function: the hname allocation, the UDP send failure and the truncated-response path. I said four last time, which was the count in the TCP block you moved to #3027 - these are the rest.

[github-actions]

Conflicts have been resolved.

[brealorg]

Rebased onto current development and addressed the remaining review points:

• preserved the secure query ID path from Fix non-reentrant libc calls reachable from FTL's threads #3030 and derive request_id from the final dns.id;
• read the TCP DNS body to the announced length before parsing;
• corrected the three remaining return NULL; cases in the bool resolver function;
• adapted the deterministic PTR regression harness to the secure query-ID path.

The exact rebased tree passed locally in the current FTL build image: 193 BATS tests, 151 pytest API tests, 12 DoT/DoH client tests, 25 DoT/DoH server tests, 9 final validation tests, and the TLS terminator test.

Fresh GitHub workflows are now awaiting maintainer approval.