test if nameservers with DNSSEC support return "ad"-flagged data

  DNSSEC-validating nameservers return an "ad" (Authenticated Data)
flag in the DNS response header. This can be used as a negative
indicator for DNSSEC validation: In case a nameserver does not
return the flag, but failes to look up a domain with an invalid
signature, it does not support DNSSEC validation.

This makes it easier to detect nameservers which do not fully
comply to the RFCs or try to tamper DNS queries.

See bug #11595 (https://bugzilla.ipfire.org/show_bug.cgi?id=11595) for further details.

Signed-off-by: Peter Müller <peter.mueller@link38.eu>
---
 src/initscripts/system/unbound | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
  

Hi,

basically this patch works. But I have a few issues I would like to point out
and use as a bit of an exercise for everyone.

On Sat, 2018-01-20 at 16:25 +0100, Peter Müller wrote:
> DNSSEC-validating nameservers return an "ad" (Authenticated Data)
> flag in the DNS response header. This can be used as a negative
> indicator for DNSSEC validation: In case a nameserver does not
> return the flag, but failes to look up a domain with an invalid
> signature, it does not support DNSSEC validation.
> 
> This makes it easier to detect nameservers which do not fully
> comply to the RFCs or try to tamper DNS queries.
> 
> See bug #11595 (https://bugzilla.ipfire.org/show_bug.cgi?id=11595) for further
> details.
> 
> Signed-off-by: Peter Müller <peter.mueller@link38.eu>
> ---
>  src/initscripts/system/unbound | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/initscripts/system/unbound b/src/initscripts/system/unbound
> index 4e7e63e5f..410631f86 100644
> --- a/src/initscripts/system/unbound
> +++ b/src/initscripts/system/unbound
> @@ -364,7 +364,12 @@ ns_is_validating() {
>  	local ns=${1}
>  	shift
>  
> -	dig @${ns} A ${TEST_DOMAIN_FAIL} $@ | grep -q SERVFAIL
> +	if ! dig @${ns} A ${TEST_DOMAIN_FAIL} $@ | grep -q SERVFAIL; then
> +		return 1
> +	else
> +		# Determine if NS replies with "ad" data flag if DNSSEC
> enabled
> +		dig @${ns} +dnssec SOA ${TEST_DOMAIN} $@ | grep "\;\;\
> flags:" | awk -F\: '{ print $2 }' | awk -F\; '{ print $1 }' | grep -q "\ ad"
> +	fi

a) Parsing the human-readable output of a tool is always a bad idea. It might
change or change just one character and the entire chain doesn't work any more.
Let's hope we will notice that before our users do.

b) There is no need to use grep here. awk can grep like this:

  awk /\;\; flags:/ -F: '{ print $2 }'

That will save you from calling grep here which increases performance. Using awk
and grep is also not really a good idea because every subprocess takes ages to
execute.

The network code in IPFire 3 doesn't use awk at all and grep in rare cases.

  https://git.ipfire.org/?p=network.git&a=search&h=HEAD&st=grep&s=grep
  https://git.ipfire.org/?p=network.git&a=search&h=HEAD&st=grep&s=awk

But to not complicate the code too much we can use awk here. But there is no
need for grep.

>  }
>  
>  # Checks if we can retrieve the DNSKEY for this domain.

Best,
-Michael
  

Hello,

> Hi,
> 
> basically this patch works. But I have a few issues I would like to point out
> and use as a bit of an exercise for everyone.
> 
> On Sat, 2018-01-20 at 16:25 +0100, Peter Müller wrote:
> > DNSSEC-validating nameservers return an "ad" (Authenticated Data)
> > flag in the DNS response header. This can be used as a negative
> > indicator for DNSSEC validation: In case a nameserver does not
> > return the flag, but failes to look up a domain with an invalid
> > signature, it does not support DNSSEC validation.
> > 
> > This makes it easier to detect nameservers which do not fully
> > comply to the RFCs or try to tamper DNS queries.
> > 
> > See bug #11595 (https://bugzilla.ipfire.org/show_bug.cgi?id=11595) for further
> > details.
> > 
> > Signed-off-by: Peter Müller <peter.mueller@link38.eu>
> > ---
> >  src/initscripts/system/unbound | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/initscripts/system/unbound b/src/initscripts/system/unbound
> > index 4e7e63e5f..410631f86 100644
> > --- a/src/initscripts/system/unbound
> > +++ b/src/initscripts/system/unbound
> > @@ -364,7 +364,12 @@ ns_is_validating() {
> >  	local ns=${1}
> >  	shift
> >  
> > -	dig @${ns} A ${TEST_DOMAIN_FAIL} $@ | grep -q SERVFAIL
> > +	if ! dig @${ns} A ${TEST_DOMAIN_FAIL} $@ | grep -q SERVFAIL; then
> > +		return 1
> > +	else
> > +		# Determine if NS replies with "ad" data flag if DNSSEC
> > enabled
> > +		dig @${ns} +dnssec SOA ${TEST_DOMAIN} $@ | grep "\;\;\
> > flags:" | awk -F\: '{ print $2 }' | awk -F\; '{ print $1 }' | grep -q "\ ad"
> > +	fi  
> 
> a) Parsing the human-readable output of a tool is always a bad idea. It might
> change or change just one character and the entire chain doesn't work any more.
> Let's hope we will notice that before our users do.
You are right. There seems to be no way how to just get the answer flags out of dig,
all we can do is suppressing some data parts unused here:

dig SOA +dnssec +adflag +noanswer +noquestion +nostats ipfire.org

This does not fully solve the problem, but reduces the chance that some clutter
in the actual answer data section might cause false-positives.
> 
> b) There is no need to use grep here. awk can grep like this:
> 
>   awk /\;\; flags:/ -F: '{ print $2 }'
> 
Thank you, I will send in a second patch.
> That will save you from calling grep here which increases performance. Using awk
> and grep is also not really a good idea because every subprocess takes ages to
> execute.
Agreed.
> 
> The network code in IPFire 3 doesn't use awk at all and grep in rare cases.
> 
>   https://git.ipfire.org/?p=network.git&a=search&h=HEAD&st=grep&s=grep
>   https://git.ipfire.org/?p=network.git&a=search&h=HEAD&st=grep&s=awk
Wow! I use them virtually everywhere... :-)

Best regards,
Peter Müller
> 
> But to not complicate the code too much we can use awk here. But there is no
> need for grep.
> 
> >  }
> >  
> >  # Checks if we can retrieve the DNSKEY for this domain.  
> 
> Best,
> -Michael