From 252a1ae0a17317318a95d90768689a8807074623 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Mon, 19 Apr 2021 16:32:40 +0200 Subject: [PATCH 1/2] Lock kasp when looking for zone keys We should also lock kasp when reading key files, because at the same time the zone in another view may be updating the key file. --- bin/named/server.c | 6 +- lib/dns/include/dns/zone.h | 18 ++++++ lib/dns/update.c | 11 +++- lib/dns/win32/libdns.def.in | 2 + lib/dns/zone.c | 112 ++++++++++++++++++++++++++++++++---- 5 files changed, 133 insertions(+), 16 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 1bd3bb505a..4be656b1f1 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -15162,10 +15162,10 @@ named_server_dnssec(named_server_t *server, isc_lex_t *lex, CHECK(dns_db_findnode(db, origin, false, &node)); dns_db_currentversion(db, &version); /* Get keys from private key files. */ - LOCK(&kasp->lock); - result = dns_dnssec_findmatchingkeys(origin, dir, now, + dns_zone_lock_keyfiles(zone); + result = dns_dnssec_findmatchingkeys(dns_zone_getorigin(zone), dir, now, dns_zone_getmctx(zone), &keys); - UNLOCK(&kasp->lock); + dns_zone_unlock_keyfiles(zone); if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) { goto cleanup; } diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index de9ffda620..c417c25a8f 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -363,6 +363,24 @@ dns_zone_getmaxttl(dns_zone_t *zone); *\li dns_ttl_t maxttl. */ +void +dns_zone_lock_keyfiles(dns_zone_t *zone); +/*%< + * Lock associated keyfiles for this zone. + * + * Require: + *\li 'zone' to be a valid zone. + */ + +void +dns_zone_unlock_keyfiles(dns_zone_t *zone); +/*%< + * Unlock associated keyfiles for this zone. + * + * Require: + *\li 'zone' to be a valid zone. + */ + isc_result_t dns_zone_load(dns_zone_t *zone, bool newonly); diff --git a/lib/dns/update.c b/lib/dns/update.c index 6bd84823e2..71ef7dde46 100644 --- a/lib/dns/update.c +++ b/lib/dns/update.c @@ -1064,11 +1064,16 @@ find_zone_keys(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, isc_stdtime_t now; dns_dbnode_t *node = NULL; const char *directory = dns_zone_getkeydirectory(zone); + CHECK(dns_db_findnode(db, dns_db_origin(db), false, &node)); isc_stdtime_get(&now); - CHECK(dns_dnssec_findzonekeys(db, ver, node, dns_db_origin(db), - directory, now, mctx, maxkeys, keys, - nkeys)); + + dns_zone_lock_keyfiles(zone); + result = dns_dnssec_findzonekeys(db, ver, node, dns_db_origin(db), + directory, now, mctx, maxkeys, keys, + nkeys); + dns_zone_unlock_keyfiles(zone); + failure: if (node != NULL) { dns_db_detachnode(db, &node); diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 2a702765fc..2900281a72 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -1276,6 +1276,7 @@ dns_zone_keydone dns_zone_link dns_zone_load dns_zone_loadandthaw +dns_zone_lock_keyfiles dns_zone_log dns_zone_logc dns_zone_logv @@ -1376,6 +1377,7 @@ dns_zone_setzeronosoattl dns_zone_signwithkey dns_zone_synckeyzone dns_zone_unload +dns_zone_unlock_keyfiles dns_zone_verifydb dns_zonekey_iszonekey dns_zonemgr_attach diff --git a/lib/dns/zone.c b/lib/dns/zone.c index f621e15898..bea3e709b6 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -123,6 +123,19 @@ #define ID(x) dst_key_id(x) #define ALG(x) dst_key_alg(x) +/*% + * KASP flags + */ +#define KASP_LOCK(k) \ + if ((k) != NULL) { \ + LOCK((&((k)->lock))); \ + } + +#define KASP_UNLOCK(k) \ + if ((k) != NULL) { \ + UNLOCK((&((k)->lock))); \ + } + /* * Default values. */ @@ -191,6 +204,9 @@ typedef struct dns_include dns_include_t; #define ZONEDB_LOCK(l, t) RWLOCK((l), (t)) #define ZONEDB_UNLOCK(l, t) RWUNLOCK((l), (t)) +#define LOCK_KEYFILES(z) LOCK(&(z)->keyflock) +#define UNLOCK_KEYFILES(z) UNLOCK(&(z)->keyflock) + #ifdef ENABLE_AFL extern bool dns_fuzzing_resolver; #endif /* ifdef ENABLE_AFL */ @@ -199,6 +215,7 @@ struct dns_zone { /* Unlocked */ unsigned int magic; isc_mutex_t lock; + isc_mutex_t keyflock; #ifdef DNS_ZONE_CHECKLOCK bool locked; #endif /* ifdef DNS_ZONE_CHECKLOCK */ @@ -1041,6 +1058,7 @@ dns_zone_create(dns_zone_t **zonep, isc_mem_t *mctx) { zone->mctx = NULL; isc_mem_attach(mctx, &zone->mctx); isc_mutex_init(&zone->lock); + isc_mutex_init(&zone->keyflock); ZONEDB_INITLOCK(&zone->dblock); /* XXX MPA check that all elements are initialised */ #ifdef DNS_ZONE_CHECKLOCK @@ -1102,6 +1120,7 @@ free_refs: isc_refcount_destroy(&zone->erefs); isc_refcount_destroy(&zone->irefs); ZONEDB_DESTROYLOCK(&zone->dblock); + isc_mutex_destroy(&zone->keyflock); isc_mutex_destroy(&zone->lock); isc_mem_putanddetach(&zone->mctx, zone, sizeof(*zone)); return (result); @@ -1276,6 +1295,7 @@ zone_free(dns_zone_t *zone) { /* last stuff */ ZONEDB_DESTROYLOCK(&zone->dblock); + isc_mutex_destroy(&zone->keyflock); isc_mutex_destroy(&zone->lock); zone->magic = 0; isc_mem_putanddetach(&zone->mctx, zone, sizeof(*zone)); @@ -6341,6 +6361,58 @@ was_dumping(dns_zone_t *zone) { return (false); } +static void +dns__zone_lockunlock_keyfiles(dns_zone_t *zone, bool lock) { + dns_viewlist_t *vlist = NULL; + dns_view_t *v = NULL; + + REQUIRE(DNS_ZONE_VALID(zone)); + + if (zone->kasp == NULL) { + /* No need to lock, nothing is writing key files. */ + return; + } + + if (zone->view == NULL || zone->view->viewlist == NULL) { + if (lock) { + LOCK_KEYFILES(zone); + } else { + UNLOCK_KEYFILES(zone); + } + return; + } + + /* + * Also lock keyfiles for zones with the same name in a different view. + */ + vlist = zone->view->viewlist; + for (v = ISC_LIST_HEAD(*vlist); v != NULL; v = ISC_LIST_NEXT(v, link)) { + dns_zone_t *z = NULL; + isc_result_t ret = dns_view_findzone(v, &zone->origin, &z); + if (ret == ISC_R_SUCCESS) { + INSIST(DNS_ZONE_VALID(z)); + + /* WMM check if policy is the same? */ + if (lock) { + LOCK_KEYFILES(z); + } else { + UNLOCK_KEYFILES(z); + } + dns_zone_detach(&z); + } + } +} + +void +dns_zone_lock_keyfiles(dns_zone_t *zone) { + dns__zone_lockunlock_keyfiles(zone, true); +} + +void +dns_zone_unlock_keyfiles(dns_zone_t *zone) { + dns__zone_lockunlock_keyfiles(zone, false); +} + /*% * Find up to 'maxkeys' DNSSEC keys used for signing version 'ver' of database * 'db' for zone 'zone' in its key directory, then load these keys into 'keys'. @@ -6357,13 +6429,21 @@ dns__zone_findkeys(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, CHECK(dns_db_findnode(db, dns_db_origin(db), false, &node)); memset(keys, 0, sizeof(*keys) * maxkeys); + + dns_zone_lock_keyfiles(zone); + result = dns_dnssec_findzonekeys(db, ver, node, dns_db_origin(db), directory, now, mctx, maxkeys, keys, nkeys); + + dns_zone_unlock_keyfiles(zone); + if (result == ISC_R_NOTFOUND) { result = ISC_R_SUCCESS; } + failure: + if (node != NULL) { dns_db_detachnode(db, &node); } @@ -7303,7 +7383,7 @@ signed_with_good_key(dns_zone_t *zone, dns_db_t *db, dns_dbnode_t *node, int zsk_count = 0; bool approved; - LOCK(&kasp->lock); + KASP_LOCK(kasp); for (kkey = ISC_LIST_HEAD(dns_kasp_keys(kasp)); kkey != NULL; kkey = ISC_LIST_NEXT(kkey, link)) { @@ -7314,7 +7394,7 @@ signed_with_good_key(dns_zone_t *zone, dns_db_t *db, dns_dbnode_t *node, zsk_count++; } } - UNLOCK(&kasp->lock); + KASP_UNLOCK(kasp); if (type == dns_rdatatype_dnskey || type == dns_rdatatype_cdnskey || type == dns_rdatatype_cds) @@ -19988,6 +20068,8 @@ zone_rekey(dns_zone_t *zone) { TIME_NOW(&timenow); now = isc_time_seconds(&timenow); + kasp = dns_zone_getkasp(zone); + dnssec_log(zone, ISC_LOG_INFO, "reconfiguring zone keys"); /* Get the SOA record's TTL */ @@ -20001,9 +20083,18 @@ zone_rekey(dns_zone_t *zone) { dns_rdatatype_none, 0, &keyset, &keysigs); if (result == ISC_R_SUCCESS) { ttl = keyset.ttl; - CHECK(dns_dnssec_keylistfromrdataset( + + dns_zone_lock_keyfiles(zone); + + result = dns_dnssec_keylistfromrdataset( &zone->origin, dir, mctx, &keyset, &keysigs, &soasigs, - false, false, &dnskeys)); + false, false, &dnskeys); + + dns_zone_unlock_keyfiles(zone); + + if (result != ISC_R_SUCCESS) { + goto failure; + } } else if (result != ISC_R_NOTFOUND) { goto failure; } @@ -20028,10 +20119,8 @@ zone_rekey(dns_zone_t *zone) { */ fullsign = DNS_ZONEKEY_OPTION(zone, DNS_ZONEKEY_FULLSIGN); - kasp = dns_zone_getkasp(zone); - if (kasp != NULL) { - LOCK(&kasp->lock); - } + KASP_LOCK(kasp); + dns_zone_lock_keyfiles(zone); result = dns_dnssec_findmatchingkeys(&zone->origin, dir, now, mctx, &keys); @@ -20051,13 +20140,16 @@ zone_rekey(dns_zone_t *zone) { "zone_rekey:dns_dnssec_keymgr " "failed: %s", isc_result_totext(result)); - UNLOCK(&kasp->lock); + dns_zone_unlock_keyfiles(zone); + KASP_UNLOCK(kasp); goto failure; } } - UNLOCK(&kasp->lock); } + dns_zone_unlock_keyfiles(zone); + KASP_UNLOCK(kasp); + if (result == ISC_R_SUCCESS) { bool cds_delete = false; isc_stdtime_t when; From fa1cd0a1f19d457b8436c6dfb31926e0f45dfa56 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Fri, 16 Apr 2021 08:39:40 +0200 Subject: [PATCH 2/2] Release notes and changes for [#1875] --- CHANGES | 3 +++ doc/notes/notes-current.rst | 3 +++ 2 files changed, 6 insertions(+) diff --git a/CHANGES b/CHANGES index 90418068b2..f6bf029d52 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5644. [bug] Fix a race condition in reading and writing key files + for KASP zones in multiple views. [GL #1875] + 5643. [placeholder] 5642. [bug] Check "key-directory" conflicts in "named.conf" for diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index a1272fb24a..71368b7ba7 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -70,6 +70,9 @@ Feature Changes Bug Fixes ~~~~~~~~~ +- Fix a race condition in reading and writing key files for KASP zones in + multiple views. :gl:`#1875` + - When dumping the cache to file, TTLs were being increased with ``max-stale-ttl``. Also the comment above stale RRsets could have nonsensical values if the RRset was still marked a stale but the ``max-stale-ttl`` has