[Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
Hey Simon,
the current dnsmasq master version contains a bug rewriting all
NXDOMAIN replies from upstream with NOERROR.
The error has been introduced in commit
d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (see attached diff) and is
ultimately caused by
> lookup_domain(daemon->namebuff, F_CONFIG, NULL, NULL)
at line 668/669 returning 1 when it shouldn't.
How to reproduce:
1. Start dnsmasq
2. Query a non-existing domain such as "google.comxxx".
dnsmasq 6860cf932baeaf1c2f09c2a58e38be189ae394de (and older) replies
with NXDOMAN (as expected)
dnsmasq d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (and newer) replies
incorrectly with NOERROR and sets the AA bit.
Let me know if you need any further information.
Best regards,
Dominik
diff --git a/src/domain-match.c b/src/domain-match.c
index 85ce76e..f7bb506 100644
--- a/src/domain-match.c
+++ b/src/domain-match.c
@@ -75,6 +75,8 @@ void build_server_array(void)
A flag of F_DNSSECOK returns a DNSSEC capable server only and
also disables NODOTS servers from consideration.
A flag of F_DOMAINSRV returns a domain-specific server only.
+ A flag of F_CONFIG returns anything that generates a local
+ reply of IPv4 or IPV6.
return 0 if nothing found, 1 otherwise.
*/
int lookup_domain(char *qdomain, int flags, int *lowout, int *highout)
@@ -85,8 +87,6 @@ int lookup_domain(char *qdomain, int flags, int *lowout, int *highout)
int nlow = 0, nhigh = 0;
char *cp;
- int compares = 0;
-
/* may be no configured servers. */
if (daemon->serverarraysz == 0)
return 0;
@@ -121,8 +121,6 @@ int lookup_domain(char *qdomain, int flags, int *lowout, int *highout)
{
try = (low + high)/2;
- compares++;
-
if ((rc = order(qdomain, leading_dot, qlen, daemon->serverarray[try])) == 0)
break;
@@ -186,8 +184,6 @@ int lookup_domain(char *qdomain, int flags, int *lowout, int *highout)
qdomain += crop_query;
}
- printf("compares: %d\n", compares);
-
/* domain has no dots, and we have at least one server configured to handle such,
These servers always sort to the very end of the array.
A configured server eg server=/lan/ will take precdence. */
@@ -240,47 +236,56 @@ int filter_servers(int seed, int flags, int *lowout, int *highout)
See which of those match our query in that priority order and narrow (low, high) */
- for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_6ADDR); i++);
-
- if (i != nlow && (flags & F_IPV6))
+#define SERV_LOCAL_ADDRESS (SERV_6ADDR | SERV_4ADDR | SERV_ALL_ZEROS)
+
+ for (i = nlow; (flags & F_CONFIG) && i < nhigh && (daemon->serverarray[i]->flags & SERV_LOCAL_ADDRESS); i++);
+
+ if (i != nlow)
nhigh = i;
else
{
- nlow = i;
-
- for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_4ADDR); i++);
+ for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_6ADDR); i++);
- if (i != nlow && (flags & F_IPV4))
+ if (i != nlow && (flags & F_IPV6))
nhigh = i;
else
{
nlow = i;
- for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_ALL_ZEROS); i++);
+ for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_4ADDR); i++);
- if (i != nlow && (flags & (F_IPV4 | F_IPV6)))
+ if (i != nlow && (flags & F_IPV4))
nhigh = i;
else
{
nlow = i;
- for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_LITERAL_ADDRESS); i++);
+ for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_ALL_ZEROS); i++);
- /* --local=/domain/, only return if we don't need a server. */
- if (i != nlow && !(flags & (F_DNSSECOK | F_DOMAINSRV | F_SERVER)))
+ if (i != nlow && (flags & (F_IPV4 | F_IPV6)))
nhigh = i;
else
{
nlow = i;
- /* If we want a server that can do DNSSEC, and this one can't,
- return nothing. */
- if ((flags & F_DNSSECOK) && !(daemon->serverarray[nlow]->flags & SERV_DO_DNSSEC))
- nlow = nhigh;
+
+ for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_LITERAL_ADDRESS); i++);
+
+ /* --local=/domain/, only return if we don't need a server. */
+ if (i != nlow && !(flags & (F_DNSSECOK | F_DOMAINSRV | F_SERVER)))
+ nhigh = i;
+ else
+ {
+ nlow = i;
+ /* If we want a server that can do DNSSEC, and this one can't,
+ return nothing. */
+ if ((flags & F_DNSSECOK) && !(daemon->serverarray[nlow]->flags & SERV_DO_DNSSEC))
+ nlow = nhigh;
+ }
}
}
}
}
-
+
*lowout = nlow;
*highout = nhigh;
@@ -291,7 +296,7 @@ int is_local_answer(time_t now, int first, char *name)
{
int flags = 0;
int rc = 0;
-
+
if ((flags = daemon->serverarray[first]->flags) & SERV_LITERAL_ADDRESS)
{
if (flags & SERV_4ADDR)
@@ -301,7 +306,19 @@ int is_local_answer(time_t now, int first, char *name)
else if (flags & SERV_ALL_ZEROS)
rc = F_IPV4
Re: [Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
> On 5 Jul 2021, at 16:53, Dominik DL6ER wrote: > > Hey Simon, > > the current dnsmasq master version contains a bug rewriting all > NXDOMAIN replies from upstream with NOERROR. > > The error has been introduced in commit > d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (see attached diff) and is > ultimately caused by Oooh what fun! :-) Attached patch fixes for me Cheers, Kevin D-B gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A 0001-fix-lookups.patch Description: Binary data signature.asc Description: Message signed with OpenPGP ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
On 05/07/2021 19:31, Kevin Darbyshire-Bryant wrote: > > >> On 5 Jul 2021, at 16:53, Dominik DL6ER wrote: >> >> Hey Simon, >> >> the current dnsmasq master version contains a bug rewriting all >> NXDOMAIN replies from upstream with NOERROR. >> >> The error has been introduced in commit >> d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (see attached diff) and is >> ultimately caused by > > Oooh what fun! :-) > > Attached patch fixes for me > > That's not the correct fix. check_for_local_domain() returns true for names which are handled and looked up in the cache, from /etc/hosts, or configuration, like --host-record. lookup_domain(..., F_CONFIG, .) checks for addresses associated with whole domains, so if the query is www.example.com and there exists --address=/example.com/1.2.3.4 so OR is the correct conjunction. The problem was that the new code in lookup_domain() got the wrong answer sometimes. df25f204ba822c9c00bc9372c85da58e9aff6e86 fixes. Cheers, Simon. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
On 05/07/2021 16:53, Dominik DL6ER wrote: > Hey Simon, > > the current dnsmasq master version contains a bug rewriting all > NXDOMAIN replies from upstream with NOERROR. > > The error has been introduced in commit > d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (see attached diff) and is > ultimately caused by > >> lookup_domain(daemon->namebuff, F_CONFIG, NULL, NULL) > > at line 668/669 returning 1 when it shouldn't. > > How to reproduce: > > 1. Start dnsmasq > 2. Query a non-existing domain such as "google.comxxx". > > dnsmasq 6860cf932baeaf1c2f09c2a58e38be189ae394de (and older) replies > with NXDOMAN (as expected) > dnsmasq d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (and newer) replies > incorrectly with NOERROR and sets the AA bit. > > Let me know if you need any further information. > Nice catch, thanks. I believe that df25f204ba822c9c00bc9372c85da58e9aff6e86 should fix this. Cheers, Simon. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
> On 5 Jul 2021, at 21:12, Simon Kelley wrote: > > On 05/07/2021 19:31, Kevin Darbyshire-Bryant wrote: >> >> >>> On 5 Jul 2021, at 16:53, Dominik DL6ER wrote: >>> >>> Hey Simon, >>> >>> the current dnsmasq master version contains a bug rewriting all >>> NXDOMAIN replies from upstream with NOERROR. >>> >>> The error has been introduced in commit >>> d0ae3f5a4dc094e8fe2a3c607028c1c59f42f473 (see attached diff) and is >>> ultimately caused by >> >> Oooh what fun! :-) >> >> Attached patch fixes for me >> >> > > That's not the correct fix. Goes to show how much I really know :-D At least I got one fix right. Cheers, Kevin D-B gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A signature.asc Description: Message signed with OpenPGP ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
On Mon, 2021-07-05 at 21:06 +0100, Simon Kelley wrote: > Nice catch, thanks. I believe that > df25f204ba822c9c00bc9372c85da58e9aff6e86 > should fix this. Looks good. We've started a Pi-hole beta testing round which uses the tip of dnsmasq's master branch. Hereby we will have many more users testing dnsmasq before the next release. Since we don't collect any metrics, I cannot say how many contribute to our testing round but from the current feedback it seems at least a dozen additional bleeding edge testers are onboard right now. I'll come back to you in case we discover any further issues. Best, Dominik ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [BUG] dnsmasq rewriting NXDOMAIN to NOERROR
On 06/07/2021 13:39, Dominik DL6ER wrote: > On Mon, 2021-07-05 at 21:06 +0100, Simon Kelley wrote: >> Nice catch, thanks. I believe that >> df25f204ba822c9c00bc9372c85da58e9aff6e86 >> should fix this. > > Looks good. We've started a Pi-hole beta testing round which uses the > tip of dnsmasq's master branch. Hereby we will have many more users > testing dnsmasq before the next release. Since we don't collect any > metrics, I cannot say how many contribute to our testing round but from > the current feedback it seems at least a dozen additional bleeding edge > testers are onboard right now. > > I'll come back to you in case we discover any further issues. > > Best, > Dominik > > Excellent. More testing is very useful. Simon. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
