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.

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

Update embedded dnsmasq to latest tag v2.91test2 #2136

Merged
merged 76 commits into from
Dec 22, 2024
Merged

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Dec 19, 2024

What does this implement/fix?

See title.

CHANGELOG:

  • Fix spurious "resource limit exceeded messages". Thanks to Dominik Derigs for the bug report.
  • Fix out-of-bounds heap read in order_qsort(). We only need to order two server records on the ->serial field. Literal address records are smaller and don't have this field and don't need to be ordered on it. To actually provoke this bug seems to need the same server-literal to be repeated twice, eg --address=/a/1.1.1.1 --address-/a/1.1.1.1 which is clearly rare in the wild, but if it did exist it could provoke a SIGSEV. Thanks to Daniel Rhea for fuzzing this one.
  • Fix buffer overflow when configured lease-change script name is too long. Thanks to Daniel Rhea for finding this one.
  • Improve behaviour in the face of non-responsive upstream TCP DNS servers. Without shorter timeouts, clients are blocked for too long and fail wuth their own timeouts.
  • Set --fast-dns-retries by default when doing DNSSEC. A single downstream query can trigger many upstream queries. On an unreliable network, there may not be enough downstream retries to ensure that all these queries complete.
  • Improve behaviour in the face of truncated answers to queries for DNSSEC records. Getting these answers by TCP doesn't now involve a faked truncated answer to the downstream client to force it to move to TCP. This improves performance and robustness in the face of broken clients which can't fall back to TCP.
  • No longer remove data from truncated upstream answers. If an upstream replies with a truncated answer, but the answer has some RRs included, return those RRs, rather than returning and empty answer.
    -Fix handling of EDNS0 UDP packet sizes. When talking upstream we always add a pseudoheader, and set the UDP packet size to --edns-packet-max. Answering queries from downstream, we get the answer (either from upstream or local data) If local data won't fit the advertised size (or 512 if there's not an EDNS0 header) return truncated. If upstream returns truncated, do likewise. If upstream is OK, but the answer is too big for downstream, truncate the answer.
  • Modify the behaviour of --synth-domain for IPv6. When deriving a domain name from an IPv6 address, an address such as 1234:: would become 1234--.example.com, which is not legal in IDNA2008. Stop using the :: compression method, so 1234:: becomes 1234-0000-0000-0000-0000-0000-0000-0000.example.com
  • Fix broken dhcp-relay on *BSD. Thanks to Harold for finding this problem.
  • Add --dhcp-option-pxe config. This acts almost exactly like --dhcp-option except that the defined option is only sent when replying to PXE clients. More importantly, these options are sent in reply PXE clients when dnsmasq in acting in PXE proxy mode. In PXE proxy mode, the set of options sent is defined by the PXE standard and the normal set of options is not sent. This config allows arbitrary options in PXE-proxy replies. A typical use-case is to send option 175 to iPXE. Thanks to Jason Berry for finding the requirement for this.
  • Support PXE proxy-DHCP and DHCP-relay at the same time. When using PXE proxy-DHCP, dnsmasq supplies PXE information to the client, which also talks to another "normal" DHCP server for address allocation and similar. The normal DHCP server may be on the local network, but it may also be remote, and accessed via a DHCP relay. This change allows dnsmasq to act as both a PXE proxy-DHCP server AND a DHCP relay for the same network.
  • Fix erroneous "DNSSEC validated" state with non-DNSSEC upstream servers. Thanks to Dominik Derigs for the bug report.

Related issue or feature (if applicable): N/A

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. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

simonkelley and others added 30 commits February 23, 2024 22:01
Also applies to SetFilterAAAA.

Thanks to Clayton Craft for spotting the issue.

Signed-off-by: DL6ER <[email protected]>
In generalising the RR filter code, the Dbus methods
controlling filtering A and AAAA records
got severely broken. This, and the previous commit,
fixes things.

Signed-off-by: DL6ER <[email protected]>
CAP_NET_ADMIN is needed in the DHCPv4 code to place entries into
the ARP cache. If it's configured to unconditionally broadcast
to unconfigured clients, it never touches the ARP cache and
doesn't need CAP_NET_ADMIN.

Thanks to Martin Ivičič <[email protected]> for prompting this.

Signed-off-by: DL6ER <[email protected]>
…ource problem and fail validation with suitable logging.

Signed-off-by: DL6ER <[email protected]>
Confusion in the code to free old DHCP configuration when it's
being reloaded causes invalid pointers to be followed and a crash.

https://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2024q4/017764.html

has a more complete explanation of the problem.

Signed-off-by: DL6ER <[email protected]>
The msg_controllen field passed to sendmsg is computed using the
CMSG_SPACE(), which is correct, but CMSG_SPACE() can include
padding bytes at the end of the passed structure if they are required
to align subsequent structures in the buffer. Make sure these
bytes are zeroed to avoid passing uninitiased memory to the kernel,
even though it will never touch these bytes.

Also tidy up the mashalling code in send_from to use pointers to
the structure being filled out, rather than a temporary copy which
then gets memcpy()'d into place. The DHCP sendmsg code has always
worked like this.

Thanks to  Dominik Derigs for running Memcheck as submitting the
initial patch.

Signed-off-by: DL6ER <[email protected]>
Heretofore, when a validating the result of an external query triggers
a DNSKEY or DS query and the result of that query is truncated, dnsmasq
has forced the whole validation process to move to TCP by returning a
truncated reply to the original requestor. This forces the original
requestor to retry the query in TCP mode, and the DNSSEC subqueries
also get made via TCP and everything works.

Note that in general the actual answer being validated is not large
enough to trigger truncation, and there's no reason not to return that
answer via UDP if we can validate it successfully. It follows that
a substandard client which can't do TCP queries will still work if the
answer could be returned via UDP, but fails if it gets an artifically
truncated answer and cannot move to TCP.

This patch teaches dnsmasq to move to TCP for DNSSEC queries when
validating UDP answers. That makes the substandard clients mentioned
above work, and saves a round trip even for clients that can do TCP.

Signed-off-by: DL6ER <[email protected]>
…P) retry once we've switched to TCP.

Signed-off-by: DL6ER <[email protected]>
…ating.

A relatively common situation is that the reply to a downstream query
will fit in a UDP packet when no DNSSEC RRs are present, but overflows
when the RRSIGS, NSEC ect are added. This extends the automatic
move from UDP to TCP to downstream queries which get truncated replies,
in the hope that once stripped of the DNSSEC RRs, the reply can be returned
via UDP, nwithout making the downstream retry with TCP.

If the downstream sets the DO bit, (ie it wants the DNSSEC RRs, then
this path is not taken, since the downstream will have to get a truncated
repsonse and retry to get a correct answer.

Signed-off-by: DL6ER <[email protected]>
We do this in many cases anyway (DNSSEC, fast-retry) and doing
it unconditionally allows much simplification of knarly code, to follow.

Signed-off-by: DL6ER <[email protected]>
Now we're always saving the query, this is no longer necessary,
and allows the removal of a lot of quite hairy code.

Much more code removal to come, once the TCP code path is also purged.

Signed-off-by: DL6ER <[email protected]>
They have smaller structs which don't include that field,
so this is a buffer overlow.

Error introduced in f5cdb007d8845dde8e5053229f47b46b1b756473

Signed-off-by: DL6ER <[email protected]>
We only need to order two server records on the ->serial field.
Literal address records are smaller and don't have
this field and don't need to be ordered on it.
To actually provoke this bug seems to need the same server-literal
to be repeated twice, eg --address=/a/1.1.1.1 --address-/a/1.1.1.1
which is clearly rare in the wild, but if it did exist it could
provoke a SIGSEV. Thanks to Daniel Rhea for fuzzing this one.

Signed-off-by: DL6ER <[email protected]>
…ong.

Thanks to Daniel Rhea for finding this one.

Signed-off-by: DL6ER <[email protected]>
The saved query includes all the modifications, made on first
forwarding, which simplifies retries.

Signed-off-by: DL6ER <[email protected]>
harry and others added 5 commits December 19, 2024 09:57
Analogous fix to 5a1f2c577db58ea47727f1b6900c0be25e6db205 for DHCPv6 relay.

Signed-off-by: DL6ER <[email protected]>
@DL6ER
Copy link
Member Author

DL6ER commented Dec 19, 2024

Opening this as draft for now as there is an issue with 7572e59 which is currently under investigation in private communication.

@DL6ER DL6ER changed the title Update embedded dnsmasq to latest tas v2.91test1 Update embedded dnsmasq to latest tag v2.91test1 Dec 20, 2024
@DL6ER DL6ER changed the title Update embedded dnsmasq to latest tag v2.91test1 Update embedded dnsmasq to latest tag v2.91test2 Dec 21, 2024
@DL6ER DL6ER requested a review from a team December 21, 2024 10:43
@DL6ER DL6ER marked this pull request as ready for review December 21, 2024 10:43
@DL6ER DL6ER merged commit f7bcff3 into development Dec 22, 2024
21 checks passed
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/dns-resolution-not-working-and-docker-container-unhealthy-after-recent-update/74940/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants