[9.20] fix: usr: Implement sig0key-checks-limit and sig0message-checks-limit

Previously a hard-coded limitation of maximum two key or message
verification checks were introduced when checking the message's
SIG(0) signature. It was done in order to protect against possible
DoS attacks. The logic behind choosing the number 2 was that more
than a single key should only be required during key rotations, and
in that case two keys are enough. But later it became apparent that
there are other use cases too where even more keys are required, see
issue number #5050 in GitLab.

This change introduces two new configuration options for the views,
`sig0key-checks-limit` and `sig0message-checks-limit`, which define how
many keys are allowed to be checked to find a matching key, and how
many message verifications are allowed to take place once a matching
key has been found. The latter protects against expensive cryptographic
operations when there are keys with colliding tags and algorithm
numbers, with default being 2, and the former protects against a bit
less expensive key parsing operations and defaults to 16.

Closes #5050

Backport of MR !9967

Merge branch 'backport-5050-sig0-let-considering-more-than-two-keys-9.20' into 'bind-9.20'

See merge request isc-projects/bind9!10141

bin/named/config.c

@@ -112,6 +112,8 @@ options {\n\
 	session-keyname local-ddns;\n\
 	startup-notify-rate 20;\n\
 	sig0checks-quota 1;\n\
+	sig0key-checks-limit 16;\n\
+	sig0message-checks-limit 2;\n\
 	statistics-file \"named.stats\";\n\
 	tcp-advertised-timeout 300;\n\
 	tcp-clients 150;\n\

bin/named/server.c

@@ -5149,6 +5149,19 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	dns_view_settransports(view, transports);
 	dns_transport_list_detach(&transports);
 
+	/*
+	 * Configure SIG(0) check limits when matching a DNS message to a view.
+	 */
+	obj = NULL;
+	result = named_config_get(maps, "sig0key-checks-limit", &obj);
+	INSIST(result == ISC_R_SUCCESS);
+	view->sig0key_checks_limit = cfg_obj_asuint32(obj);
+
+	obj = NULL;
+	result = named_config_get(maps, "sig0message-checks-limit", &obj);
+	INSIST(result == ISC_R_SUCCESS);
+	view->sig0message_checks_limit = cfg_obj_asuint32(obj);
+
 	/*
 	 * Configure the view's TSIG keys.
 	 */

bin/tests/system/upforwd/setup.sh

@@ -44,7 +44,7 @@ fi
 cat_i <keyname.err
 
 cat ns1/example1.db >ns1/example2-toomanykeys.db
-for i in 1 2 3; do
+for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17; do
   keyname=$($KEYGEN -q -n HOST -a ${DEFAULT_ALGORITHM} -T KEY sig0.example2-toomanykeys 2>/dev/null)
   if test -n "$keyname"; then
     cat $keyname.key >>ns1/example2-toomanykeys.db

bin/tests/system/upforwd/tests.sh

@@ -460,7 +460,7 @@ EOF
   ret=0
   good=0
   bad=0
-  for i in 1 2 3; do
+  for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17; do
     keyname=$(cat keyname$i)
     $NSUPDATE -d -D -k $keyname.private -- - <<EOF >nsupdate.out.test$n.$i 2>&1 && good=$((good + 1)) || bad=$((bad + 1))
 	local 10.53.0.1
@@ -470,9 +470,9 @@ EOF
 	send
 EOF
   done
-  # There are three keys in the zone but named checks the signature using
-  # maximum two keys, so one of these updates should have been failed.
-  [ $good = 2 ] && [ $bad = 1 ] || ret=1
+  # There are 17 keys in the zone but by default named checks maximum 16 keys
+  # to find a matching key, so one of these updates should have been failed.
+  [ $good = 16 ] && [ $bad = 1 ] || ret=1
   if [ $ret != 0 ]; then echo_i "failed"; fi
   status=$((status + ret))
   n=$((n + 1))

doc/arm/reference.rst

@@ -4074,6 +4074,26 @@ system.
            2001:db8::100;
        };
 
+.. namedconf:statement:: sig0key-checks-limit
+   :tags: server
+   :short: Specifies the maximum number of SIG(0) keys to consider when trying to verify a message.
+
+   This is the maximum number of keys to consider for a SIG(0)-signed message
+   when trying to verify it. :iscman:`named` will parse the candidate keys and
+   check whether their key tag and algorithm matches with the expected one
+   before trying to verify the signature. If the limit is reached the message
+   verification fails. The value of ``0`` disables the limitation. The default
+   is ``16``.
+
+.. namedconf:statement:: sig0message-checks-limit
+   :tags: server
+   :short: Specifies the maximum number of matching SIG(0) keys to try to verify a message.
+
+   This is the maximum number of keys which (when correctly parsed and matched
+   against the expected key tag and algorithm) :iscman:`named` uses to verify
+   a SIG(0)-signed message. If the limit is reached the message verification
+   fails. The value of ``0`` disables the limitation. The default is ``2``.
+
 .. _intervals:
 
 Periodic Task Intervals

doc/misc/options

@@ -284,6 +284,8 @@ options {
 	sig-validity-interval <integer> [ <integer> ]; // obsolete
 	sig0checks-quota <integer>; // experimental
 	sig0checks-quota-exempt { <address_match_element>; ... }; // experimental
+	sig0key-checks-limit <integer>;
+	sig0message-checks-limit <integer>;
 	sortlist { <address_match_element>; ... }; // deprecated
 	stale-answer-client-timeout ( disabled | off | <integer> );
 	stale-answer-enable <boolean>;
@@ -582,6 +584,8 @@ view <string> [ <class> ] {
 	sig-signing-signatures <integer>;
 	sig-signing-type <integer>;
 	sig-validity-interval <integer> [ <integer> ]; // obsolete
+	sig0key-checks-limit <integer>;
+	sig0message-checks-limit <integer>;
 	sortlist { <address_match_element>; ... }; // deprecated
 	stale-answer-client-timeout ( disabled | off | <integer> );
 	stale-answer-enable <boolean>;

lib/dns/include/dns/view.h

@@ -183,6 +183,8 @@ struct dns_view {
 	uint32_t	      fail_ttl;
 	dns_badcache_t	     *failcache;
 	unsigned int	      udpsize;
+	uint32_t	      sig0key_checks_limit;
+	uint32_t	      sig0message_checks_limit;
 	uint32_t	      maxrrperset;
 	uint32_t	      maxtypepername;
 	uint16_t	      max_queries;

lib/dns/message.c

@@ -3296,12 +3296,7 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) {
 		dns_rdata_sig_t sig;
 		dns_rdataset_t keyset;
 		isc_result_t result;
-		/*
-		 * In order to protect from a possible DoS attack, we are
-		 * going to check at most two KEY RRs.
-		 */
-		const size_t max_keys = 2;
-		size_t n;
+		uint32_t key_checks, message_checks;
 
 		result = dns_rdataset_first(msg->sig0);
 		INSIST(result == ISC_R_SUCCESS);
@@ -3342,8 +3337,25 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) {
 		result = dns_rdataset_first(&keyset);
 		INSIST(result == ISC_R_SUCCESS);
 
-		for (n = 0; result == ISC_R_SUCCESS && n < max_keys;
-		     n++, result = dns_rdataset_next(&keyset))
+		/*
+		 * In order to protect from a possible DoS attack, this function
+		 * supports limitations on how many keyid checks and how many
+		 * key checks (message verifications using a matched key) are
+		 * going to be allowed.
+		 */
+		const uint32_t max_key_checks =
+			view->sig0key_checks_limit > 0
+				? view->sig0key_checks_limit
+				: UINT32_MAX;
+		const uint32_t max_message_checks =
+			view->sig0message_checks_limit > 0
+				? view->sig0message_checks_limit
+				: UINT32_MAX;
+
+		for (key_checks = 0, message_checks = 0;
+		     result == ISC_R_SUCCESS && key_checks < max_key_checks &&
+		     message_checks < max_message_checks;
+		     key_checks++, result = dns_rdataset_next(&keyset))
 		{
 			dst_key_t *key = NULL;
 
@@ -3370,8 +3382,21 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) {
 			if (result == ISC_R_SUCCESS) {
 				break;
 			}
+			message_checks++;
 		}
-		if (result == ISC_R_NOMORE || n == max_keys) {
+		if (result == ISC_R_NOMORE) {
+			result = DNS_R_KEYUNAUTHORIZED;
+		} else if (key_checks == max_key_checks) {
+			isc_log_write(dns_lctx, ISC_LOGCATEGORY_GENERAL,
+				      DNS_LOGMODULE_MESSAGE, ISC_LOG_DEBUG(3),
+				      "sig0key-checks-limit reached when "
+				      "trying to check a message signature");
+			result = DNS_R_KEYUNAUTHORIZED;
+		} else if (message_checks == max_message_checks) {
+			isc_log_write(dns_lctx, ISC_LOGCATEGORY_GENERAL,
+				      DNS_LOGMODULE_MESSAGE, ISC_LOG_DEBUG(3),
+				      "sig0message-checks-limit reached when "
+				      "trying to check a message signature");
 			result = DNS_R_KEYUNAUTHORIZED;
 		}
 

lib/isccfg/namedconf.c

@@ -2276,6 +2276,8 @@ static cfg_clausedef_t view_clauses[] = {
 	{ "rrset-order", &cfg_type_rrsetorder, 0 },
 	{ "send-cookie", &cfg_type_boolean, 0 },
 	{ "servfail-ttl", &cfg_type_duration, 0 },
+	{ "sig0key-checks-limit", &cfg_type_uint32, 0 },
+	{ "sig0message-checks-limit", &cfg_type_uint32, 0 },
 	{ "sortlist", &cfg_type_bracketed_aml, CFG_CLAUSEFLAG_DEPRECATED },
 	{ "stale-answer-enable", &cfg_type_boolean, 0 },
 	{ "stale-answer-client-timeout", &cfg_type_staleanswerclienttimeout,