[Dnsmasq-discuss] [PATCH] Treat records signed using unknown algorithms as unsigned instead of bogus

[Simon Kelley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 17/12/15 14:31, Michał Kępień wrote:
>> OK. Fixing this turned into a marathon re-write session. The
>> result is a huge improvement: by doing the core things right I've
>> vastly simplified the code and made it much easier to understand
>> and modify.
>> 
>> The final patch, to make a zone which has a valid key of an 
>> unsupported algorithm count as insecure, even if also has a key
>> with a supported algorithm which is validated by the DS record,
>> Just went in. It's gratifyingly small and simple.
>> 
>> All the new code is in the git repo. Please test and play.
> 
> Thank you for your work.  I did some testing and, unfortunately,
> found numerous issues with the latest master.  I'll describe them
> below, ordered by decreasing importance (IMHO of course).
> 
> First and foremost, the primary issue of returning SERVFAILs for
> zones with DNSKEY RRsets signed using _only_ unknown algorithms
> (even though we are able to hash the DNSKEY RRset and authenticate
> the DS at the parent) is not resolved.  Quick example: if you try
> to resolve anything within a zone signed using only ECC-GOST keys,
> with an SHA-256 DS at the parent, SERVFAIL is still returned as
> dnssec_validate_by_ds() still requires the validate_rrset() call
> for DNSKEY RRset to succeed for it to return anything else than
> STAT_BOGUS.  In my patch which started this thread I tried to
> demonstrate that if all DNSKEY RRset validations fail only due to
> the lack of support for the _signing_ algorithms used by its 
> RRSIGs, the zone should be marked as insecure, not bogus.


Did you test this on a real domain and see a failure? The way it's
intended to work is that the call to zone_status() at line 2042 will
work down from the root to the DS, where it will find that the SHA-256
DS covers a ECC-GOST key and return STAT_INSECURE at line 1869. That
will be returned from dnssec_validate_reply at line 2049, before the
call to validate_rrset is even made. dnssec_validate_by_ds() should
never be called.

An example of a domain that fails here would be really useful. I did
testing by removing algorithms but as there are no rare algorithms
it's difficult not to cause early failure of the process before the
test case is reached.

> 
> Then there's commit 2dbba34, whose log message says: "A zone which
> has at least one key with an algorithm we don't support should be
> considered as insecure."


> That's not right.  Only a zone whose _all_ keys use algorithms we
> don't support should be considered as insecure.

No, a zone may have a key we support but records are signed only using
the key we don't support.

If a zone has a key of a type we don't understand, than we must accept
an insecure answer, (one signed only with the unsupported algorithm)
so an insecure result is legitimate for that zone. In the case that
the records are actually signed by an algorithm we do support, a
secure result is possible and an insecure response is sub-optimal, but
it's not wrong.

> AFAICT, the implementation follows the commit message, i.e. if 
> algo_digest_name() returns NULL for any of the DNSKEY RRs cached
> for this zone, zone_status() will immediately return STAT_INSECURE.
> But why would it?  Even if one DNSKEY RR uses a weird algorithm,
> others may use algorithms we understand.
> 
That's correct.


> Next up, there's a nasty bug in dnssec_validate_ds() caused by
> improper closing brace placement:
> 
> if (!neganswer) { cache_start_insert();
> 
> for (i = 0; i < ntohs(header->ancount); i++) { if (!(rc =
> extract_name(header, plen, &p, name, 0, 10))) return STAT_BOGUS; /*
> bad packet */
> 
> GETSHORT(atype, p); GETSHORT(aclass, p); GETLONG(ttl, p); 
> GETSHORT(rdlen, p);
> 
> if (!CHECK_LEN(header, p, plen, rdlen)) return STAT_BOGUS; /* bad
> packet */
> 
> if (aclass == class && atype == T_DS && rc == 1) { int algo,
> digest, keytag; unsigned char *psave = p; ... p = psave;
> 
> if (!ADD_RDLEN(header, p, plen, rdlen)) return STAT_BOGUS; /* bad
> packet */ }
> 
> cache_end_insert(); }
> 
> }
> 
> The closing brace present after cache_end_insert() should be moved
> just below the line which takes care of restoring p to a saved
> value. Otherwise, two things happen:
> 
> * DS records with multiple RRSIGs returned in the answer will be 
> considered bogus due to the GETx() calls parsing rubbish (first 
> RRSIG's RDATA) in the third iteration of the for loop,
> 
> * only the first DS RR for a zone will be cached, which causes
> issues for zones with more than one DS RR at the parent.
> 
> Patch follows (whitespace not corrected for clarity):
> 
> ----------------------------------------------------------------------
- --
>
> 
diff --git a/src/dnssec.c b/src/dnssec.c
> index dc563e0..b8630d7 100644 --- a/src/dnssec.c +++
> b/src/dnssec.c @@ -1224,13 +1224,13 @@ int
> dnssec_validate_ds(time_t now, struct dns_header *header, size_t
> plen, char }  p = psave; +	}  if (!ADD_RDLEN(header, p, plen,
> rdlen)) return STAT_BOGUS; /* bad packet */ }  cache_end_insert(); 
> -	} } else { 
> ----------------------------------------------------------------------
- --
>
>  While debugging the latest code, I also noticed that the
> algorithm support checks seem inconsistent.  AFAICT, checking
> whether a given signing algorithm is supported is done using
> algo_digest_name().  What that function actually checks is whether
> the hashing algorithm used by the given signing scheme is
> supported, not whether the actual signing scheme is supported.
> Let's compare the behavior for ECC-GOST and ECDSA. For ECC-GOST,
> algo_digest_name() will always return "gosthash94", which the
> caller interprets as "this signing scheme is supported".  However, 
> verify() will always return 0 for algorithm 12, because ECC-GOST
> signing isn't actually implemented.  Assume now that dnsmasq was
> compiled with -DNO_NETTLE_ECC.  If you ask algo_digest_name() about
> ECDSA support, it will immediately tell the caller that "this
> signing scheme is _not_ supported", even though libnettle wouldn't
> have problems with calculating a SHA-256/SHA-384 hash.  This is not
> that big of an issue, but it hindered my debugging efforts.

That's a good point. The problem is that Nettle supports introspection
for hash functions, but not public-key signatures. algo_digest_name()
uses the introspection, but it doesn't tell you that algorithms 13 and
14 are not available because ECC is not available. Hence I added the
#ifdef NO_NETTLE_ECC, which should encompass 12. Even that's not
right, because 12 needs ECC-GOST, which isn't implemented. The
canonical place for this information is actually verify(): it knows
which algorithms are OK. I've fixed the code to do that.  A digest is
supported if it's in the switch in algo_digest_name() AND supported by
the current Nettle. A signature algo is supported if it's in the
switch in verify_func(). That has to be kept up-to-date with Nettle's
capabilities.


> 
> Finally, you used STAT_NEED_DNSKEY in the comments while the actual
> name of that constant in src/dnsmasq.h is STAT_NEED_KEY.
> 

Fixed, thanks.

I pushed these changes. I'm looking at how to determine a secure
answer when a zone may be insecure through use of an unsupported
algorithm.


Cheers,

Simon.

> Hope this helps.  As always, please let me know if anything above
> is not clear or my interpretation is incorrect.
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBCAAGBQJWcvJ+AAoJEBXN2mrhkTWiPz8P/1LN18KRqZwoNIc63pD0eLjg
uxD+c4M/KKm0F7eZEsn95y7OFhaGQqVIwQjm97oAt21B5n/uT0BPb4B79TT4HzOH
jSI20zOOuVwMgSrE6EI6JB1dkjnyhokmdxWXBKejz6RPTDX1qlz5ecTYYvhs3jns
PVyvbVLOu4JefXMml6rOnxabRQiBkE5CL/Dzh0ntGBrUl3JGrZBTUGxmBebd74Qb
aToOdev6OtlsLUZHEAQF3ATEVyoFlbsEh0csQl+GMFwPfXvk381rUAswHdxwW+2G
bWl6eri+6lu1A/PZQ9VJUgdTz4JUdzIgpqSSYX/e+suyZgZUbTXa0deBROVFiOKj
NDHqxaXyZU7oaQwlYplF8v5T+62Ue5HoJL9rTlniXmZRNViGuejdaDbz3UJqD2Kg
2UfW8Xy+/mMX9HDuzVXQXaTI0bDJDh2lppZuqCzCwaMi+V2X+9c/Qvcfo3W03WOD
yzvOwAjsFDhCBORxMuZTL0a+jZkXXbKJah7A8qJzrfhq0G0x/71gTXaliPGAu8Jq
+6evRpk6LJU1mGeTMWfolqKbmvWD+77h7FjDrzKy87f3KMz0jJBeJamlRiVB535F
tvM+z2RTHdA7ry9HskKvFJn67KiuE/eTOjfZZo1X11pddyT6G5W6+AHcFwxJPLdL
wb2I+SlulAYEjUov/YF2
=GJ9S
-----END PGP SIGNATURE-----