fix: dev: Fix handling of revoked keys

When a key is revoked, its key ID changes due to the inclusion of the "revoked" flag. A collision between this changed key ID
and an unrelated public-only key could cause a crash in `dnssec-signzone`.

Closes #5231

Merge branch '5231-fix-keyid-collision' into 'main'

See merge request isc-projects/bind9!10233

bin/tests/system/dnssec/signer/general/K.+013+23640.key

@@ -0,0 +1,6 @@
+; This is a key-signing key, keyid 23640, for .
+; Created: 20250310185208 (Mon Mar 10 18:52:08 2025)
+; Publish: 20250310185208 (Mon Mar 10 18:52:08 2025)
+; Activate: 20250310185208 (Mon Mar 10 18:52:08 2025)
+; Revoke: 20250310185208 (Mon Mar 10 18:52:08 2025)
+. IN DNSKEY 257 3 13 uKwpRtMH+9iuUk/Xj6LciIP5ZckaBtXaUqxUxzJYexXjvxGZGX4470Jv hq2NCI3HBZQNaCCP/h9sluhIzRGPTA==

bin/tests/system/dnssec/signer/general/K.+013+23640.private

@@ -0,0 +1,7 @@
+Private-key-format: v1.3
+Algorithm: 13 (ECDSAP256SHA256)
+PrivateKey: m5udfGNSijISQ8Tfp4kx09O1em4PErLUw/mCj3SKmqw=
+Created: 20250310185208
+Publish: 20250310185208
+Activate: 20250310185208
+Revoke: 20250310185208

bin/tests/system/dnssec/signer/general/K.+013+23768.key

@@ -0,0 +1,5 @@
+; This is a zone-signing key, keyid 23768, for .
+; Created: 20250310185208 (Mon Mar 10 18:52:08 2025)
+; Publish: 20250310185208 (Mon Mar 10 18:52:08 2025)
+; Activate: 20250310185208 (Mon Mar 10 18:52:08 2025)
+. IN DNSKEY 256 3 13 TFelYtTRBWeA9A307vvuWIcaNwW4txW4RgSELtsi46ZQs24ncRxmxtFf uJuPyVXePNiE4HNI9CIowGUsn5WuBw==

bin/tests/system/dnssec/signer/general/test12.zone

@@ -0,0 +1,37 @@
+; Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+;
+; SPDX-License-Identifier: MPL-2.0
+;
+; This Source Code Form is subject to the terms of the Mozilla Public
+; License, v. 2.0.  If a copy of the MPL was not distributed with this
+; file, you can obtain one at https://mozilla.org/MPL/2.0/.
+;
+; See the COPYRIGHT file distributed with this work for additional
+; information regarding copyright ownership.
+
+;	This is a zone which has two DNSKEY records, both of which have
+; existing private key files available.  They should be loaded automatically
+; and the zone correctly signed.
+;
+$TTL 30	; 30 seconds
+.			IN SOA	a.root.servers.nil. each.isc.org. (
+				2000042101 ; serial
+				600        ; refresh (10 minutes)
+				600        ; retry (10 minutes)
+				1200       ; expire (20 minutes)
+				600        ; minimum (10 minutes)
+				)
+			NS	a.root-servers.nil.
+			DNSKEY	256 3 13 (
+				TFelYtTRBWeA9A307vvuWIcaNwW4txW4RgSELtsi46ZQ
+				s24ncRxmxtFfuJuPyVXePNiE4HNI9CIowGUsn5WuBw==
+				) ; ZSK; alg = ECDSAP256SHA256 ; key id = 23768
+			DNSKEY	257 3 13 (
+				OSmhpULEDCUzHCBeDU5uJXzkCcGuW2qrkQznKRPGhRZN
+				j7ZUIGInGzM5Um5m02ULWt8tKbi55NJUeifKWegQ0g==
+				) ; KSK; alg = ECDSAP256SHA256 ; key id = 22255
+			DNSKEY	385 3 13 (
+				uKwpRtMH+9iuUk/Xj6LciIP5ZckaBtXaUqxUxzJYexXj
+				vxGZGX4470Jvhq2NCI3HBZQNaCCP/h9sluhIzRGPTA==
+				) ; revoked KSK; alg = ECDSAP256SHA256 ; key id = 23768
+a.root-servers.nil.	A	10.53.0.1

bin/tests/system/dnssec/tests.sh

@@ -1564,6 +1564,18 @@ n=$((n + 1))
 test "$ret" -eq 0 || echo_i "failed"
 status=$((status + ret))
 
+echo_ic "revoked KSK ID collides with ZSK ($n)"
+ret=0
+# signing should fail, but should not coredump
+(
+  cd signer/general || exit 0
+  rm -f signed.zone
+  $SIGNER -S -f signed.zone -o . test12.zone >signer.out.$n
+) && ret=1
+n=$((n + 1))
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status + ret))
+
 echo_ic "check that dnssec-signzone rejects excessive NSEC3 iterations ($n)"
 ret=0
 (

lib/dns/dnssec.c

@@ -1428,29 +1428,35 @@ addkey(dns_dnsseckeylist_t *keylist, dst_key_t **newkey, bool savekeys,
 
 	if (key != NULL) {
 		/*
-		 * Found a match.  If the old key was only public and the
+		 * Found a match. If we already had a private key, then
-		 * new key is private, replace the old one; otherwise
+		 * the new key can't be an improvement. If the existing
-		 * leave it.  But either way, mark the key as having
+		 * key was public-only but the new key is too, then it's
-		 * been found in the zone.
+		 * still not an improvement. Mark the old key as having
+		 * been found in the zone and stop.
 		 */
-		if (dst_key_isprivate(key->key)) {
+		if (dst_key_isprivate(key->key) || !dst_key_isprivate(*newkey))
-			dst_key_free(newkey);
+		{
-		} else if (dst_key_isprivate(*newkey)) {
+			key->source = dns_keysource_zoneapex;
-			dst_key_free(&key->key);
+			return;
-			key->key = *newkey;
 		}
 
-		key->source = dns_keysource_zoneapex;
+		/*
-		return;
+		 * However, if the old key was public-only, and the new key
+		 * is private, then we're throwing away the old key.
+		 */
+		dst_key_free(&key->key);
+		ISC_LIST_UNLINK(*keylist, key, link);
+		dns_dnsseckey_destroy(mctx, &key);
 	}
 
+	/* Store the new key. */
 	dns_dnsseckey_create(mctx, newkey, &key);
+	key->source = dns_keysource_zoneapex;
 	key->pubkey = pubkey_only;
 	if (key->legacy || savekeys) {
 		key->force_publish = true;
 		key->force_sign = dst_key_isprivate(key->key);
 	}
-	key->source = dns_keysource_zoneapex;
 	ISC_LIST_APPEND(*keylist, key, link);
 	*newkey = NULL;
 }