diff --git a/CHANGES b/CHANGES index 1ef3b55bf8..79790b05a9 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +5446. [bug] The validator could fail to accept a properly signed + RRset if an unsupported algorithm appeared earlier in + the DNSKEY RRset than a supported algorithm. It could + also stop if it detected a malformed public key. + [GL #1689] + 5445. [cleanup] Disable and disallow static linking. [GL #1933] 5444. [bug] 'rndc dnstap -roll ' was not limiting the diff --git a/bin/tests/system/dnssec/tests.sh b/bin/tests/system/dnssec/tests.sh index 84e4ad627b..562be71122 100644 --- a/bin/tests/system/dnssec/tests.sh +++ b/bin/tests/system/dnssec/tests.sh @@ -3562,6 +3562,13 @@ n=$((n+1)) test "$ret" -eq 0 || echo_i "failed" status=$((status+ret)) +# TODO: test case for GL #1689. +# If we allow the dnssec tools to use deprecated algorithms (such as RSAMD5) +# we could write a test that signs a zone with supported and unsupported +# algorithm, apply a fixed rrset order such that the unsupported algorithm +# precedes the supported one in the DNSKEY RRset, and verify the result still +# validates succesfully. + echo_i "check that a lone non matching CDNSKEY record is rejected ($n)" ret=0 ( diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 8c6e86a3b9..ce8ddc72b4 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -48,3 +48,8 @@ Bug Fixes - 'rndc dnstap -roll ' was not limiting the number of saved files to . [GL !3728] + +- The validator could fail to accept a properly signed RRset if an + unsupported algorithm appeared earlier in the DNSKEY RRset than a + supported algorithm. It could also stop if it detected a malformed + public key. [GL #1689] diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 9b5c891f94..e31ad661e5 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1126,24 +1126,25 @@ select_signing_key(dns_validator_t *val, dns_rdataset_t *rdataset) { INSIST(val->key == NULL); result = dst_key_fromdns(&siginfo->signer, rdata.rdclass, &b, val->view->mctx, &val->key); - if (result != ISC_R_SUCCESS) { - goto failure; - } - if (siginfo->algorithm == (dns_secalg_t)dst_key_alg(val->key) && - siginfo->keyid == (dns_keytag_t)dst_key_id(val->key) && - dst_key_iszonekey(val->key)) - { - if (foundold) { - /* - * This is the key we're looking for. - */ - return (ISC_R_SUCCESS); - } else if (dst_key_compare(oldkey, val->key)) { - foundold = true; - dst_key_free(&oldkey); + if (result == ISC_R_SUCCESS) { + if (siginfo->algorithm == + (dns_secalg_t)dst_key_alg(val->key) && + siginfo->keyid == + (dns_keytag_t)dst_key_id(val->key) && + dst_key_iszonekey(val->key)) + { + if (foundold) { + /* + * This is the key we're looking for. + */ + return (ISC_R_SUCCESS); + } else if (dst_key_compare(oldkey, val->key)) { + foundold = true; + dst_key_free(&oldkey); + } } + dst_key_free(&val->key); } - dst_key_free(&val->key); dns_rdata_reset(&rdata); result = dns_rdataset_next(rdataset); } while (result == ISC_R_SUCCESS);