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.
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.
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.
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.
If the cache insertion process fails for any reason, any
blockdata storage allocated needs to be freed.
Thanks to Damian Sawicki for spotting the problem and
supplying patches against earlier releases. This patch by SRK,
and any bugs are his.
answer_request() builds answers in the same packet buffer
as the request. This means that any EDNS0 header from the
original request is overwritten. If the answer is in cache, that's
fine: dnsmasq adds its own EDNS0 header, but if the cache lookup fails
partially and the request needs to be sent upstream, it's a problem.
This was fixed a long, long time ago by running the cache
lookup twice if the request included an EDNS0 header. The first time,
nothing would be written to the answer packet, nad if the cache
lookup failed, the untouched question packet was still available
to forward upstream. If cache lookup succeeded, the whole thing
was done again, this time writing the data into the reply packet.
In a world where EDNS0 was rare and so was memory, this was a
reasonable solution. Today EDNS0 is ubiquitous so basically
every query is being looked up twice in the cache. There's also
the problem that any code change which makes successive cache lookups
for a query possibly return different answers adds a subtle hidden
bug, because this hack depends on absence of that behaviour.
This commit removes the lookup-twice hack entirely. answer_request()
can now return zero and overwrite the question packet. The code which
was previously added to support stale caching by saving a copy of the
query in the block-storage system is extended to always be active.
This handles the case where answer_request() returns no answer OR
a stale answer and a copy of the original query is needed to forward
upstream.
Caching an answer which has more that one RR, with at least
one answer being <=13 bytes and at least one being >13 bytes
can screw up the F_KEYTAG flag bit, resulting in the wrong
type of the address union being used and either a bad value
return or a crash in the block code.
Thanks to Dominik Derigs and the Pi-hole project for finding
and characterising this.
the compiler padding it with an extra 8 bytes.
Use the F_KEYTAG flag in a a cache record to discriminate between
an arbitrary RR stored entirely in the addr union and one
which has a point to block storage.
If a NODATA answer is returned instead of actual data for A or AAAA
queries because of the existence of --filter-A or --filter-AAAA
config options, then mark the replies with an EDE "filtered" tag.
Basic patch by Petr Menšík, tweaked by Simon Kelley to apply onto
the preceding caching patches.
If --filter-AAAA is set and we have cached entry for
the domain in question fpr any RR type that allows us to
return a NODATA reply when --filter-AAAA is set without
going upstream. Similarly for --filter-A.
As defined in the C standard:
In all cases the argument is an int, the value of which shall
be representable as an unsigned char or shall equal the value
of the macro EOF. If the argument has any other value, the
behavior is undefined.
This is because they're designed to work with the int values returned
by getc or fgetc; they need extra work to handle a char value.
If EOF is -1 (as it almost always is), with 8-bit bytes, the allowed
inputs to the ctype(3) functions are:
{-1, 0, 1, 2, 3, ..., 255}.
However, on platforms where char is signed, such as x86 with the
usual ABI, code like
char *arg = ...;
... isspace(*arg) ...
may pass in values in the range:
{-128, -127, -126, ..., -2, -1, 0, 1, ..., 127}.
This has two problems:
1. Inputs in the set {-128, -127, -126, ..., -2} are forbidden.
2. The non-EOF byte 0xff is conflated with the value EOF = -1, so
even though the input is not forbidden, it may give the wrong
answer.
Casting char to int first before passing the result to ctype(3)
doesn't help: inputs like -128 are unchanged by this cast. It is
necessary to cast char inputs to unsigned char first; you can then
cast to int if you like but there's no need because the functions
will always convert the argument to int by definition. So the above
fragment needs to be:
char *arg = ...;
... isspace((unsigned char)*arg) ...
This patch inserts unsigned char casts where necessary, and changes
int casts to unsigned char casts where the input is char.
I left alone int casts where the input is unsigned char already --
they're not immediately harmful, although they would have the effect
of suppressing some compiler warnings if the input is ever changed to
be char instead of unsigned char, so it might be better to remove
those casts too.
I also left alone calls where the input is int to begin with because
it came from getc; casting to unsigned char here would be wrong, of
course.
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.
Try and log exactly what was returned, rather than just what
got cached. Also give validation status of RRsets if extra logging specified.
This commit also fixes a long-standing bug in caching of CNAME chains
leading to a PTR record.
Based on and inspired by a patch from Dominik DL6ER <dl6er@dl6er.de>
This fixes a problem with ipset processing that got recently introduced
when `extract_request` filtering was tightened. During the recent change
an incorrect assumption was made that `extract_request` was only called
for requests but with ipset it is also called when processing responses.
The fix ensures that the new filters only apply to requests (QR=0 @ hdr)
Signed-off-by: Etan Kissling <etan.kissling@gmail.com>
This extends query filtering support beyond what is currently possible
with the `--ipset` configuration option, by adding support for:
1) Specifying allowlists on a per-client basis, based on their
associated Linux connection track mark.
2) Dynamic configuration of allowlists via Ubus.
3) Reporting when a DNS query resolves or is rejected via Ubus.
4) DNS name patterns containing wildcards.
Disallowed queries are not forwarded; they are rejected
with a REFUSED error code.
Signed-off-by: Etan Kissling <etan_kissling@apple.com>
(addressed reviewer feedback)
Signed-off-by: Etan Kissling <etan.kissling@gmail.com>
This should be largely transparent, but it drastically
improves performance and reduces memory foot-print when
configuring large numbers domains of the form
local=/adserver.com/
or
local=/adserver.com/#
Lookup times now grow as log-to-base-2 of the number of domains,
rather than greater than linearly, as before.
The change makes multiple addresses associated with a domain work
address=/example.com/1.2.3.4
address=/example.com/5.6.7.8
It also handles multiple upstream servers for a domain better; using
the same try/retry alogrithms as non domain-specific servers. This
also applies to DNSSEC-generated queries.
Finally, some of the oldest and gnarliest code in dnsmasq has had
a significant clean-up. It's far from perfect, but it _is_ better.
