From 5d69aab92dd356d837652b2b74609593deb5198a Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 21 Jan 2025 13:20:12 +0000 Subject: [PATCH 1/2] 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. (cherry picked from commit 716b9360452bcebd89d48faa895375fa3360b9ed) --- bin/named/config.c | 2 ++ bin/named/server.c | 13 ++++++++++ bin/tests/system/upforwd/setup.sh | 2 +- bin/tests/system/upforwd/tests.sh | 8 +++--- doc/misc/options | 4 +++ lib/dns/include/dns/view.h | 2 ++ lib/dns/message.c | 43 ++++++++++++++++++++++++------- lib/isccfg/namedconf.c | 2 ++ 8 files changed, 62 insertions(+), 14 deletions(-) diff --git a/bin/named/config.c b/bin/named/config.c index 8f83902859..c72ca9fdff 100644 --- a/bin/named/config.c +++ b/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\ diff --git a/bin/named/server.c b/bin/named/server.c index 1782d02619..f11a4e97e0 100644 --- a/bin/named/server.c +++ b/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. */ diff --git a/bin/tests/system/upforwd/setup.sh b/bin/tests/system/upforwd/setup.sh index 0df66cb6f2..7fddf0175c 100644 --- a/bin/tests/system/upforwd/setup.sh +++ b/bin/tests/system/upforwd/setup.sh @@ -44,7 +44,7 @@ fi cat_i 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 5e1f4550bd..5591aabb67 100644 --- 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 -- - <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 78b7b10b8f..c57d32446c 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -284,6 +284,8 @@ options { sig-validity-interval [ ]; // obsolete sig0checks-quota ; // experimental sig0checks-quota-exempt { ; ... }; // experimental + sig0key-checks-limit ; + sig0message-checks-limit ; sortlist { ; ... }; // deprecated stale-answer-client-timeout ( disabled | off | ); stale-answer-enable ; @@ -582,6 +584,8 @@ view [ ] { sig-signing-signatures ; sig-signing-type ; sig-validity-interval [ ]; // obsolete + sig0key-checks-limit ; + sig0message-checks-limit ; sortlist { ; ... }; // deprecated stale-answer-client-timeout ( disabled | off | ); stale-answer-enable ; diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index b840df8874..d91745809a 100644 --- a/lib/dns/include/dns/view.h +++ b/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; diff --git a/lib/dns/message.c b/lib/dns/message.c index 143f2fa49b..b630810cd3 100644 --- a/lib/dns/message.c +++ b/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; } diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index a6b7415318..3daaeae911 100644 --- a/lib/isccfg/namedconf.c +++ b/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, From 33ddef1244660b190900c5f2b3dbb91378cbcee6 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 21 Jan 2025 13:44:09 +0000 Subject: [PATCH 2/2] Document sig0key-checks-limit and sig0message-checks-limit (cherry picked from commit 5861c10dfb3a704af189265f4bc0b01cdd86c562) --- doc/arm/reference.rst | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 6a5ddb4257..a259f79625 100644 --- a/doc/arm/reference.rst +++ b/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