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 two was that more
than one key should only be required only 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.

diff --git a/bin/named/config.c b/bin/named/config.c
index e3aaecdd3e49f320a9883423f49ab91e781010cc..bdc1722470ad27fb3c1e6e2691c84ce1c25345bd 100644 (file)
--- a/bin/named/config.c
+++ b/bin/named/config.c
@@ -111,6 +111,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\

diff --git a/bin/named/server.c b/bin/named/server.c
index 3dce65e359394a7a7c65c3bd6bded4d78e80ac0d..14bf29d826758a3ec95c88b51baac16608533513 100644 (file)
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -4703,6 +4703,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.
         */

diff --git a/bin/tests/system/upforwd/setup.sh b/bin/tests/system/upforwd/setup.sh
index 0df66cb6f2d525b714d0be26aa7c29ab0ab9efbe..7fddf0175c887b5e151660d659e65bc0dea694d1 100644 (file)
--- a/bin/tests/system/upforwd/setup.sh
+++ b/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

diff --git a/bin/tests/system/upforwd/tests.sh b/bin/tests/system/upforwd/tests.sh
index 5e1f4550bd6a7f8d1b0ca43b3e742d0435050a40..5591aabb67717bfe1237fd85cba3bdb90e4424ee 100644 (file)
--- a/bin/tests/system/upforwd/tests.sh
+++ b/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))

diff --git a/doc/misc/options b/doc/misc/options
index f49c0a8800f7d3c4ad99adfba611491d8c4da563..3fa81f4baedb7771797d49f129db4ed0b850aef4 100644 (file)
--- a/doc/misc/options
+++ b/doc/misc/options
@@ -279,6 +279,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>;
        stale-answer-client-timeout ( disabled | off | <integer> );
        stale-answer-enable <boolean>;
        stale-answer-ttl <duration>;
@@ -573,6 +575,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>;
        stale-answer-client-timeout ( disabled | off | <integer> );
        stale-answer-enable <boolean>;
        stale-answer-ttl <duration>;

diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index aa87fa6c9a3385fd1bb7bdba7cae65b75e08681c..266ba49d6c0ced6bfe72e57a70232a1327355391 100644 (file)
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -179,6 +179,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;

diff --git a/lib/dns/message.c b/lib/dns/message.c
index e174cf8a68307142ea05e784531ecb6759695fcd..bdb3044a78ac5f42e7434beb5db40b703509e7a1 100644 (file)
--- a/lib/dns/message.c
+++ b/lib/dns/message.c
@@ -3279,12 +3279,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);
@@ -3325,8 +3320,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;
 
@@ -3353,8 +3365,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(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(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;
                }
 

diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c
index 6319d0df8f06cab090660dfdb0f37b624a7e781b..85d1c531c0037d2b663e5a11a7b7cd9b6ce4ef8c 100644 (file)
--- a/lib/isccfg/namedconf.c
+++ b/lib/isccfg/namedconf.c
@@ -2122,6 +2122,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_ANCIENT },
        { "stale-answer-enable", &cfg_type_boolean, 0 },
        { "stale-answer-client-timeout", &cfg_type_staleanswerclienttimeout,