From 4e55893d307162770fbd7e0967043f0aef9d7583 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 18 Dec 2009 22:16:49 +0000 Subject: [PATCH] 2813. [bug] Better handling of unreadable DNSSEC key files. [RT #20710] 2812. [bug] Make sure updates can't result in a zone with NSEC-only keys and NSEC3 records. [RT 20748] --- CHANGES | 6 +++ bin/named/update.c | 89 ++++++++++++++++++++------------------- lib/dns/dnssec.c | 58 ++++++++++++++++++++++--- lib/dns/include/dns/log.h | 3 +- lib/dns/log.c | 3 +- lib/dns/zone.c | 79 +++++++++++++++++++++++++++++++--- 6 files changed, 179 insertions(+), 59 deletions(-) diff --git a/CHANGES b/CHANGES index 1fd1d90916..b693749408 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +2813. [bug] Better handling of unreadable DNSSEC key files. + [RT #20710] + +2812. [bug] Make sure updates can't result in a zone with + NSEC-only keys and NSEC3 records. [RT 20748] + 2811. [cleanup] Add "rndc sign" to list of commands in rndc usage output. [RT #20733] diff --git a/bin/named/update.c b/bin/named/update.c index 8dfd121dab..a832f3431d 100644 --- a/bin/named/update.c +++ b/bin/named/update.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: update.c,v 1.176 2009/12/04 21:09:32 marka Exp $ */ +/* $Id: update.c,v 1.177 2009/12/18 22:16:49 each Exp $ */ #include @@ -3034,61 +3034,62 @@ static isc_result_t check_dnssec(ns_client_t *client, dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dns_diff_t *diff) { - dns_diff_t temp_diff; - dns_diffop_t op; - dns_difftuple_t *tuple, *newtuple = NULL, *next; - isc_boolean_t flag; + dns_difftuple_t *tuple; + isc_boolean_t nseconly = ISC_FALSE, nsec3 = ISC_FALSE; isc_result_t result; unsigned int iterations = 0, max; dns_rdatatype_t privatetype = dns_zone_getprivatetype(zone); - dns_diff_init(diff->mctx, &temp_diff); + /* Scan the tuples for an NSEC-only DNSKEY or an NSEC3PARAM */ + for (tuple = ISC_LIST_HEAD(diff->tuples); + tuple != NULL; + tuple = ISC_LIST_NEXT(tuple, link)) { + if (tuple->op != DNS_DIFFOP_ADD) + continue; - CHECK(dns_nsec_nseconly(db, ver, &flag)); + if (tuple->rdata.type == dns_rdatatype_dnskey) { + isc_uint8_t alg; + alg = tuple->rdata.data[3]; + if (alg == DST_ALG_RSAMD5 || alg == DST_ALG_RSASHA1 || + alg == DST_ALG_DSA || alg == DST_ALG_ECC) { + nseconly = ISC_TRUE; + break; + } + } else if (tuple->rdata.type == dns_rdatatype_nsec3param) { + nsec3 = ISC_TRUE; + break; + } + } - if (flag) + /* Check existing DB for NSEC-only DNSKEY */ + if (!nseconly) + CHECK(dns_nsec_nseconly(db, ver, &nseconly)); + + /* Check existing DB for NSEC3 */ + if (!nsec3) CHECK(dns_nsec3_activex(db, ver, ISC_FALSE, - privatetype, &flag)); - if (flag) { - update_log(client, zone, ISC_LOG_WARNING, + privatetype, &nsec3)); + + /* Refuse to allow NSEC3 with NSEC-only keys */ + if (nseconly && nsec3) { + update_log(client, zone, ISC_LOG_ERROR, "NSEC only DNSKEYs and NSEC3 chains not allowed"); - } else { - CHECK(get_iterations(db, ver, privatetype, &iterations)); - CHECK(dns_nsec3_maxiterations(db, ver, client->mctx, &max)); - if (max != 0 && iterations > max) { - flag = ISC_TRUE; - update_log(client, zone, ISC_LOG_WARNING, - "too many NSEC3 iterations (%u) for " - "weakest DNSKEY (%u)", iterations, max); - } - } - if (flag) { - for (tuple = ISC_LIST_HEAD(diff->tuples); - tuple != NULL; - tuple = next) { - next = ISC_LIST_NEXT(tuple, link); - if (tuple->rdata.type != dns_rdatatype_dnskey && - tuple->rdata.type != dns_rdatatype_nsec3param) - continue; - op = (tuple->op == DNS_DIFFOP_DEL) ? - DNS_DIFFOP_ADD : DNS_DIFFOP_DEL; - CHECK(dns_difftuple_create(temp_diff.mctx, op, - &tuple->name, tuple->ttl, - &tuple->rdata, &newtuple)); - CHECK(do_one_tuple(&newtuple, db, ver, &temp_diff)); - INSIST(newtuple == NULL); - } - for (tuple = ISC_LIST_HEAD(temp_diff.tuples); - tuple != NULL; - tuple = ISC_LIST_HEAD(temp_diff.tuples)) { - ISC_LIST_UNLINK(temp_diff.tuples, tuple, link); - dns_diff_appendminimal(diff, &tuple); - } + result = DNS_R_REFUSED; + goto failure; } + /* Verify NSEC3 params */ + CHECK(get_iterations(db, ver, privatetype, &iterations)); + CHECK(dns_nsec3_maxiterations(db, ver, client->mctx, &max)); + if (max != 0 && iterations > max) { + update_log(client, zone, ISC_LOG_ERROR, + "too many NSEC3 iterations (%u) for " + "weakest DNSKEY (%u)", iterations, max); + result = DNS_R_REFUSED; + goto failure; + } failure: - dns_diff_clear(&temp_diff); return (result); } diff --git a/lib/dns/dnssec.c b/lib/dns/dnssec.c index d71a05a560..4920b6516f 100644 --- a/lib/dns/dnssec.c +++ b/lib/dns/dnssec.c @@ -16,7 +16,7 @@ */ /* - * $Id: dnssec.c,v 1.115 2009/11/24 23:48:12 tbox Exp $ + * $Id: dnssec.c,v 1.116 2009/12/18 22:16:49 each Exp $ */ /*! \file */ @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -663,7 +664,22 @@ dns_dnssec_findzonekeys2(dns_db_t *db, dns_dbversion_t *ver, } } - if (result == ISC_R_FILENOTFOUND) { + if (result != ISC_R_SUCCESS) { + char keybuf[DNS_NAME_FORMATSIZE]; + char algbuf[DNS_SECALG_FORMATSIZE]; + dns_name_format(dst_key_name(pubkey), keybuf, + sizeof(keybuf)); + dns_secalg_format(dst_key_alg(pubkey), algbuf, + sizeof(algbuf)); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, + DNS_LOGMODULE_DNSSEC, ISC_LOG_WARNING, + "dns_dnssec_findzonekeys2: error " + "reading private key file %s/%s/%d: %s", + keybuf, algbuf, dst_key_id(pubkey), + isc_result_totext(result)); + } + + if (result == ISC_R_FILENOTFOUND || result == ISC_R_NOPERM) { keys[count] = pubkey; pubkey = NULL; count++; @@ -1233,9 +1249,23 @@ dns_dnssec_findmatchingkeys(dns_name_t *origin, const char *directory, continue; dstkey = NULL; - RETERR(dst_key_fromnamedfile(dir.entry.name, directory, - DST_TYPE_PUBLIC | DST_TYPE_PRIVATE, - mctx, &dstkey)); + result = dst_key_fromnamedfile(dir.entry.name, + directory, + DST_TYPE_PUBLIC | + DST_TYPE_PRIVATE, + mctx, &dstkey); + + if (result != ISC_R_SUCCESS) { + isc_log_write(dns_lctx, + DNS_LOGCATEGORY_GENERAL, + DNS_LOGMODULE_DNSSEC, + ISC_LOG_WARNING, + "dns_dnssec_findmatchingkeys: " + "error reading key file %s: %s", + dir.entry.name, + isc_result_totext(result)); + continue; + } RETERR(dns_dnsseckey_create(mctx, &dstkey, &key)); key->source = dns_keysource_repository; @@ -1418,7 +1448,23 @@ dns_dnssec_keylistfromrdataset(dns_name_t *origin, dst_key_alg(pubkey), DST_TYPE_PUBLIC|DST_TYPE_PRIVATE, directory, mctx, &privkey); - if (result == ISC_R_FILENOTFOUND) { + + if (result != ISC_R_SUCCESS) { + char keybuf[DNS_NAME_FORMATSIZE]; + char algbuf[DNS_SECALG_FORMATSIZE]; + dns_name_format(dst_key_name(pubkey), keybuf, + sizeof(keybuf)); + dns_secalg_format(dst_key_alg(pubkey), algbuf, + sizeof(algbuf)); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, + DNS_LOGMODULE_DNSSEC, ISC_LOG_WARNING, + "dns_dnssec_keylistfromrdataset: error " + "reading private key file %s/%s/%d: %s", + keybuf, algbuf, dst_key_id(pubkey), + isc_result_totext(result)); + } + + if (result == ISC_R_FILENOTFOUND || result == ISC_R_NOPERM) { addkey(keylist, &pubkey, savekeys, mctx); goto skip; } diff --git a/lib/dns/include/dns/log.h b/lib/dns/include/dns/log.h index 5eac7dfb53..fef8bd32ed 100644 --- a/lib/dns/include/dns/log.h +++ b/lib/dns/include/dns/log.h @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: log.h,v 1.44 2009/01/17 23:47:43 tbox Exp $ */ +/* $Id: log.h,v 1.45 2009/12/18 22:16:49 each Exp $ */ /*! \file dns/log.h * \author Principal Authors: DCL */ @@ -73,6 +73,7 @@ LIBDNS_EXTERNAL_DATA extern isc_logmodule_t dns_modules[]; #define DNS_LOGMODULE_HINTS (&dns_modules[24]) #define DNS_LOGMODULE_ACACHE (&dns_modules[25]) #define DNS_LOGMODULE_DLZ (&dns_modules[26]) +#define DNS_LOGMODULE_DNSSEC (&dns_modules[27]) ISC_LANG_BEGINDECLS diff --git a/lib/dns/log.c b/lib/dns/log.c index 7551e15f25..9c6cd5080f 100644 --- a/lib/dns/log.c +++ b/lib/dns/log.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: log.c,v 1.45 2007/06/18 23:47:40 tbox Exp $ */ +/* $Id: log.c,v 1.46 2009/12/18 22:16:49 each Exp $ */ /*! \file */ @@ -79,6 +79,7 @@ LIBDNS_EXTERNAL_DATA isc_logmodule_t dns_modules[] = { { "dns/hints", 0 }, { "dns/acache", 0 }, { "dns/dlz", 0 }, + { "dns/dnssec", 0 }, { NULL, 0 } }; diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 84732becf6..f08b72ecb1 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: zone.c,v 1.541 2009/12/11 01:06:03 each Exp $ */ +/* $Id: zone.c,v 1.542 2009/12/18 22:16:49 each Exp $ */ /*! \file */ @@ -2544,8 +2544,8 @@ set_resigntime(dns_zone_t *zone) { dns_rdataset_init(&rdataset); dns_fixedname_init(&fixed); - result = dns_db_getsigningtime(zone->db, &rdataset, - dns_fixedname_name(&fixed)); + result = dns_db_getsigningtime(zone->db, &rdataset, + dns_fixedname_name(&fixed)); if (result != ISC_R_SUCCESS) { isc_time_settoepoch(&zone->resigntime); return; @@ -13629,6 +13629,59 @@ sign_dnskey(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dst_key_free(&zone_keys[i]); } +/* + * Prevent the zone entering a inconsistent state where + * NSEC only DNSKEYs are present with NSEC3 chains. + * See update.c:check_dnssec() + */ +static isc_boolean_t +dnskey_sane(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, + dns_diff_t *diff) +{ + isc_result_t result; + dns_difftuple_t *tuple; + isc_boolean_t nseconly = ISC_FALSE, nsec3 = ISC_FALSE; + dns_rdatatype_t privatetype = dns_zone_getprivatetype(zone); + + /* Scan the tuples for an NSEC-only DNSKEY */ + for (tuple = ISC_LIST_HEAD(diff->tuples); + tuple != NULL; + tuple = ISC_LIST_NEXT(tuple, link)) { + isc_uint8_t alg; + if (tuple->rdata.type != dns_rdatatype_dnskey || + tuple->op != DNS_DIFFOP_ADD) + continue; + + alg = tuple->rdata.data[3]; + if (alg == DST_ALG_RSAMD5 || alg == DST_ALG_RSASHA1 || + alg == DST_ALG_DSA || alg == DST_ALG_ECC) { + nseconly = ISC_TRUE; + break; + } + } + + /* Check existing DB for NSEC-only DNSKEY */ + if (!nseconly) + CHECK(dns_nsec_nseconly(db, ver, &nseconly)); + + /* Check existing DB for NSEC3 */ + if (!nsec3) + CHECK(dns_nsec3_activex(db, ver, ISC_FALSE, + privatetype, &nsec3)); + + /* Refuse to allow NSEC3 with NSEC-only keys */ + if (nseconly && nsec3) { + dns_zone_log(zone, ISC_LOG_ERROR, + "NSEC only DNSKEYs and NSEC3 chains not allowed"); + goto failure; + } + + return (ISC_TRUE); + + failure: + return (ISC_FALSE); +} + static isc_result_t zone_rekey(dns_zone_t *zone) { isc_result_t result; @@ -13689,10 +13742,22 @@ zone_rekey(dns_zone_t *zone) { isc_boolean_t check_ksk; check_ksk = DNS_ZONE_OPTION(zone, DNS_ZONEOPT_UPDATECHECKKSK); - CHECK(dns_dnssec_updatekeys(&dnskeys, &keys, &rmkeys, - &zone->origin, ttl, &diff, - ISC_TF(!check_ksk), mctx, logmsg)); - if (!ISC_LIST_EMPTY(diff.tuples)) { + result = dns_dnssec_updatekeys(&dnskeys, &keys, &rmkeys, + &zone->origin, ttl, &diff, + ISC_TF(!check_ksk), + mctx, logmsg); + + /* Keys couldn't be updated for some reason; try again later. */ + if (result != ISC_R_SUCCESS) { + dns_zone_log(zone, ISC_LOG_ERROR, "zone_rekey:" + "couldn't update zone keys: %s", + isc_result_totext(result)); + zone->refreshkeytime.seconds += HOUR; + goto failure; + } + + if (!ISC_LIST_EMPTY(diff.tuples) && + dnskey_sane(zone, db, ver, &diff)) { commit = ISC_TRUE; dns_diff_apply(&diff, db, ver); sign_dnskey(zone, db, ver, &diff);