From eabf898b36c4cdad06e0379fa95ff80c3489bcd8 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Thu, 16 Jan 2020 18:50:59 +0000 Subject: [PATCH] Suppress SHA-1 DS records in dnssec-cds Previously, when dnssec-cds copied CDS records to make DS records, its -a algorithm option did not have any effect. This means that if the child zone is signed with older software that generates SHA-1 CDS records, dnssec-cds would (by default) create SHA-1 DS records in violation of RFC 8624. This change makes the dnssec-cds -a option apply to CDS records as well as CDNSKEY records. In the CDS case, the -a algorithms are the acceptable subset of possible CDS algorithms. If none of the CDS records are acceptable, dnssec-cds tries to generate DS records from CDNSKEY records. --- bin/dnssec/dnssec-cds.c | 180 ++++++++++++++++++---------------- bin/dnssec/dnssec-cds.rst | 15 ++- bin/dnssec/dnssectool.c | 2 +- bin/tests/system/cds/setup.sh | 6 ++ bin/tests/system/cds/tests.sh | 57 +++++++---- doc/man/dnssec-cds.1in | 15 ++- 6 files changed, 161 insertions(+), 114 deletions(-) diff --git a/bin/dnssec/dnssec-cds.c b/bin/dnssec/dnssec-cds.c index b58e62d78e..093b47ec55 100644 --- a/bin/dnssec/dnssec-cds.c +++ b/bin/dnssec/dnssec-cds.c @@ -124,7 +124,8 @@ typedef struct keyinfo { /* A replaceable function that can generate a DS RRset from some input */ typedef isc_result_t -ds_maker_func_t(dns_rdatalist_t *dslist, isc_buffer_t *buf, dns_rdata_t *rdata); +ds_maker_func_t(isc_buffer_t *buf, dns_rdata_t *ds, dns_dsdigest_t dt, + dns_rdata_t *crdata); static dns_rdataset_t cdnskey_set, cdnskey_sig; static dns_rdataset_t cds_set, cds_sig; @@ -724,82 +725,82 @@ signed_strict(dns_rdataset_t *dsset, dns_secalg_t *algo) { return (all_ok); } -static dns_rdata_t * -rdata_get(void) { - dns_rdata_t *rdata; - - rdata = isc_mem_get(mctx, sizeof(*rdata)); - dns_rdata_init(rdata); - - return (rdata); -} - -static isc_result_t -rdata_put(isc_result_t result, dns_rdatalist_t *rdlist, dns_rdata_t *rdata) { - if (result == ISC_R_SUCCESS) { - ISC_LIST_APPEND(rdlist->rdata, rdata, link); - } else { - isc_mem_put(mctx, rdata, sizeof(*rdata)); - } - - return (result); -} - /* * This basically copies the rdata into the buffer, but going via the - * unpacked struct has the side-effect of changing the rdatatype. The - * dns_rdata_cds_t and dns_rdata_ds_t types are aliases. + * unpacked struct lets us change the rdatatype. (The dns_rdata_cds_t + * and dns_rdata_ds_t types are aliases.) */ static isc_result_t -ds_from_cds(dns_rdatalist_t *dslist, isc_buffer_t *buf, dns_rdata_t *cds) { +ds_from_cds(isc_buffer_t *buf, dns_rdata_t *rds, dns_dsdigest_t dt, + dns_rdata_t *cds) { isc_result_t result; dns_rdata_ds_t ds; - dns_rdata_t *rdata; REQUIRE(buf != NULL); - rdata = rdata_get(); - result = dns_rdata_tostruct(cds, &ds, NULL); check_result(result, "dns_rdata_tostruct(CDS)"); ds.common.rdtype = dns_rdatatype_ds; - result = dns_rdata_fromstruct(rdata, rdclass, dns_rdatatype_ds, &ds, - buf); + if (ds.digest_type != dt) { + return (ISC_R_IGNORE); + } - return (rdata_put(result, dslist, rdata)); + return (dns_rdata_fromstruct(rds, rdclass, dns_rdatatype_ds, &ds, buf)); } static isc_result_t -ds_from_cdnskey(dns_rdatalist_t *dslist, isc_buffer_t *buf, +ds_from_cdnskey(isc_buffer_t *buf, dns_rdata_t *ds, dns_dsdigest_t dt, dns_rdata_t *cdnskey) { isc_result_t result; - unsigned i, n; + isc_region_t r; REQUIRE(buf != NULL); - n = sizeof(dtype) / sizeof(dtype[0]); - for (i = 0; i < n; i++) { - if (dtype[i] != 0) { - dns_rdata_t *rdata; - isc_region_t r; + isc_buffer_availableregion(buf, &r); + if (r.length < DNS_DS_BUFFERSIZE) { + return (ISC_R_NOSPACE); + } - isc_buffer_availableregion(buf, &r); - if (r.length < DNS_DS_BUFFERSIZE) { - return (ISC_R_NOSPACE); - } + result = dns_ds_buildrdata(name, cdnskey, dt, r.base, ds); + if (result == ISC_R_SUCCESS) { + isc_buffer_add(buf, DNS_DS_BUFFERSIZE); + } - rdata = rdata_get(); - result = dns_ds_buildrdata(name, cdnskey, dtype[i], - r.base, rdata); - if (result == ISC_R_SUCCESS) { - isc_buffer_add(buf, DNS_DS_BUFFERSIZE); - } + return (result); +} - result = rdata_put(result, dslist, rdata); - if (result != ISC_R_SUCCESS) { - return (result); - } +static isc_result_t +append_new_ds_set(ds_maker_func_t *ds_from_rdata, isc_buffer_t *buf, + dns_rdatalist_t *dslist, dns_dsdigest_t dt, + dns_rdataset_t *crdset) { + isc_result_t result; + + for (result = dns_rdataset_first(crdset); result == ISC_R_SUCCESS; + result = dns_rdataset_next(crdset)) + { + dns_rdata_t crdata = DNS_RDATA_INIT; + dns_rdata_t *ds = NULL; + + dns_rdataset_current(crdset, &crdata); + + ds = isc_mem_get(mctx, sizeof(*ds)); + dns_rdata_init(ds); + + result = ds_from_rdata(buf, ds, dt, &crdata); + + switch (result) { + case ISC_R_SUCCESS: + ISC_LIST_APPEND(dslist->rdata, ds, link); + break; + case ISC_R_IGNORE: + isc_mem_put(mctx, ds, sizeof(*ds)); + continue; + case ISC_R_NOSPACE: + isc_mem_put(mctx, ds, sizeof(*ds)); + return (result); + default: + check_result(result, "ds_from_rdata()"); } } @@ -808,14 +809,14 @@ ds_from_cdnskey(dns_rdatalist_t *dslist, isc_buffer_t *buf, static void make_new_ds_set(ds_maker_func_t *ds_from_rdata, uint32_t ttl, - dns_rdataset_t *rdset) { + dns_rdataset_t *crdset) { + isc_result_t result; + dns_rdatalist_t *dslist; unsigned int size = 16; + unsigned i, n; + for (;;) { - isc_result_t result; - dns_rdatalist_t *dslist; - dslist = isc_mem_get(mctx, sizeof(*dslist)); - dns_rdatalist_init(dslist); dslist->rdclass = rdclass; dslist->type = dns_rdatatype_ds; @@ -827,29 +828,22 @@ make_new_ds_set(ds_maker_func_t *ds_from_rdata, uint32_t ttl, isc_buffer_allocate(mctx, &new_ds_buf, size); - for (result = dns_rdataset_first(rdset); - result == ISC_R_SUCCESS; result = dns_rdataset_next(rdset)) - { - isc_result_t tresult; - dns_rdata_t rdata = DNS_RDATA_INIT; - - dns_rdataset_current(rdset, &rdata); - - tresult = ds_from_rdata(dslist, new_ds_buf, &rdata); - if (tresult == ISC_R_NOSPACE) { - vbprintf(20, "DS list buffer size %u\n", size); - freelist(&new_ds_set); - isc_buffer_free(&new_ds_buf); - size *= 2; + n = sizeof(dtype) / sizeof(dtype[0]); + for (i = 0; i < n && dtype[i] != 0; i++) { + result = append_new_ds_set(ds_from_rdata, new_ds_buf, + dslist, dtype[i], crdset); + if (result != ISC_R_SUCCESS) { break; } - - check_result(tresult, "ds_from_rdata()"); + } + if (result == ISC_R_SUCCESS) { + return; } - if (result == ISC_R_NOMORE) { - break; - } + vbprintf(2, "doubling DS list buffer size from %u\n", size); + freelist(&new_ds_set); + isc_buffer_free(&new_ds_buf); + size *= 2; } } @@ -874,8 +868,9 @@ consistent_digests(dns_rdataset_t *dsset) { int i, j, n, d; /* - * First sort the dsset. DS rdata fields are tag, algorithm, digest, - * so sorting them brings together all the records for each key. + * First sort the dsset. DS rdata fields are tag, algorithm, + * digest, so sorting them brings together all the records for + * each key. */ n = dns_rdataset_count(dsset); @@ -1093,8 +1088,8 @@ main(int argc, char *argv[]) { break; case 'i': /* - * This is a bodge to make the argument optional, - * so that it works just like sed(1). + * This is a bodge to make the argument + * optional, so that it works just like sed(1). */ if (isc_commandline_argument == argv[isc_commandline_index - 1]) { @@ -1197,9 +1192,10 @@ main(int argc, char *argv[]) { old_key_tbl = match_keyset_dsset(&dnskey_set, &old_ds_set, LOOSE); /* - * We have now identified the keys that are allowed to authenticate - * the DNSKEY RRset (RFC 4035 section 5.2 bullet 2), and CDNSKEY and - * CDS RRsets (RFC 7344 section 4.1 bullet 2). + * We have now identified the keys that are allowed to + * authenticate the DNSKEY RRset (RFC 4035 section 5.2 bullet + * 2), and CDNSKEY and CDS RRsets (RFC 7344 section 4.1 bullet + * 2). */ vbprintf(1, "verify DNSKEY signature(s)\n"); @@ -1264,6 +1260,24 @@ main(int argc, char *argv[]) { make_new_ds_set(ds_from_cdnskey, ttl, &cdnskey_set); } + /* + * Try to use CDNSKEY records if the CDS records are missing + * or did not match. + */ + if (dns_rdataset_count(&new_ds_set) == 0 && + dns_rdataset_isassociated(&cdnskey_set)) + { + vbprintf(1, "CDS records have no allowed digest types; " + "using CDNSKEY instead\n"); + freelist(&new_ds_set); + isc_buffer_free(&new_ds_buf); + make_new_ds_set(ds_from_cdnskey, ttl, &cdnskey_set); + } + if (dns_rdataset_count(&new_ds_set) == 0) { + fatal("CDS records at %s do not match any -a digest types", + namestr); + } + /* * Now we have a candidate DS RRset, we need to check it * won't break the delegation. diff --git a/bin/dnssec/dnssec-cds.rst b/bin/dnssec/dnssec-cds.rst index b49583bf45..708ecef7ed 100644 --- a/bin/dnssec/dnssec-cds.rst +++ b/bin/dnssec/dnssec-cds.rst @@ -83,14 +83,19 @@ Options ~~~~~~~ ``-a algorithm`` - This option specifies a digest algorithm to use when converting CDNSKEY records to - DS records. This option can be repeated, so that multiple DS records - are created for each CDNSKEY record. This option has no effect when - using CDS records. + When converting CDS records to DS records, this option specifies + the acceptable digest algorithms. This option can be repeated, so + that multiple digest types are allowed. If none of the CDS records + use an acceptable digest type, ``dnssec-cds`` will try to use CDNSKEY + records instead; if there are no CDNSKEY records, it reports an error. + + When converting CDNSKEY records to DS records, this option specifies the + digest algorithm to use. It can be repeated, so that multiple DS records + are created for each CDNSKEY records. The algorithm must be one of SHA-1, SHA-256, or SHA-384. These values are case-insensitive, and the hyphen may be omitted. If no algorithm - is specified, the default is SHA-256. + is specified, the default is SHA-256 only. ``-c class`` This option specifies the DNS class of the zones. diff --git a/bin/dnssec/dnssectool.c b/bin/dnssec/dnssectool.c index 5138a70062..4d931bc90c 100644 --- a/bin/dnssec/dnssectool.c +++ b/bin/dnssec/dnssectool.c @@ -62,7 +62,7 @@ static const char *keystates[KEYSTATES_NVALUES] = { int verbose = 0; bool quiet = false; -uint8_t dtype[8]; +dns_dsdigest_t dtype[8]; static fatalcallback_t *fatalcallback = NULL; diff --git a/bin/tests/system/cds/setup.sh b/bin/tests/system/cds/setup.sh index 4d528eac12..35a0e3e491 100644 --- a/bin/tests/system/cds/setup.sh +++ b/bin/tests/system/cds/setup.sh @@ -45,6 +45,8 @@ convert() { grep ' 8 1 ' DS.$n >DS.$n-1 grep ' 8 2 ' DS.$n >DS.$n-2 sed 's/ IN DS / IN CDS /' >CDS.$n + sed 's/ IN DS / IN CDS /' >CDS.$n-1 + sed 's/ IN DS / IN CDS /' >CDS.$n-2 sed 's/ IN DNSKEY / IN CDNSKEY /' <$key.key >CDNSKEY.$n sed 's/ IN DS / 3600 IN DS /' DS.ttl$n sed 's/ IN DS / 7200 IN DS /' DS.ttlong$n @@ -109,6 +111,10 @@ tac sig.cds.rev1 cat db.null CDNSKEY.2 | sign cdnskey.2 cat db.null CDS.2 CDNSKEY.2 | sign cds.cdnskey.2 +cat db.null CDS.1 CDNSKEY.2 | sign cds1.cdnskey2 + +cat db.null CDS.2-1 | sign cds.2.sha1 +cat db.null CDS.2-1 CDNSKEY.2 | sign cds.cdnskey.2.sha1 $mangle '\s+IN\s+RRSIG\s+CDS .* '$idz' '$Z'\. ' \ brk.rrsig.cds.zsk diff --git a/bin/tests/system/cds/tests.sh b/bin/tests/system/cds/tests.sh index 8344798561..49393f5d11 100644 --- a/bin/tests/system/cds/tests.sh +++ b/bin/tests/system/cds/tests.sh @@ -117,7 +117,7 @@ $CDS -v3 -s -7200 -f sig.cds.1 -d DS.1 $Z 1>xout 2>xerr testcase 0 $PERL checktime.pl 3600 xerr name='in-place reads modification time' -testcase 0 $CDS -f sig.cds.1 -i.bak -d DS.inplace $Z +testcase 0 $CDS -a1 -a2 -f sig.cds.1 -i.bak -d DS.inplace $Z name='in-place output correct modification time' testcase 0 $PERL checkmtime.pl 3600 DS.inplace @@ -134,21 +134,21 @@ testcase 0 $DIFF DS.1 DS.inplace.bak name='one mangled DS' err='found RRSIG by key' out=DS.1 -testcase 0 $CDS -v1 -s -7200 -f sig.cds.1 -d DS.broke1 $Z +testcase 0 $CDS -v1 -a1 -a2 -s -7200 -f sig.cds.1 -d DS.broke1 $Z name='other mangled DS' err='found RRSIG by key' out=DS.1 -testcase 0 $CDS -v1 -s -7200 -f sig.cds.1 -d DS.broke2 $Z +testcase 0 $CDS -v1 -a1 -a2 -s -7200 -f sig.cds.1 -d DS.broke2 $Z name='both mangled DS' err='could not validate child DNSKEY RRset' -testcase 1 $CDS -v1 -s -7200 -f sig.cds.1 -d DS.broke12 $Z +testcase 1 $CDS -v1 -a1 -a2 -s -7200 -f sig.cds.1 -d DS.broke12 $Z name='mangle RRSIG CDS by ZSK' err='found RRSIG by key' out=DS.1 -testcase 0 $CDS -v1 -s -7200 -f brk.rrsig.cds.zsk -d DS.1 $Z +testcase 0 $CDS -v1 -a1 -a2 -s -7200 -f brk.rrsig.cds.zsk -d DS.1 $Z name='mangle RRSIG CDS by KSK' err='could not validate child CDS RRset' @@ -156,11 +156,11 @@ testcase 1 $CDS -v1 -s -7200 -f brk.rrsig.cds.ksk -d DS.1 $Z name='mangle CDS 1' err='could not validate child DNSKEY RRset with new DS records' -testcase 1 $CDS -s -7200 -f sig.cds-mangled -d DS.1 $Z +testcase 1 $CDS -a1 -a2 -s -7200 -f sig.cds-mangled -d DS.1 $Z name='inconsistent digests' err='do not cover each key with the same set of digest types' -testcase 1 $CDS -s -7200 -f sig.bad-digests -d DS.1 $Z +testcase 1 $CDS -a1 -a2 -s -7200 -f sig.bad-digests -d DS.1 $Z name='inconsistent algorithms' err='missing signature for algorithm' @@ -168,49 +168,49 @@ testcase 1 $CDS -s -7200 -f sig.bad-algos -d DS.1 $Z name='add DS records' out=DS.both -$CDS -s -7200 -f sig.cds.both -d DS.1 $Z >DS.out +$CDS -a1 -a2 -s -7200 -f sig.cds.both -d DS.1 $Z >DS.out # sort to allow for numerical vs lexical order of key tags testcase 0 sort DS.out name='update add' out=UP.add2 -testcase 0 $CDS -u -s -7200 -f sig.cds.both -d DS.1 $Z +testcase 0 $CDS -a1 -a2 -u -s -7200 -f sig.cds.both -d DS.1 $Z name='remove DS records' out=DS.2 -testcase 0 $CDS -s -7200 -f sig.cds.2 -d DS.both $Z +testcase 0 $CDS -a1 -a2 -s -7200 -f sig.cds.2 -d DS.both $Z name='update del' out=UP.del1 -testcase 0 $CDS -u -s -7200 -f sig.cds.2 -d DS.both $Z +testcase 0 $CDS -a1 -a2 -u -s -7200 -f sig.cds.2 -d DS.both $Z name='swap DS records' out=DS.2 -testcase 0 $CDS -s -7200 -f sig.cds.2 -d DS.1 $Z +testcase 0 $CDS -a1 -a2 -s -7200 -f sig.cds.2 -d DS.1 $Z name='update swap' out=UP.swap -testcase 0 $CDS -u -s -7200 -f sig.cds.2 -d DS.1 $Z +testcase 0 $CDS -a1 -a2 -u -s -7200 -f sig.cds.2 -d DS.1 $Z name='TTL from -T' out=DS.ttl2 -testcase 0 $CDS -T 3600 -s -7200 -f sig.cds.2 -d DS.1 $Z +testcase 0 $CDS -a1 -a2 -T 3600 -s -7200 -f sig.cds.2 -d DS.1 $Z name='update TTL from -T' out=UP.swapttl -testcase 0 $CDS -u -T 3600 -s -7200 -f sig.cds.2 -d DS.1 $Z +testcase 0 $CDS -a1 -a2 -u -T 3600 -s -7200 -f sig.cds.2 -d DS.1 $Z name='update TTL from dsset' out=UP.swapttl -testcase 0 $CDS -u -s -7200 -f sig.cds.2 -d DS.ttl1 $Z +testcase 0 $CDS -a1 -a2 -u -s -7200 -f sig.cds.2 -d DS.ttl1 $Z name='TTL from -T overrides dsset' out=DS.ttlong2 -testcase 0 $CDS -T 7200 -s -7200 -f sig.cds.2 -d DS.ttl1 $Z +testcase 0 $CDS -a1 -a2 -T 7200 -s -7200 -f sig.cds.2 -d DS.ttl1 $Z name='stable DS record order (changes)' out=DS.1 -testcase 0 $CDS -s -7200 -f sig.cds.rev1 -d DS.2 $Z +testcase 0 $CDS -a1 -a2 -s -7200 -f sig.cds.rev1 -d DS.2 $Z name='CDNSKEY default algorithm' out=DS.2-2 @@ -230,11 +230,28 @@ testcase 0 $CDS -a SHA256 -a SHA1 -s -7200 -f sig.cdnskey.2 -d DS.1 $Z name='CDNSKEY and CDS' out=DS.2 -testcase 0 $CDS -s -7200 -f sig.cds.cdnskey.2 -d DS.1 $Z +testcase 0 $CDS -a1 -a2 -s -7200 -f sig.cds.cdnskey.2 -d DS.1 $Z name='prefer CDNSKEY' out=DS.2-2 -testcase 0 $CDS -D -s -7200 -f sig.cds.cdnskey.2 -d DS.1 $Z +testcase 0 $CDS -D -s -7200 -f sig.cds1.cdnskey2 -d DS.1 $Z + +name='CDS subset default (SHA-256)' +out=DS.2-2 +testcase 0 $CDS -s -7200 -f sig.cds.2 -d DS.1 $Z + +name='CDS subset replace SHA1 with SHA2' +out=DS.2-2 +testcase 0 $CDS -s -7200 -f sig.cds.cdnskey.2.sha1 -d DS.1 $Z + +name='CDS subset mismatch' +err='do not match any -a digest types' +testcase 1 $CDS -s -7200 -f sig.cds.2.sha1 -d DS.1 $Z + +name='CDS algorithm unavailable, use CDNSKEY' +err='using CDNSKEY instead' +out=DS.2-2 +testcase 0 $CDS -v1 -a SHA256 -s -7200 -f sig.cds.cdnskey.2.sha1 -d DS.1 $Z echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 diff --git a/doc/man/dnssec-cds.1in b/doc/man/dnssec-cds.1in index 764ccfaab9..ac07f19c1a 100644 --- a/doc/man/dnssec-cds.1in +++ b/doc/man/dnssec-cds.1in @@ -86,14 +86,19 @@ maintain a \fBdsset\-\fP file as well as emit an \fBnsupdate\fP script. .INDENT 0.0 .TP .B \fB\-a algorithm\fP -This option specifies a digest algorithm to use when converting CDNSKEY records to -DS records. This option can be repeated, so that multiple DS records -are created for each CDNSKEY record. This option has no effect when -using CDS records. +When converting CDS records to DS records, this option specifies +the acceptable digest algorithms. This option can be repeated, so +that multiple digest types are allowed. If none of the CDS records +use an acceptable digest type, \fBdnssec\-cds\fP will try to use CDNSKEY +records instead; if there are no CDNSKEY records, it reports an error. +.sp +When converting CDNSKEY records to DS records, this option specifies the +digest algorithm to use. It can be repeated, so that multiple DS records +are created for each CDNSKEY records. .sp The algorithm must be one of SHA\-1, SHA\-256, or SHA\-384. These values are case\-insensitive, and the hyphen may be omitted. If no algorithm -is specified, the default is SHA\-256. +is specified, the default is SHA\-256 only. .TP .B \fB\-c class\fP This option specifies the DNS class of the zones.