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 v2.90 #1875

Merged
merged 50 commits into from
Feb 13, 2024
Merged

Update embedded dnsmasq to v2.90 #1875

merged 50 commits into from
Feb 13, 2024

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Feb 3, 2024

What does this implement/fix?

See title, this brings development-v6 to the just released dnsmasq v2.90 including a fix against a potential DoS vulnerability when DNSSEC is enabled.

CHANGELOG

  • Add limits on the resources used to do DNSSEC validation.
    DNSSEC introduces a potential CPU DoS, because a crafted domain can force a validator to a large number of cryptographic operations whilst attempting to do validation. When using TCP transport a DNSKEY RRset contain thousands of members and any RRset can have thousands of signatures. The potential number of signature validations to follow the RFC for validation for one RRset is the cross product of the keys and signatures, so millions. In practice, the actual numbers are much lower, so attacks can be mitigated by limiting the amount of cryptographic "work" to a much lower amount. The actual limits are number a signature validation fails per RRset(20), number of signature validations and hash computations per query(200), number of sub-queries to fetch DS and DNSKEY RRsets per query(40), and the number of iterations in a NSEC3 record(150). These values are sensible, but there is, as yet, no standardisation on the values for a "conforming" domain, so a new option --dnssec-limit is provided should they need to be altered. The algorithm to validate DS records has also been altered to reduce the maximum work from cross product of the number of DS records and number of DNSKEYs to the cross product of the number of DS records and supported DS digest types. As the number of DS digest types is in single figures, this reduces the exposure.

    Credit is due to Elias Heftrig, Haya Schulmann, Niklas Vogel, and Michael Waidner from the German National Research Center for Applied Cybersecurity ATHENE for finding this vulnerability.

    CVE 2023-50387 and CVE 2023-50868 apply. Note that the is a security vulnerablity only when DNSSEC validation is enabled.

  • Fix reversion in --rev-server introduced in 2.88 which caused breakage if the prefix length is not exactly divisible by 8 (IPv4) or 4 (IPv6).

  • Fix possible SEGV when there server(s) for a particular domain are configured, but no server which is not qualified for a particular domain.
    Thanks to Daniel Danzberger for spotting this bug.

  • Set the default maximum DNS UDP packet sice to 1232. This has been the recommended value since 2020 because it's the largest value that avoid fragmentation, and fragmentation is just not reliable on the modern internet, especially for IPv6. It's still possible to override this with --edns-packet-max for special circumstances.

  • Add --no-dhcpv4-interface and --no-dhcpv6-interface for better control over which interfaces are providing DHCP service.

  • Fix issue with stale caching: After replying with stale data, dnsmasq sends the query upstream to refresh the cache asynchronously and sometimes sends the wrong packet: packet length can be wrong, and if an EDE marking stale data is added to the answer that can end up in the query also. This bug only seems to cause problems when the usptream server is a DOH/DOT proxy. Thanks to Justin He for the bug report.

  • Add configurable caching for arbitrary RR-types.

  • Add --filter-rr option, to filter arbitrary RR-types.
    --filter-rr=ANY has a special meaning: it filters the answers to queries for the ANY RR-type.


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.

DL6ER and others added 20 commits November 22, 2023 17:45
Signed-off-by: DL6ER <dl6er@dl6er.de>
…0e105cae07ae55

Signed-off-by: DL6ER <dl6er@dl6er.de>
Thanks to Kevin Darbyshire-Bryant for the bug report.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Thanks to  Kevin Darbyshire-Bryant for the initial patch, which was
modified by srk - any remaining bugs are his.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Bug report here:
https://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2023q4/017332.html

This error probably has no practical effect since even if the hash
is wrong, it's only compared internally to other hashes computed using
the same code.

Understanding the error:

hash-questions.c:168:21: runtime error: left shift of 128 by 24 places
cannot be represented in type 'int'

requires a certain amount of c-lawyerliness. I think the problem is that

m[i] = data[j] << 24

promotes the unsigned char data array value to int before doing the shift and
then promotes the result to unsigned char to match the type of m[i].
What needs to happen is to cast the unsigned char to unsigned int
BEFORE the shift.

This patch does that with explicit casts.

Signed-off-by: DL6ER <dl6er@dl6er.de>
In bind-dynamic mode, its OK to fail to bind a socket to an address
given by --listen-address if no interface with that address exists
for the time being. Dnsmasq will attempt to create the socket again
when the host's network configuration changes.

The code used to ignore pretty much any error from bind(), which is
incorrect and can lead to confusing behaviour. This change make ONLY
a return of EADDRNOTAVAIL from bind() a non-error: anything else will be
fatal during startup phase, or logged after startup phase.

Thanks to Petr Menšík for the problem report and first-pass patch.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Add the relevant information to the metrics and to the output of
dump_cache() (which is called when dnsmasq receives SIGUSR1).
Hence, users not collecting metrics will still be able to
troubleshoot with SIGUSR1. In addition to the current usage,
dump_cache() contains the information on the highest usage
since it was last called.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
At startup, the leases file is read by lease_init(), and
in lease_init() undecorated hostnames are expanded into
FQDNs by adding the domain associated with the address
of the lease.

lease_init() happens relavtively early in the startup, party because
if it calls  the dhcp-lease helper script, we don't want that to inherit
a load of sensitive file descriptors. This has implications if domains
are defined using the --domain=example.com,eth0 format since it's long
before we call enumerate_interfaces(), so get_domain fails for such domains.

The patch just moves the hostname expansion function to a seperate
subroutine that gets called later, after enumerate_interfaces().

Signed-off-by: DL6ER <dl6er@dl6er.de>
By design, dnsmasq forwards queries for RR-types it has no data
on, even if it has data for the same domain and other RR-types.

This can lead to an inconsitent view of the DNS when an upstream
server returns NXDOMAIN for an RR-type and domain but the same domain
but a different RR-type gets an answer from dnsmasq. To avoid this,
dnsmasq converts NXDOMAIN answer from upstream to NODATA answers if
it would answer a query for the domain and a different RR-type.

An oversight missed out --synth-domain from the code to do this, so
--synth-domain=thekelleys.org.uk,192.168.0.0/24
would result in the correct answer to an A query for
192-168.0.1.thekelleys.org.uk and an AAAA query for the same domain
would be forwarded upstream and the resulting NXDOMAIN reply
returned.

After the fix, the reply gets converted to NODATA.

Thanks to Matt Wong for spotting the bug.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Similar to local-service, but more strict. Listen only on localhost
unless other interface is specified. Has no effect when interface is
provided explicitly. I had multiple bugs fillen on Fedora, because I have
changed default configuration to:

interface=lo
bind-interfaces

People just adding configuration parts to /etc/dnsmasq.d or appending to
existing configuration often fail to see some defaults are already there.
Give them auto-ignored configuration as smart default.

Signed-off-by: Petr Menšík <pemensik@redhat.com>

Do not add a new parameter on command line. Instead add just parameter
for behaviour modification of existing local-service option. Now it
accepts two optional values:
- net: exactly the same as before
- host: bind only to lo interface, do not listen on any other addresses
  than loopback.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from a team February 3, 2024 21:11
@DL6ER DL6ER marked this pull request as draft February 8, 2024 16:19
@DL6ER DL6ER removed the request for review from a team February 8, 2024 16:19
@DL6ER DL6ER changed the title Update embedded dnsmasq to v2.90test3 Update embedded dnsmasq to v2.90 Feb 8, 2024
@DL6ER
Copy link
Member Author

DL6ER commented Feb 8, 2024

I set this PR back into draft mode as we are awaiting the release of the final v2.90 sometime next week. I'll prepare a backport to v5.x in a separate PR.

Now we can cache arbirary RRs, give more correct answers when
replying negative answers from cache.

To implement this needed the DNS-doctor code to be untangled from
find_soa(), so it should be under suspicion for any regresssions
in that department.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
In extract_addresses() the "secure" argument is only set if the
whole reply is validated (ie the AD bit can be set). Even without
that, some records may be validated, and should be marked
as such in the cache.

Related, the DNS doctor code has to update the flags for individual
RRs as it works, not the global "secure" flag.

Signed-off-by: DL6ER <dl6er@dl6er.de>
…atch importing

Signed-off-by: DL6ER <dl6er@dl6er.de>
DL6ER and others added 23 commits February 13, 2024 07:12
Thanks to Dominik Derigs for an earlier patch which inspired this.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…ies That Have QTYPE=ANY) by default

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…ready set elsehow)

Signed-off-by: DL6ER <dl6er@dl6er.de>
An attacker can create DNSSEC signed domains which need a lot of
work to verfify. We limit the number of crypto operations to
avoid DoS attacks by CPU exhaustion.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
By calculating the hash of a DNSKEY once for each digest algo,
we reduce the hashing work from (no. DS) x (no. DNSKEY) to
(no. DNSKEY) x (no. distinct digests)

The number of distinct digests can never be more than 255 and
it's limited by which hashes we implement, so currently only 4.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER marked this pull request as ready for review February 13, 2024 16:10
@DL6ER DL6ER requested a review from a team February 13, 2024 16:10
@DL6ER
Copy link
Member Author

DL6ER commented Feb 13, 2024

All CI builds 🟢

@DL6ER DL6ER merged commit 6681804 into development-v6 Feb 13, 2024
18 checks passed
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.

5 participants