get_rdata() contains another instance of a crash caused by
a false value of RDLEN in a resource-record.
An RR containing name which is longer than the rdlen of the RR can
get get_rdata into a state where it calculates the length of the data
following the name as a negative number which is then promoted
to a very large positive number, and causing SIGSEGV.
This can be triggered by crafted packets from malicous DNS servers,
but only when DNSSEC validation is enabled. The attack packets do not
have to be correctly DNSSEC signed.
Thanks to Tristan G. for spotting this problem.
Also fix error handling for RRs with a fixed size fields in the specification
where the rrsize indicates that there are zero bytes in that position.
Also ripple effects for most of its callers.
This fixes an OOB write caused by bad pointer arithmetic
calculating the pointer-to-end-of-packet used before.
Thanks to Donghyeon Jeong for spotting the bug.
commit 1269f074f8 broke
hostname_issubdomain() and then accidentally fixed
some of the problems by inverting the argument order
in the new uses of the function it introduced.
All the pre-existing calls to hostname_issubdomain()
were left in a non-working state.
This fixes hostname_issubdomain() back to the state
before 1269f074f8, adds
some logic to give correct answers when comparing
to the root domain, and gets right the new calls to
hostname_issubdomain() added in
1269f074f8
Thanks to Jean Thomas for spotting this problem.
Bug report from Royce M <royce@xchglabs.com>
This avoids crafted packets which give a value for rdlen _less_
then the space taken up by the fixed data and the signer's name
and engender a negative calculated length for the signature.
Report from Royce M <royce@xchglabs.com>.
Location: dnssec.c:1290-1306, dnssec.c:1450-1463
The bitmap window iteration advances by p[1] instead of p[1]+2
(missing the 2-byte window header). With bitmap_length=0, both rdlen and p are
unchanged, causing an infinite loop and dnsmasq stops responding to all queries.
Bug is reachable before RRSIG validation
(confirmed by the source comment at line 2125), so no valid
DNSSEC signatures are needed.
This is the stable release fix for CVE-2026-2291
Dnsmasq recieves and sends domain names within DNS packets in RFC-1035
format, a string of counted labels terminated by a zero length label.
Internally to dnsmasq, domain names are represented as zero-terminated C strings
with the labels seperated by '.' characters, like the normal presentation
format for domain names.
The RFC-1035 limit for a domain name in wire format is 255 octets, and
when converted to presentation format, this makes a string of
maximum 253 characters. However dnsmasq needs to be able to
represent any 8-bit character within a label, and this causes
problems with /000 (which would terminate the c-string early) and
'.' (which would terminate a label early). To avoid this, dnsmasq
escapes both of these characters, which means that they take two bytes
in the string. The domain name with must charaters in it has four labels
and the 255 octet wire length limits these four labels to a total of
250 characters. The other five octets are the four label length bytes
and the zero length byte terminator. If all 250 characters need escaping
that makes the maximum length of the dnsmasq internal format 503
characters (250 escaped characters in labels, amd three dots between
four labels) Since this format is zero-terminated c-string, buffers
have to be allocated as 504 bytes.
This arrangement grew in a somewhat ad-hoc manner and early dnsmasq
didn't do the escaping trick, and didn't really differentiate between
the lengths of the wire format and the presentation format, since
they were very similar. It also, for reasons lost in time, inherited
a defintion for MAXDNAME from early BIND headers, set at 1025 bytes.
This value was used as the buffer size for both wire and presentation
formats.
This patch makes everything consistent. It declares two constants
MAXDNAME 255 /* max size of wire format domain name */
MAXDNAMESTR 503 /* max size of internal format created from a 255 octet wire format name */
All internal name buffers are allocated as MAXDNAMESTR+1 bytes,
to hold a maximum size internal format name and zero termination.
Names parsed out of incoming packets are rejected if their
wire format is greater than 255 bytes, which ensures that
their internal representation cannot exceed 503 characters.
Names inserted into outgoing packets are failed similarly if
they exceed 255 bytes. (this would in theory be possible
for a legal internal-representaion name if it has at least
one unescaped character in a label.
The addition of "(not supported)" to logs of DS and DNSKEY replies
overflows the buffer used to construct the string.
Re-arrange things to avoid this, and add checks to avoid the same problem
if the logging calls change in the future.
Thanks to Yiwei Hou for finding this.
The problem exists is DNSSEC is enabled and query logging is also
enabled. The overwrite is of bounded length and the bytes
written are not in control of an attacker, so this is not considered
a likely remote-execution vector.
If there's no configured server for the parent of the domain
covered by a domain-specific server, assume that said domain is
not signed. This extends the existing logic which makes similar
decisions for replies to DS queries from tghe parent server.
While this won't do harm on systems that do 2's completement,
it triggers the compilers' undefined-behavior sanitizer and
fixes sanitizer error such as the one below (where the 1694... will
vary) and is distracting while debugging.
dnssec.c:1427:12: runtime error: left shift of 1694604366 by 1 places
cannot be represented in type 'int'
A CNAME reply to a DNSSEC query was confusing the validation logic.
It now accepts a signed CNAME reply to a DS query as proof that no DS
exists at the domain. This fixes the DS/zone break detection logic.
If we're doing DNSSEC validation and fail because the upstream
reply to our query is SERVFAIL, log this as the reason in
validation result line.
This will make maintainers lives easier when they get reports
of "wrong" validation failure, which is sometimes an upstream problem.
The presence of wrong RRSIG RRs in replies causes DNSSEC
validation to fail even when the RRs do not require validation
because the zone is unsigned.
This patch solves the problem and tidies up the logic.
Included is some fixes for hostname_issubdomain() which suffered
some confusions about argument order :) I've clarified that and
checked every to the function to make sure they are using the
correct argument order.
Note that, at the time of this commit, Google DNS appears to have
the same bug, so if you're using 8.8.8.8 or friends as upstream,
resolving the broken zones (eg rivcoed.org) will still fail.
Thanks to Petr Menšík for the bug report.
If we have a wildcard record *.example.com and recieve a query for
a.example.com then that's OK, but we have to check that there isn't
an actual a.example.com record. The corner case is when we get a
query for *.example.com in that case the non-existence check
is not required, but was being done.
Thanks to Jan Breig for spotting this.
The large public DNS services seem not to return proof-of-nonexistence
for DS records at the start of RFC-1918 in-addr.arpa domains and the their
IPv6 equivalents. 10.in-addr.arpa, 168.192.in-addr.arpa etc.
Since dnsmasq already has an option which instructs it not bother
upstream servers with pointless queries about these address ranges,
namely --bogus-priv, we extend that to enable behaviour which allows
dnsmasq to assume that insecure NXDOMAIN replies for these domains
are expected and to assume that the domains are legitimately unsigned.
This behaviour only matters when some address range is directed to
another upstream server using --rev-server. In that case it allows
replies from that server to pass DNSSEC validation. Without such a
server configured, queries are never sent upstream so they are never
validated and the new behaviour is moot.
This should just work in all cases now. If the normal chain-of-trust exists into
the delegated domain then whether the domain is signed or not, DNSSEC
validation will function normally. In the case the delgated domain
is an "overlay" on top of the global DNS and no NS and/or DS records
exist connecting it to the global dns, then if the domain is
unsigned the situation will be handled by synthesising a
proof-of-non-existance-of-DS for the domain and queries will be
answered unvalidated; this action will be logged. A signed domain
without chain-of-trust can be validated if a suitable trust-anchor
is provided using --trust-anchor.
Thanks to Uwe Kleine-König for prompting this change, and contributing
valuable insights into what could be improved.
This provides extra protection against reply-spoof attacks.
Since DNS queries are case-insensitive, it's possible to randomly flip
the case of letters in a query and still get the correct answer back.
This adds an extra dimension for a cache-poisoning attacker to guess
when sending replies in-the-blind since it's expected that the
legitimate answer will have the same pattern of upper and lower case
as the query, so any replies which don't can be ignored as
malicious.
The amount of extra entropy clearly depends on the number
of a-z and A-Z characters in the query, and this implementation puts a
hard limit of 32 bits to make rescource allocation easy. This about
doubles entropy over the standard random ID and random port
combination.
This was kinda strange before, with a lot of cargo-cult copied code,
and no clear strategy.
Now it works like this:
When talking upstream we always add a pseudoheader, and set the
UDP packet size to --edns-packet-max unless we've had problems
talking to a server, when it's reduced to 1280 if that fixes things.
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 pseudoheader) return truncated. If upstream
returns truncated, do likewise. If upstream is OK, but the answer is
too big for downstream, truncate the answer.
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.
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.
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.
On 15/5/2023 8.8.8.8 was returning SERVFAIL for a query on ec.europa.eu
ec.europa.eu is not a domain cut, that happens at jrc.ec.europa.eu. which
does return a signed proof of non-existance for a DS record.
Abandoning the search for a DS or proof of non existence at ec.europa.eu
renders everything within that domain BOGUS, since nothing is signed.
This code changes behaviour on a SERVFAIL to continue looking
deeper for a DS or proof of its nonexistence.
Error: CHECKED_RETURN (CWE-252): [#def26]
dnsmasq-2.86rc3/src/dnssec.c:727: check_return: Calling "extract_name" without checking return value (as is done elsewhere 9 out of 10 times).
dnsmasq-2.86rc3/src/dnssec.c:459: example_checked: Example 1: "extract_name(header, plen, &p, keyname, 1, 0)" has its value checked in "extract_name(header, plen, &p, keyname, 1, 0)".
dnsmasq-2.86rc3/src/dnssec.c:269: example_checked: Example 2: "extract_name(header, plen, &state->ip, state->buff, 1, 0)" has its value checked in "extract_name(header, plen, &state->ip, state->buff, 1, 0)".
dnsmasq-2.86rc3/src/dnssec.c:569: example_checked: Example 3: "extract_name(header, plen, &p, keyname, 1, 0)" has its value checked in "extract_name(header, plen, &p, keyname, 1, 0)".
dnsmasq-2.86rc3/src/rfc1035.c:648: example_checked: Example 4: "extract_name(header, qlen, &p1, name, 1, 0)" has its value checked in "extract_name(header, qlen, &p1, name, 1, 0)".
dnsmasq-2.86rc3/src/rfc1035.c:787: example_checked: Example 5: "extract_name(header, qlen, &p1, name, 1, 0)" has its value checked in "extract_name(header, qlen, &p1, name, 1, 0)".
# 725| /* namebuff used for workspace above, restore to leave unchanged on exit */
# 726| p = (unsigned char*)(rrset[0]);
# 727|-> extract_name(header, plen, &p, name, 1, 0);
# 728|
# 729| if (key)
Error: CHECKED_RETURN (CWE-252): [#def27]
dnsmasq-2.86rc3/src/dnssec.c:1020: check_return: Calling "extract_name" without checking return value (as is done elsewhere 7 out of 8 times).
dnsmasq-2.86rc3/src/auth.c:140: example_checked: Example 1: "extract_name(header, qlen, &p, name, 1, 4)" has its value checked in "extract_name(header, qlen, &p, name, 1, 4)".
dnsmasq-2.86rc3/src/dnssec.c:771: example_checked: Example 2: "extract_name(header, plen, &p, name, 1, 4)" has its value checked in "extract_name(header, plen, &p, name, 1, 4)".
dnsmasq-2.86rc3/src/hash-questions.c:57: example_checked: Example 3: "extract_name(header, plen, &p, name, 1, 4)" has its value checked in "extract_name(header, plen, &p, name, 1, 4)".
dnsmasq-2.86rc3/src/rfc1035.c:1028: example_checked: Example 4: "extract_name(header, qlen, &p, name, 1, 4)" has its value checked in "extract_name(header, qlen, &p, name, 1, 4)".
dnsmasq-2.86rc3/src/rfc1035.c:1438: example_checked: Example 5: "extract_name(header, qlen, &p, name, 1, 4)" has its value checked in "extract_name(header, qlen, &p, name, 1, 4)".
# 1018|
# 1019| p = (unsigned char *)(header+1);
# 1020|-> extract_name(header, plen, &p, name, 1, 4);
# 1021| p += 4; /* qtype, qclass */
# 1022|
This patch also changes the method of calling querystr() such that
it is only called when logging is enabled, to eliminate any
possible performance problems from searching the larger table.
Simon Kelley