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

Michał Kępień michal.kepien at nask.pl
Thu Dec 17 14:31:27 GMT 2015


> 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.

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.  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.

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.

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

Hope this helps.  As always, please let me know if anything above is not
clear or my interpretation is incorrect.

-- 
Best regards,
Michał Kępień



More information about the Dnsmasq-discuss mailing list