Merge branch '1875-kasp-views-keyfile-race' into 'main'

Lock key files when looking for zone keys

Closes #1875

See merge request isc-projects/bind9!4919

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

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;
 	}

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

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);
 

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);

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

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;