From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 7 Jan 2021 11:12:46 +0000 (+0100)
Subject: Fix DS/DNSKEY hidden or chained functions
X-Git-Tag: v9.17.10~5^2~4
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=291bcc37217a7d375926921199c1acc8f2e92109;p=thirdparty%2Fbind9.git

Fix DS/DNSKEY hidden or chained functions

There was a bug in function 'keymgr_ds_hidden_or_chained()'.

The funcion 'keymgr_ds_hidden_or_chained()' implements (3e) of rule2
as defined in the "Flexible and Robust Key Rollover" paper. The rules
says: All DS records need to be in the HIDDEN state, or if it is not
there must be a key with its DNSKEY and KRRSIG in OMNIPRESENT, and
its DS in the same state as the key in question. In human langauge,
if all keys have their DS in HIDDEN state you can do what you want,
but if a DS record is available to some validators, there must be
a chain of trust for it.

Note that the barriers on transitions first check if the current
state is valid, and then if the next state is valid too. But
here we falsely updated the 'dnskey_omnipresent' (now 'dnskey_chained')
with the next state. The next state applies to 'key' not to the state
to be checked. Updating the state here leads to (true) always, because
the key that will move its state will match the falsely updated
expected state. This could lead to the assumption that Key 2 would be
a valid chain of trust for Key 1, while clearly the presence of any
DS is uncertain.

The fix here is to check if the DNSKEY and KRRSIG are in OMNIPRESENT
state for the key that does not have its DS in the HIDDEN state, and
only if that is not the case, ensure that there is a key with the same
algorithm, that provides a valid chain of trust, that is, has its
DNSKEY, KRRSIG, and DS in OMNIPRESENT state.

The changes in 'keymgr_dnskey_hidden_or_chained()' are only cosmetical,
renaming 'rrsig_omnipresent' to 'rrsig_chained' and removing the
redundant initialization of the DST_KEY_DNSKEY expected state to NA.
---

diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index 77fa8846730..076eb40e4b0 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -770,8 +770,8 @@ static bool
 keymgr_ds_hidden_or_chained(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key,
 			    int type, dst_key_state_t next_state,
 			    bool match_algorithms, bool must_be_hidden) {
-	dst_key_state_t dnskey_omnipresent[4] = { OMNIPRESENT, NA, OMNIPRESENT,
-						  NA };	       /* (3e) */
+	dst_key_state_t dnskey_chained[4] = { OMNIPRESENT, NA, OMNIPRESENT,
+					      NA };	       /* (3e) */
 	dst_key_state_t ds_hidden[4] = { NA, NA, NA, HIDDEN }; /* (3e) */
 	dst_key_state_t na[4] = { NA, NA, NA, NA }; /* successor n/a */
 
@@ -801,18 +801,20 @@ keymgr_ds_hidden_or_chained(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key,
 		 * least one key with the same algorithm that provides a
 		 * chain of trust (can be this key).
 		 */
-		dnskey_omnipresent[DST_KEY_DS] = NA;
-		if (next_state != NA &&
-		    dst_key_id(dkey->key) == dst_key_id(key->key)) {
-			/* Check next state rather than current state. */
-			dnskey_omnipresent[DST_KEY_DS] = next_state;
-		} else {
-			(void)dst_key_getstate(dkey->key, DST_KEY_DS,
-					       &dnskey_omnipresent[DST_KEY_DS]);
+		if (keymgr_key_match_state(dkey->key, key->key, type,
+					   next_state, dnskey_chained))
+		{
+			/* This DNSKEY and KRRSIG are OMNIPRESENT. */
+			continue;
 		}
-		if (!keymgr_key_exists_with_state(
-			    keyring, key, type, next_state, dnskey_omnipresent,
-			    na, false, match_algorithms))
+
+		/*
+		 * Perhaps another key provides a chain of trust.
+		 */
+		dnskey_chained[DST_KEY_DS] = OMNIPRESENT;
+		if (!keymgr_key_exists_with_state(keyring, key, type,
+						  next_state, dnskey_chained,
+						  na, false, match_algorithms))
 		{
 			/* There is no chain of trust. */
 			return (false);
@@ -836,8 +838,8 @@ keymgr_dnskey_hidden_or_chained(dns_dnsseckeylist_t *keyring,
 				dns_dnsseckey_t *key, int type,
 				dst_key_state_t next_state,
 				bool match_algorithms) {
-	dst_key_state_t rrsig_omnipresent[4] = { NA, OMNIPRESENT, NA,
-						 NA };		   /* (3i) */
+	dst_key_state_t rrsig_chained[4] = { OMNIPRESENT, OMNIPRESENT, NA,
+					     NA };		   /* (3i) */
 	dst_key_state_t dnskey_hidden[4] = { HIDDEN, NA, NA, NA }; /* (3i) */
 	dst_key_state_t na[4] = { NA, NA, NA, NA }; /* successor n/a */
 
@@ -861,12 +863,11 @@ keymgr_dnskey_hidden_or_chained(dns_dnsseckeylist_t *keyring,
 		 * least one key with the same algorithm that has its RRSIG
 		 * records OMNIPRESENT.
 		 */
-		rrsig_omnipresent[DST_KEY_DNSKEY] = NA;
 		(void)dst_key_getstate(dkey->key, DST_KEY_DNSKEY,
-				       &rrsig_omnipresent[DST_KEY_DNSKEY]);
+				       &rrsig_chained[DST_KEY_DNSKEY]);
 		if (!keymgr_key_exists_with_state(keyring, key, type,
-						  next_state, rrsig_omnipresent,
-						  na, false, match_algorithms))
+						  next_state, rrsig_chained, na,
+						  false, match_algorithms))
 		{
 			/* There is no chain of trust. */
 			return (false);