Allow response with AD bit in resolver - nginx-dev-email-history

K

15 Jun '24

Greetings,

Here a trivial patch which allows DNS responses with enabled AD bit
from used resolver.

Index: src/core/ngx_resolver.c
--- src/core/ngx_resolver.c.orig
+++ src/core/ngx_resolver.c
@@ -1774,7 +1774,7 @@ ngx_resolver_process_response(ngx_resolver_t *r, u_cha
                    (response->nar_hi << 8) + response->nar_lo);

     /* response to a standard query */
-    if ((flags & 0xf870) != 0x8000 || (trunc && tcp)) {
+    if ((flags & 0xf850) != 0x8000 || (trunc && tcp)) {
         ngx_log_error(r->log_level, r->log, 0,
                       "invalid %s DNS response %ui fl:%04Xi",
                       tcp ? "TCP" : "UDP", ident, flags);

-- 
wbr, Kirill

share

M

Maxim

16 Jun '24

Hello!

On Sat, Jun 15, 2024 at 12:02:28PM +0100, Kirill A. Korinsky wrote:

> Greetings,
> 
> Here a trivial patch which allows DNS responses with enabled AD bit
> from used resolver.
> 
> Index: src/core/ngx_resolver.c
> --- src/core/ngx_resolver.c.orig
> +++ src/core/ngx_resolver.c
> @@ -1774,7 +1774,7 @@ ngx_resolver_process_response(ngx_resolver_t *r, u_cha
>                     (response->nar_hi << 8) + response->nar_lo);
>  
>      /* response to a standard query */
> -    if ((flags & 0xf870) != 0x8000 || (trunc && tcp)) {
> +    if ((flags & 0xf850) != 0x8000 || (trunc && tcp)) {
>          ngx_log_error(r->log_level, r->log, 0,
>                        "invalid %s DNS response %ui fl:%04Xi",
>                        tcp ? "TCP" : "UDP", ident, flags);
> 

Looks good to me, pushed with an appropriate commit log, thanks.

-- 
Maxim Dounin
http://mdounin.ru/

share

J

16 Jun '24

On Sun, 16 Jun 2024 04:29:51 +0300
Maxim Dounin <mdounin at mdounin.ru> wrote:

> Hello!
> 
> On Sat, Jun 15, 2024 at 12:02:28PM +0100, Kirill A. Korinsky wrote:
> 
> > Greetings,
> > 
> > Here a trivial patch which allows DNS responses with enabled AD bit
> > from used resolver.
> > 
> > Index: src/core/ngx_resolver.c
> > --- src/core/ngx_resolver.c.orig
> > +++ src/core/ngx_resolver.c
> > @@ -1774,7 +1774,7 @@ ngx_resolver_process_response(ngx_resolver_t *r, u_cha
> >                     (response->nar_hi << 8) + response->nar_lo);
> >  
> >      /* response to a standard query */
> > -    if ((flags & 0xf870) != 0x8000 || (trunc && tcp)) {
> > +    if ((flags & 0xf850) != 0x8000 || (trunc && tcp)) {
> >          ngx_log_error(r->log_level, r->log, 0,
> >                        "invalid %s DNS response %ui fl:%04Xi",
> >                        tcp ? "TCP" : "UDP", ident, flags);
> > 
> 
> Looks good to me, pushed with an appropriate commit log, thanks.
>

Sounds familiar :)

https://mailman.nginx.org/pipermail/nginx-devel/2022-May/YQ3MYP4VNQYWEJS3XYLPMU4HZUKS4PYF.html

share

K

Kirill

16 Jun '24

On Sun, 16 Jun 2024 02:45:15 +0100,
J Carter <jordanc.carter at outlook.com> wrote:
> 
> Sounds familiar :)
> 
> https://mailman.nginx.org/pipermail/nginx-devel/2022-May/YQ3MYP4VNQYWEJS3XYLPMU4HZUKS4PYF.html

Unfortunately, the AD bit is set by the libunbound-based resolver when it is
configured to use an upstream forwarder that also supports security.

My guess is that unbound uses itself as DNS client in this case and set such
bit to request to the upstream.

Probably it can be fixed in unbound / libunbound, but such behavior exists
right now and affects many different devices...

Thus, RFC 6840 suggested to set such bit when a request has one, but not
required, which means that current logic of libunbound RFC complaint, and
nginx is violating by rejecting such a request.

-- 
wbr, Kirill

share

J

16 Jun '24

Hello,

On Sun, 16 Jun 2024 10:07:28 +0100
Kirill A. Korinsky <kirill at korins.ky> wrote:

> On Sun, 16 Jun 2024 02:45:15 +0100,
> J Carter <jordanc.carter at outlook.com> wrote:
> > 
> > Sounds familiar :)
> > 
> > https://mailman.nginx.org/pipermail/nginx-devel/2022-May/YQ3MYP4VNQYWEJS3XYLPMU4HZUKS4PYF.html
> 
> Unfortunately, the AD bit is set by the libunbound-based resolver when it is
> configured to use an upstream forwarder that also supports security.
> 
> My guess is that unbound uses itself as DNS client in this case and set such
> bit to request to the upstream.
> 
> Probably it can be fixed in unbound / libunbound, but such behavior exists
> right now and affects many different devices...
> 
> Thus, RFC 6840 suggested to set such bit when a request has one, but not
> required, which means that current logic of libunbound RFC complaint, and
> nginx is violating by rejecting such a request.
> 

Well *I* quite agree. 

I would also suggest that as DNS functionality in nginx is strictly
limited to resolving as client in quite a simplistic fashion, and nginx
does not support DNSSEC, it makes little sense to hyper-strict about
the DNSSEC extension bits in general regardless of what is written in
the RFCs.

Perhaps it would be better if the patch linked to in my previous
response was bumped and reconsidered over your patch, as that would also
ignore incorrectly set CD bit in addition to ignoring AD bit, which
also appears to be a common issue with certain recursive resolvers.

share

K

Kirill

17 Jun '24

On Mon, 17 Jun 2024 00:21:27 +0100,
J Carter <jordanc.carter at outlook.com> wrote:
>
> Well *I* quite agree.
>
> I would also suggest that as DNS functionality in nginx is strictly
> limited to resolving as client in quite a simplistic fashion, and nginx
> does not support DNSSEC, it makes little sense to hyper-strict about
> the DNSSEC extension bits in general regardless of what is written in
> the RFCs.
>
> Perhaps it would be better if the patch linked to in my previous
> response was bumped and reconsidered over your patch, as that would also
> ignore incorrectly set CD bit in addition to ignoring AD bit, which
> also appears to be a common issue with certain recursive resolvers.

Well, the CD bit means that this response contains a response that fails
DNSSEC, but for some reason was sent back.

I've checked unbound and it returns SERVFAIL in such case, or wit no CD bit
enabled if DNSSEC validation is off.

Also, I've checked OpenBSD's unwind, which is libunbound-based, which has
the accept bogus option for forwarder to tolerate invalid DNSSEC.

Finally, I've tested a random WiFi router running dnsmasq (confirmed by
fpdns) and it also returns SERVFAIL with broken DNSSEC.

Do you have an example of zone and resolver that will set CD bit?

--
wbr, Kirill

share

J

17 Jun '24

Hello,

On Mon, 17 Jun 2024 10:22:24 +0100
Kirill A. Korinsky <kirill at korins.ky> wrote:

> On Mon, 17 Jun 2024 00:21:27 +0100,
> J Carter <jordanc.carter at outlook.com> wrote:
> >
> > Well *I* quite agree.
> >
> > I would also suggest that as DNS functionality in nginx is strictly
> > limited to resolving as client in quite a simplistic fashion, and nginx
> > does not support DNSSEC, it makes little sense to hyper-strict about
> > the DNSSEC extension bits in general regardless of what is written in
> > the RFCs.
> >
> > Perhaps it would be better if the patch linked to in my previous
> > response was bumped and reconsidered over your patch, as that would also
> > ignore incorrectly set CD bit in addition to ignoring AD bit, which
> > also appears to be a common issue with certain recursive resolvers.  
> 
> Well, the CD bit means that this response contains a response that fails
> DNSSEC, but for some reason was sent back.
> 
> I've checked unbound and it returns SERVFAIL in such case, or wit no CD bit
> enabled if DNSSEC validation is off.
> 
> Also, I've checked OpenBSD's unwind, which is libunbound-based, which has
> the accept bogus option for forwarder to tolerate invalid DNSSEC.
> 
> Finally, I've tested a random WiFi router running dnsmasq (confirmed by
> fpdns) and it also returns SERVFAIL with broken DNSSEC.
> 
> Do you have an example of zone and resolver that will set CD bit?
> 
> --
> wbr, Kirill
> _______________________________________________

It's caused by DNS Cache poisoning (either intentionally, or
unintentionally), from a recursive resolver that caches CD bit but 
does not zero it if a non dns-sec query hits that cached response.

I see unbound also has a ticket open for this:
https://github.com/NLnetLabs/unbound/issues/649

share

K

Kirill

17 Jun '24

Greetings,

On Mon, 17 Jun 2024 19:08:22 +0100,
J Carter <jordanc.carter at outlook.com> wrote:
> 
> It's caused by DNS Cache poisoning (either intentionally, or
> unintentionally), from a recursive resolver that caches CD bit but 
> does not zero it if a non dns-sec query hits that cached response.
> 
> I see unbound also has a ticket open for this:
> https://github.com/NLnetLabs/unbound/issues/649

I just tried it on my laptop where I use local libunbound-based resolver,
and I can't reproduce it:

~ $ dig +short sigfail.verteiltesysteme.net @127.0.0.1 
~ $ dig +cd +short sigfail.verteiltesysteme.net @127.0.0.1 
sigfail.rsa2048-sha256.ippacket.stream.
195.201.14.36
~ $ dig +short sigfail.verteiltesysteme.net @127.0.0.1     

Thus, I've tried unbound 1.18 and 1.20 as well with the same result.

But anyway, I suggest you offer a ptach because it can be quite painful for LB.

-- 
wbr, Kirill

share