From 4ab8fdc0c0d0ec757c5de0ec961a3cda4d1ffdb5 Mon Sep 17 00:00:00 2001 From: Bob Halley Date: Sat, 24 Jun 2023 07:27:25 -0700 Subject: [PATCH] Fix three problems with DNSSEC: (#946) * Fix three problems with DNSSEC: 1) Signing a relative zone didn't quite work. 2) The signer generated the wrong RRSIG labels length for a wild name. 3) The validator failed to detect 2). * fix issues detected by mypy --- dns/dnssec.py | 32 +++++++++++++++++++++++----- tests/test_dnssec.py | 50 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 73 insertions(+), 9 deletions(-) diff --git a/dns/dnssec.py b/dns/dnssec.py index 13b73133..55fd7b57 100644 --- a/dns/dnssec.py +++ b/dns/dnssec.py @@ -682,6 +682,7 @@ def _sign( lifetime: Optional[int] = None, verify: bool = False, policy: Optional[Policy] = None, + origin: Optional[dns.name.Name] = None, ) -> RRSIG: """Sign RRset using private key. @@ -717,6 +718,10 @@ def _sign( *policy*, a ``dns.dnssec.Policy`` or ``None``. If ``None``, the default policy, ``dns.dnssec.default_policy`` is used; this policy defaults to that of RFC 8624. + *origin*, a ``dns.name.Name`` or ``None``. If ``None``, the default, then all + names in the rrset (including its owner name) must be absolute; otherwise the + specified origin will be used to make names absolute when signing. + Raises ``DeniedByPolicy`` if the signature is denied by policy. """ @@ -748,12 +753,22 @@ def _sign( else: raise ValueError("expiration or lifetime must be specified") + # Derelativize now because we need a correct labels length for the + # rrsig_template. + if origin is not None: + rrname = rrname.derelativize(origin) + labels = len(rrname) - 1 + + # Adjust labels appropriately for wildcards. + if rrname.is_wild(): + labels -= 1 + rrsig_template = RRSIG( rdclass=rdclass, rdtype=dns.rdatatype.RRSIG, type_covered=rdtype, algorithm=dnskey.algorithm, - labels=len(rrname) - 1, + labels=labels, original_ttl=original_ttl, expiration=rrsig_expiration, inception=rrsig_inception, @@ -762,7 +777,7 @@ def _sign( signature=b"", ) - data = dns.dnssec._make_rrsig_signature_data(rrset, rrsig_template) + data = dns.dnssec._make_rrsig_signature_data(rrset, rrsig_template, origin) chosen_hash = _make_hash(rrsig_template.algorithm) signature = None @@ -867,9 +882,12 @@ def _make_rrsig_signature_data( raise ValidationFailure("relative RR name without an origin specified") rrname = rrname.derelativize(origin) - if len(rrname) - 1 < rrsig.labels: + name_len = len(rrname) + if rrname.is_wild() and rrsig.labels != name_len - 2: + raise ValidationFailure("wild owner name has wrong label length") + if name_len - 1 < rrsig.labels: raise ValidationFailure("owner name longer than RRSIG labels") - elif rrsig.labels < len(rrname) - 1: + elif rrsig.labels < name_len - 1: suffix = rrname.split(rrsig.labels + 1)[1] rrname = dns.name.from_text("*", suffix) rrnamebuf = rrname.to_digestable() @@ -1235,6 +1253,7 @@ def default_rrset_signer( expiration: Optional[Union[datetime, str, int, float]] = None, lifetime: Optional[int] = None, policy: Optional[Policy] = None, + origin: Optional[dns.name.Name] = None, ) -> None: """Default RRset signer""" @@ -1259,6 +1278,7 @@ def default_rrset_signer( lifetime=lifetime, signer=signer, policy=policy, + origin=origin, ) txn.add(rrset.name, rrset.ttl, rrsig) @@ -1364,6 +1384,7 @@ def sign_zone( expiration=expiration, lifetime=lifetime, policy=policy, + origin=zone.origin, ) return _sign_zone_nsec(zone, _txn, _rrset_signer) @@ -1436,7 +1457,8 @@ def _sign_zone_nsec( rrset = dns.rrset.from_rdata(name, rdataset.ttl, *rdataset) rrset_signer(txn, rrset) - if last_secure: + # We need "is not None" as the empty name is False because its length is 0. + if last_secure is not None: _txn_add_nsec(txn, last_secure, name, zone.rdclass, rrsig_ttl, rrset_signer) last_secure = name diff --git a/tests/test_dnssec.py b/tests/test_dnssec.py index 3074741f..7177dcda 100644 --- a/tests/test_dnssec.py +++ b/tests/test_dnssec.py @@ -824,6 +824,26 @@ class DNSSECValidatorTestCase(unittest.TestCase): com_txt, com_txt_rrsig[0], wildcard_keys, None, wildcard_when ) + # check some bogus label lengths + a_name = dns.name.from_text("a.example.com") + a_txt = clone_rrset(wildcard_txt, a_name) + a_txt_rrsig = clone_rrset(wildcard_txt_rrsig, a_name) + bad_rrsig = a_txt_rrsig[0].replace(labels=99) + with self.assertRaises(dns.dnssec.ValidationFailure): + dns.dnssec.validate_rrsig( + a_txt, bad_rrsig, wildcard_keys, None, wildcard_when + ) + bad_rrsig = a_txt_rrsig[0].replace(labels=3) + with self.assertRaises(dns.dnssec.ValidationFailure): + dns.dnssec.validate_rrsig( + a_txt, bad_rrsig, wildcard_keys, None, wildcard_when + ) + bad_rrsig = a_txt_rrsig[0].replace(labels=1) + with self.assertRaises(dns.dnssec.ValidationFailure): + dns.dnssec.validate_rrsig( + a_txt, bad_rrsig, wildcard_keys, None, wildcard_when + ) + def testAlternateParameterFormats(self): # type: () -> None # Pass rrset and rrsigset as (name, rdataset) tuples, not rrsets rrset = (abs_soa.name, abs_soa.to_rdataset()) @@ -936,8 +956,10 @@ class DNSSECMiscTestCase(unittest.TestCase): ts = dns.dnssec.to_timestamp(441812220) self.assertEqual(ts, REFERENCE_TIMESTAMP) - def test_sign_zone(self): - zone = dns.zone.from_text(test_zone_sans_nsec, "example.", relativize=False) + def do_test_sign_zone(self, relativize): + zone = dns.zone.from_text( + test_zone_sans_nsec, "example.", relativize=relativize + ) algorithm = dns.dnssec.Algorithm.ED25519 lifetime = 3600 @@ -966,9 +988,10 @@ class DNSSECMiscTestCase(unittest.TestCase): lifetime=lifetime, ) + print(zone.to_text()) rrsigs = set( [ - (str(name), rdataset.covers) + (str(name.derelativize(zone.origin)), rdataset.covers) for (name, rdataset) in zone.iterate_rdatasets() if rdataset.rdtype == dns.rdatatype.RRSIG ] @@ -977,7 +1000,11 @@ class DNSSECMiscTestCase(unittest.TestCase): signers = set( [ - (str(name), rdataset.covers, rdataset[0].key_tag) + ( + str(name.derelativize(zone.origin)), + rdataset.covers, + rdataset[0].key_tag, + ) for (name, rdataset) in zone.iterate_rdatasets() if rdataset.rdtype == dns.rdatatype.RRSIG ] @@ -992,6 +1019,12 @@ class DNSSECMiscTestCase(unittest.TestCase): else: self.assertEqual(key_tag, dns.dnssec.key_id(zsk_dnskey)) + def test_sign_zone_absolute(self): + self.do_test_sign_zone(False) + + def test_sign_zone_relative(self): + self.do_test_sign_zone(True) + def test_sign_zone_nsec_null_signer(self): def rrset_signer( txn: dns.transaction.Transaction, @@ -1334,6 +1367,14 @@ class DNSSECSignatureTestCase(unittest.TestCase): rrset = (name, rdataset) self._test_signature(key, dns.dnssec.Algorithm.ED448, rrset) + def testSignWildRdataset(self): # type: () -> None + key = ed448.Ed448PrivateKey.generate() + name = dns.name.from_text("*.example.com") + rdataset = dns.rdataset.from_text_list("in", "a", 30, ["10.0.0.1", "10.0.0.2"]) + rrset = (name, rdataset) + rrsigset = self._test_signature(key, dns.dnssec.Algorithm.ED448, rrset) + self.assertEqual(rrsigset[0].labels, 2) + def _test_signature(self, key, algorithm, rrset, signer=None, policy=None): ttl = 60 lifetime = 3600 @@ -1358,6 +1399,7 @@ class DNSSECSignatureTestCase(unittest.TestCase): keys = {signer: dnskey_rrset} rrsigset = dns.rrset.from_rdata(rrname, ttl, rrsig) dns.dnssec.validate(rrset=rrset, rrsigset=rrsigset, keys=keys, policy=policy) + return rrsigset if __name__ == "__main__": -- 2.47.3