When `tcp_request()` answers from stale cache data, it closes the
connection right away, so that the child process can refresh the name
and exit. A query the client has already sent on that connection is
never read, and its answer is lost.
glibc runs into this: after a truncated UDP reply, it sends the A and
AAAA queries together on one TCP connection, gets EOF after the first
answer and fails the lookup without retrying the second query. In a
test with both answers expired, 7 of 20 such lookups lost the second
answer.
Close the connection only if the client has not sent anything else
yet. Otherwise, refresh the name and go on answering the queries on the
connection, as for any other forwarded query.
Signed-off-by: DL6ER <dl6er@dl6er.de>
On further analysis, the problem is deeper: Other
error paths (which are not accessible to an attack) can also
return an incorrect header->id value, and most error paths
return incorrect query case if --do-0x20-encode is in use.
An oversight in commit f9f8d19bf5
leaves a code path where a repeat of a query gets an error reply
which discloses the id field used in interactions with upstream
servers ro get an answer to the query. By triggering this
code path, an attacker can determine the id, which makes Kaminsky
cache poisoning attacks much less expensive and much more certain.
This bug exists in stable releases 2.91, 2.92 2.92rel2 and 2.93
Thanks to Ronen Shustin from Project Atlas, Wiz for finding
this problem.
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.
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.
Bug report from Royce M <royce@xchglabs.com>
Location: forward.c:713, edns0.c:421
With --add-subnet enabled, process_reply() passes the OPT record
length (~23 bytes) instead of the packet length to check_source().
All internal bounds checks fail, and the function always returns 1.
ECS source validation per RFC 7871 Section 9.2 is completely bypassed.
The `if (serv == last) return;` was inside the loop
`for (serv = first; serv != last; ...)`, making it dead code — the
loop condition ensures the body never runs when serv == last. Replies
arriving on an unrecognised file descriptor were therefore never
rejected by the fd check. Moving the guard after the loop causes it to
fire when no server socket matches.
Signed-off-by: Dominik <dl6er@dl6er.de>
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.
In the DNS TCP code, there are a couple of places where
we have a buffer containing a message which we need to
send via TCP. The DNS protocol is that this is sent as
<16-bit length in network order><message>
Making two write calls, one for the length and one
for the message causes the TCP stack to send two packets,
one for each. A single packet containing both is
preferable from a performance POV.
Implement a scatter-gather version of our read_write()
wrapper and use it where necessary to send TCP DNS messages.
This fixes the plethora of 64k buffers that got allocated when doing
DNSSEC over TCP.
By using the UDP buffer is pass the query into tcp_talk() and
allowing tcp_talk to allocate its output buffer one the size
of the reply is known, we only need to allocate as much memory as
is required. The final reply to the TCP query still needs the 64k
buffer because answer_request() and answer_auth() are not capable
of extending their output buffers.
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.
These can't be held forever by a client that opens a connection and
then sends nothing, but we can still be DoSsed by a server which
accepts a connection and never replies.
If the TCP process times out, it sends that information to the
parent so that the UDP query can be unblocked. As the TCP
timeout is 150s it's highly unlikely that any client will
still be waiting, but the point is to free resources.
get_new_frec() garbage collects struct frec when they are
(by default) 40s old. This means that in an active dnsmasq
which is getting new queries, answers to old queries start to
be ignored soon after they hit 40s old. If there are no new
queries, get_new_frec() doesn't get called, and answers to old
queries will still be accepted for longer.
This patch stops lookup_frec() from returning old frecs which
are ready for GC. This helps avoid yet another variation
on the birthday attack, which is no particularly relevant,
given the need for an idle server.
get_new_frec() is not supposed to ever free an frec when
the force arg is set. Make this so. In theory, this fixes
a bug, but it's practically impossible to provoke and I have
no evidence that it has ever happened IRL.
TFTP fails with a timeout when the transfer exceeds 128K blocks.
The block field in an ACK packet is 16 bits, so large transfers
need to deal with this field wrapping. Testing failure meant
that it worked for the first wrap, but not for subsequent ones.
Having fixed the block number problem, file size calculations
accidentally being done in 32 bits not 64 bits then broke
transfers larger than 2G or maybe 4G.
The first problem was probably introduced in commit
ebef27f321 and has not appeared
in a stable release. The second appears to have been there forever.
Clearly, nobody is mad enough to transfer multi-gigabyte files
over TFTP.
Thanks to Jean-François JUBLIN for spotting this and supplying
useful packet dumps that made it easy to diagnose.
The proximate cause for doing this is to fix a bug that
causes replies to PTR queries with more than one answer to have the
second and subsequent answers ignored.
The fix turned into a small re-write which removed a very old hack.
When caching reponses which include CNAME records, the cache system
stores the CNAME with a link to the record representing the target of
the CNAME. This isn't possible for PTR records representing IP
addresses since the name stored is the target of the PTR, record and
its name is inferred from the address in the cache record. Such
cache records have the F_REVERSE flag set. To get
around this, long ago, the code which stores such records elided the
CNAME entirely, so
4.3.2.1.in-addr.arpa CNAME 18/3.2.1.in-addr.arpa
18/3.2.1.in-addr.arpa PTR myhost.example.com
would be stored as
4.3.2.1.in-addr.arpa PTR myhost.example.com
and returned from the cache to subsequent requestor in that form.
Since that hack was committed, dnsmasq has learned to cache arbitrary
RRs. So now we can store the PTR records for all the no-trivial cases.
The means the CNAME chains ending in PTR records don't get mangled,
and we can store PTR records whose name in not w.x.y.x.in-addr.arpa
or the IPv6 equivalent.
If DNS is happening over TCP, the query is handled by a forked
process. Of ipset ot nftset is configured, this might include
inserting addresses in the *sets. Before this update, that
was done by the forked process using handles inherited from the
parent "master" process.
This is inherently racy. If the master process or another
child process tries to do updates at the same time, the
updates can clash and fail.
To see this, you need a busy server doing lots of DNS
queries over TCP, and ipset or nftset configured.
Going forward, we use the already established pipe to send the
updates from the child back to the master process, which
serialises them.
There's no point in checking if the query has edns0 headers _after_
adding our own.
This has the affect that if --add-cpe-id or --add-subnet or their friends
are configured, a query via TCP without EDNS0 will get an answer with EDNS0.
It's highly unlikely that this breaks anything, but it is incorrect.
Thanks to Tijs Van Buggenhout for spotting this.
This is a regession introduced in 3b6df06fb8.
When dnsmasq is started without upstreams (yet), but a
DNS query comes in that needs forwarding dnsmasq now potentially crashes as
the value for "first" variable is undetermined.
A segmentation violation occurs when the index
is out of bounds of serverarray.
Credits go to pedro0311 <pedro@freshtomato.org>
To complement the previous one, which fixed the retry path
when the query is retried from a different id/source address, this
fixes retries from the same id/source address.
A retry to upstream DNS servers triggered by the following conditions
1) A query asking for the same data as a previous query which has not yet been answered.
2) The second query arrives more than two seconds after the first.
3) Either the source of the second query or the id field differs from the first.
fails to set the case of the retry to the same pattern as the first attempt.
However dnsmasq expects the reply from upstream to have the case
pattern of the first attempt.
If the answer to the retry arrives before the answer to the first
query, dnsmasq will notice the case mismatch, log an error, and
ignore the answer.
The worst case scenario would be the first upstream query or reply is
lost and there would follow a short period where all queries for that
particular domain would fail.
This is a 2.91 development issue, it doesn't apply to previous stable releases.
Fix a case sensitivity problem which has been lurking for a long while.
When we get example.com and Example.com and combine them, we send whichever
query arrives first upstream and then later answer it, and we also
answer the second with the same answer. That means that if example.com
arrives first, it will get the answer example.com - good - but Example.com
will _also_ get the answer example.com - not so good.
In theory, fixing this is simple without having to keep seperate
copies of all the queries: Just use the bit-vector representation
of case flipping that we have for 0x20-encoding to keep the
differences in case. The complication comes from the fact that
the existing bit-vector code only holds data on the first 32 alpha
letters, because we only flip that up to many for 0x20 encoding.
In practise, the delta between combined queries can almost always
be represented with that data, since almost all queries are
all lower case and we only purturb the first 32 letters with
0x20 encoding. It's therefore worth keeping the existing,
efficient data structure for the 99.9% of the time it works.
For the 0.1% it doesn't, however, one needs an arbitrary-length data
structure with the resource implications of that.
Thanks to Peter Tirsek for the well researched bug report which set me
on to these problems.
We must only compare case when mapping an answer from upstream
to a forwarding record, not when checking a query to see if it's a
duplicate. Since the saved query name is scrambled, that ensures
that almost all such checks will wrongly fail.
Thanks to Peter Tirsek for an exemplary bug report for this.
The "bit 0x20 encoding" implemented in 995a16ca0c
can interact badly with (hopefully) rare broken upstream servers. Provide
an option to turn it off and a log message to give a clue as to why DNS service
is non-functional.
DL6ER