From c7f79a0353d7562fcf3e83ac6983f4ea06050d47 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 25 Mar 2024 11:07:47 +0000 Subject: [PATCH 01/14] Add a quota for SIG(0) signature checks In order to protect from a malicious DNS client that sends many queries with a SIG(0)-signed message, add a quota of simultaneously running SIG(0) checks. This protection can only help when named is using more than one worker threads. For example, if named is running with the '-n 4' option, and 'sig0checks-quota 2;' is used, then named will make sure to not use more than 2 workers for the SIG(0) signature checks in parallel, thus leaving the other workers to serve the remaining clients which do not use SIG(0)-signed messages. That limitation is going to change when SIG(0) signature checks are offloaded to "slow" threads in a future commit. The 'sig0checks-quota-exempt' ACL option can be used to exempt certain clients from the quota requirements using their IP or network addresses. The 'sig0checks-quota-maxwait-ms' option is used to define a maximum amount of time for named to wait for a quota to appear. If during that time no new quota becomes available, named will answer to the client with DNS_R_REFUSED. --- bin/delv/delv.c | 5 +- bin/named/config.c | 2 + bin/named/server.c | 65 ++++- .../bad-sig0checks-quota-exempt.conf | 16 ++ .../good-sig0checks-quota-exempt.conf | 20 ++ doc/misc/options | 3 + lib/isccfg/check.c | 15 + lib/isccfg/namedconf.c | 3 + lib/ns/client.c | 266 +++++++++++++----- lib/ns/include/ns/client.h | 4 + lib/ns/include/ns/server.h | 12 +- lib/ns/server.c | 6 + tests/libtest/ns.c | 5 +- 13 files changed, 339 insertions(+), 83 deletions(-) create mode 100644 bin/tests/system/checkconf/bad-sig0checks-quota-exempt.conf create mode 100644 bin/tests/system/checkconf/good-sig0checks-quota-exempt.conf diff --git a/bin/delv/delv.c b/bin/delv/delv.c index e06322de11..5af7c15653 100644 --- a/bin/delv/delv.c +++ b/bin/delv/delv.c @@ -2117,12 +2117,13 @@ cleanup: static isc_result_t matchview(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, - dns_message_t *message, dns_aclenv_t *env, isc_result_t *sigresultp, - dns_view_t **viewp) { + dns_message_t *message, dns_aclenv_t *env, ns_server_t *lsctx, + isc_result_t *sigresultp, dns_view_t **viewp) { UNUSED(srcaddr); UNUSED(destaddr); UNUSED(message); UNUSED(env); + UNUSED(lsctx); UNUSED(sigresultp); *viewp = view; diff --git a/bin/named/config.c b/bin/named/config.c index 732e28e606..62c05a8314 100644 --- a/bin/named/config.c +++ b/bin/named/config.c @@ -109,6 +109,8 @@ options {\n\ # session-keyfile \"" NAMED_LOCALSTATEDIR "/run/named/session.key\";\n\ session-keyname local-ddns;\n\ startup-notify-rate 20;\n\ + sig0checks-quota 1;\n\ + sig0checks-quota-maxwait-ms 1500;\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 c41f5d9b24..162b556542 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -8412,6 +8412,8 @@ load_configuration(const char *filename, named_server_t *server, configure_server_quota(maps, "recursive-clients", &server->sctx->recursionquota); configure_server_quota(maps, "update-quota", &server->sctx->updquota); + configure_server_quota(maps, "sig0checks-quota", + &server->sctx->sig0checksquota); max = isc_quota_getmax(&server->sctx->recursionquota); if (max > 1000) { @@ -8430,9 +8432,23 @@ load_configuration(const char *filename, named_server_t *server, } else { softquota = (max * 90) / 100; } - isc_quota_soft(&server->sctx->recursionquota, softquota); + obj = NULL; + result = named_config_get(maps, "sig0checks-quota-exempt", &obj); + if (result == ISC_R_SUCCESS) { + result = cfg_acl_fromconfig( + obj, config, named_g_lctx, named_g_aclconfctx, + named_g_mctx, 0, &server->sctx->sig0checksquota_exempt); + INSIST(result == ISC_R_SUCCESS); + } + + obj = NULL; + result = named_config_get(maps, "sig0checks-quota-maxwait-ms", &obj); + if (result == ISC_R_SUCCESS) { + server->sctx->sig0checksquota_maxwaitms = cfg_obj_asuint32(obj); + } + /* * Set "blackhole". Only legal at options level; there is * no default. @@ -10048,11 +10064,12 @@ shutdown_server(void *arg) { */ static isc_result_t get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, - dns_message_t *message, dns_aclenv_t *env, + dns_message_t *message, dns_aclenv_t *env, ns_server_t *sctx, isc_result_t *sigresult, dns_view_t **viewp) { dns_view_t *view; REQUIRE(message != NULL); + REQUIRE(sctx != NULL); REQUIRE(sigresult != NULL); REQUIRE(viewp != NULL && *viewp == NULL); @@ -10063,13 +10080,49 @@ get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, message->rdclass == dns_rdataclass_any) { const dns_name_t *tsig = NULL; + int exempt_match; + isc_result_t sig0_qresult = ISC_R_UNSET; + if (message->sig0 != NULL) { + /* + * If the message has a SIG0 signature and the + * client is not exempt from the quota, then + * acquire a quota. If quota is reached, then + * return early. + */ + if (sctx->sig0checksquota_exempt != NULL) { + isc_result_t result = dns_acl_match( + srcaddr, NULL, + sctx->sig0checksquota_exempt, + env, &exempt_match, NULL); + if (result == ISC_R_SUCCESS && + exempt_match > 0) + { + sig0_qresult = ISC_R_EXISTS; + } + } + if (sig0_qresult == ISC_R_UNSET) { + sig0_qresult = isc_quota_acquire( + &sctx->sig0checksquota); + } + if (sig0_qresult == ISC_R_SOFTQUOTA) { + isc_quota_release( + &sctx->sig0checksquota); + } + if (sig0_qresult != ISC_R_SUCCESS && + sig0_qresult != ISC_R_EXISTS) + { + return (ISC_R_QUOTA); + } + } + + /* Check the signature, then release the quota */ *sigresult = dns_message_rechecksig(message, view); + if (sig0_qresult == ISC_R_SUCCESS) { + isc_quota_release(&sctx->sig0checksquota); + } if (*sigresult == ISC_R_SUCCESS) { - dns_tsigkey_t *tsigkey; - - tsigkey = message->tsigkey; - tsig = dns_tsigkey_identity(tsigkey); + tsig = dns_tsigkey_identity(message->tsigkey); } if (dns_acl_allowed(srcaddr, tsig, view->matchclients, diff --git a/bin/tests/system/checkconf/bad-sig0checks-quota-exempt.conf b/bin/tests/system/checkconf/bad-sig0checks-quota-exempt.conf new file mode 100644 index 0000000000..c54227dd91 --- /dev/null +++ b/bin/tests/system/checkconf/bad-sig0checks-quota-exempt.conf @@ -0,0 +1,16 @@ +/* + * 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. + */ + +options { + sig0checks-quota-exempt { unknownacl; }; +}; diff --git a/bin/tests/system/checkconf/good-sig0checks-quota-exempt.conf b/bin/tests/system/checkconf/good-sig0checks-quota-exempt.conf new file mode 100644 index 0000000000..2968ebefe0 --- /dev/null +++ b/bin/tests/system/checkconf/good-sig0checks-quota-exempt.conf @@ -0,0 +1,20 @@ +/* + * 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. + */ + +acl goodacl { + 192.168.0.1; +}; + +options { + sig0checks-quota-exempt { 10.0.0.0/8; 2001:db8::100; goodacl; }; +}; diff --git a/doc/misc/options b/doc/misc/options index de24eef2c2..d003e9e741 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -277,6 +277,9 @@ options { sig-signing-signatures ; sig-signing-type ; sig-validity-interval [ ]; // obsolete + sig0checks-quota ; + sig0checks-quota-exempt { ; ... }; + sig0checks-quota-maxwait-ms ; sortlist { ; ... }; // deprecated stale-answer-client-timeout ( disabled | off | ); stale-answer-enable ; diff --git a/lib/isccfg/check.c b/lib/isccfg/check.c index e0c4bdc5b8..8dd8efa934 100644 --- a/lib/isccfg/check.c +++ b/lib/isccfg/check.c @@ -2073,6 +2073,21 @@ check_options(const cfg_obj_t *options, const cfg_obj_t *config, cfg_aclconfctx_create(mctx, &actx); + obj = NULL; + (void)cfg_map_get(options, "sig0checks-quota-exempt", &obj); + if (obj != NULL) { + dns_acl_t *acl = NULL; + + tresult = cfg_acl_fromconfig(obj, config, logctx, actx, mctx, 0, + &acl); + if (acl != NULL) { + dns_acl_detach(&acl); + } + if (result == ISC_R_SUCCESS) { + result = tresult; + } + } + obj = NULL; (void)cfg_map_get(options, "listen-on", &obj); if (obj != NULL) { diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index 18b40fab7f..3a27432f7e 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -1361,6 +1361,9 @@ static cfg_clausedef_t options_clauses[] = { { "session-keyalg", &cfg_type_astring, 0 }, { "session-keyfile", &cfg_type_qstringornone, 0 }, { "session-keyname", &cfg_type_astring, 0 }, + { "sig0checks-quota", &cfg_type_uint32, 0 }, + { "sig0checks-quota-maxwait-ms", &cfg_type_uint32, 0 }, + { "sig0checks-quota-exempt", &cfg_type_bracketed_aml, 0 }, { "sit-secret", NULL, CFG_CLAUSEFLAG_ANCIENT }, { "stacksize", &cfg_type_size, CFG_CLAUSEFLAG_ANCIENT }, { "startup-notify-rate", &cfg_type_uint32, 0 }, diff --git a/lib/ns/client.c b/lib/ns/client.c index c1341be828..03afc4d25c 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -118,11 +118,27 @@ atomic_uint_fast64_t ns_client_requests = 0; +static atomic_uint_fast32_t last_sigchecks_quota_log = 0; + +static bool +can_log_sigchecks_quota(void) { + isc_stdtime_t last; + isc_stdtime_t now = isc_stdtime_now(); + last = atomic_exchange_relaxed(&last_sigchecks_quota_log, now); + if (now != last) { + return (true); + } + + return (false); +} + static void clientmgr_destroy_cb(void *arg); static void ns_client_dumpmessage(ns_client_t *client, const char *reason); static void +ns_client_request_continue(void *arg); +static void compute_cookie(ns_client_t *client, uint32_t when, const unsigned char *secret, isc_buffer_t *buf); @@ -1739,6 +1755,53 @@ ns__client_put_cb(void *client0) { ns_clientmgr_detach(&manager); } +static isc_result_t +ns_client_setup_view(ns_client_t *client, isc_netaddr_t *netaddr) { + isc_result_t result; + + result = client->manager->sctx->matchingview( + netaddr, &client->destaddr, client->message, + client->manager->aclenv, client->manager->sctx, + &client->sigresult, &client->view); + if (result != ISC_R_SUCCESS) { + if (result == ISC_R_QUOTA) { + if (can_log_sigchecks_quota()) { + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_INFO, + "SIG(0) checks quota reached"); + ns_client_dumpmessage( + client, "SIG(0) checks quota reached"); + } + } else { + char classname[DNS_RDATACLASS_FORMATSIZE]; + isc_buffer_t b; + isc_region_t *r; + + /* + * Do a dummy TSIG verification attempt so that the + * response will have a TSIG if the query did, as + * required by RFC2845. + */ + dns_message_resetsig(client->message); + r = dns_message_getrawmessage(client->message); + isc_buffer_init(&b, r->base, r->length); + isc_buffer_add(&b, r->length); + (void)dns_tsig_verify(&b, client->message, NULL, NULL); + + dns_rdataclass_format(client->message->rdclass, + classname, sizeof(classname)); + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), + "no matching view in class '%s'", + classname); + ns_client_dumpmessage(client, + "no matching view in class"); + } + } + + return (result); +} + /* * Handle an incoming request event from the socket (UDP case) * or tcpmsg (TCP case). @@ -1748,12 +1811,7 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, void *arg) { ns_client_t *client = NULL; isc_result_t result; - isc_result_t sigresult = ISC_R_SUCCESS; - isc_buffer_t *buffer = NULL; - isc_buffer_t tbuffer; dns_rdataset_t *opt = NULL; - const dns_name_t *signame = NULL; - bool ra; /* Recursion available. */ isc_netaddr_t netaddr; int match; dns_messageid_t id; @@ -1761,28 +1819,6 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, bool notimp; size_t reqsize; dns_aclenv_t *env = NULL; -#ifdef HAVE_DNSTAP - dns_transport_type_t transport_type; - dns_dtmsgtype_t dtmsgtype; -#endif /* ifdef HAVE_DNSTAP */ - static const char *ra_reasons[] = { - "ACLs not processed yet", - "no resolver in view", - "recursion not enabled for view", - "allow-recursion did not match", - "allow-query-cache did not match", - "allow-recursion-on did not match", - "allow-query-cache-on did not match", - }; - enum refusal_reasons { - INVALID, - NO_RESOLVER, - RECURSION_DISABLED, - ALLOW_RECURSION, - ALLOW_QUERY_CACHE, - ALLOW_RECURSION_ON, - ALLOW_QUERY_CACHE_ON - } ra_refusal_reason = INVALID; if (eresult != ISC_R_SUCCESS) { return; @@ -1830,14 +1866,14 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, (void)atomic_fetch_add_relaxed(&ns_client_requests, 1); - isc_buffer_init(&tbuffer, region->base, region->length); - isc_buffer_add(&tbuffer, region->length); - buffer = &tbuffer; + isc_buffer_init(&client->tbuffer, region->base, region->length); + isc_buffer_add(&client->tbuffer, region->length); + client->buffer = &client->tbuffer; client->peeraddr = isc_nmhandle_peeraddr(handle); client->peeraddr_valid = true; - reqsize = isc_buffer_usedlength(buffer); + reqsize = isc_buffer_usedlength(client->buffer); client->state = NS_CLIENTSTATE_WORKING; @@ -1876,7 +1912,7 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, ISC_LOG_DEBUG(3), "%s request", TCP_CLIENT(client) ? "TCP" : "UDP"); - result = dns_message_peekheader(buffer, &id, &flags); + result = dns_message_peekheader(client->buffer, &id, &flags); if (result != ISC_R_SUCCESS) { /* * There isn't enough header to determine whether @@ -1951,7 +1987,7 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, /* * It's a request. Parse it. */ - result = dns_message_parse(client->message, buffer, 0); + result = dns_message_parse(client->message, client->buffer, 0); if (result != ISC_R_SUCCESS) { /* * Parsing the request failed. Send a response @@ -2080,38 +2116,119 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, client->destsockaddr = isc_nmhandle_localaddr(handle); isc_netaddr_fromsockaddr(&client->destaddr, &client->destsockaddr); - result = client->manager->sctx->matchingview( - &netaddr, &client->destaddr, client->message, env, &sigresult, - &client->view); - if (result != ISC_R_SUCCESS) { - char classname[DNS_RDATACLASS_FORMATSIZE]; - - /* - * Do a dummy TSIG verification attempt so that the - * response will have a TSIG if the query did, as - * required by RFC2845. - */ - isc_buffer_t b; - isc_region_t *r; - - dns_message_resetsig(client->message); - - r = dns_message_getrawmessage(client->message); - isc_buffer_init(&b, r->base, r->length); - isc_buffer_add(&b, r->length); - (void)dns_tsig_verify(&b, client->message, NULL, NULL); - - dns_rdataclass_format(client->message->rdclass, classname, - sizeof(classname)); + result = ns_client_setup_view(client, &netaddr); + if (result == ISC_R_QUOTA) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "no matching view in class '%s'", classname); - ns_client_dumpmessage(client, "no matching view in class"); + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), + "client is starting to wait for quota"); + client->async = true; + isc_nmhandle_ref(client->handle); + isc_async_run(client->manager->loop, ns_client_request_continue, + client); + return; + } else if (result != ISC_R_SUCCESS) { ns_client_extendederror(client, DNS_EDE_PROHIBITED, NULL); ns_client_error(client, notimp ? DNS_R_NOTIMP : DNS_R_REFUSED); return; } + ns_client_request_continue(client); +} + +static void +ns_client_request_continue(void *arg) { + ns_client_t *client = arg; + isc_netaddr_t netaddr; + const dns_name_t *signame = NULL; + bool ra; /* Recursion available. */ + isc_result_t result; + static const char *ra_reasons[] = { + "ACLs not processed yet", + "no resolver in view", + "recursion not enabled for view", + "allow-recursion did not match", + "allow-query-cache did not match", + "allow-recursion-on did not match", + "allow-query-cache-on did not match", + }; + enum refusal_reasons { + INVALID, + NO_RESOLVER, + RECURSION_DISABLED, + ALLOW_RECURSION, + ALLOW_QUERY_CACHE, + ALLOW_RECURSION_ON, + ALLOW_QUERY_CACHE_ON + } ra_refusal_reason = INVALID; +#ifdef HAVE_DNSTAP + dns_transport_type_t transport_type; + dns_dtmsgtype_t dtmsgtype; +#endif /* ifdef HAVE_DNSTAP */ + + /* + * This function could be running asynchronously if a quota was reached + * before, and named was waiting for available quota. In that case we + * need to update the current 'now', and check that named doesn't wait + * for too long. + */ + if (client->async) { + uint64_t wait_us; + uint64_t maxwait_us; + + client->tnow = isc_time_now(); + client->now = isc_time_seconds(&client->tnow); + + wait_us = isc_time_microdiff(&client->tnow, + &client->requesttime); + maxwait_us = US_PER_MS * + client->manager->sctx->sig0checksquota_maxwaitms; + if (wait_us > maxwait_us) { + isc_buffer_t b; + isc_region_t *r; + + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), + "client reached max wait time for quota"); + + /* + * Do a dummy TSIG verification attempt so that the + * response will have a TSIG if the query did, as + * required by RFC2845. + */ + dns_message_resetsig(client->message); + r = dns_message_getrawmessage(client->message); + isc_buffer_init(&b, r->base, r->length); + isc_buffer_add(&b, r->length); + (void)dns_tsig_verify(&b, client->message, NULL, NULL); + + ns_client_extendederror(client, DNS_EDE_PROHIBITED, + NULL); + ns_client_error(client, DNS_R_REFUSED); + goto cleanup; + } + } + + isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); + + if (client->view == NULL) { + result = ns_client_setup_view(client, &netaddr); + if (result == ISC_R_QUOTA) { + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), + "client continues waiting for quota"); + client->async = true; + isc_nmhandle_ref(client->handle); + isc_async_run(client->manager->loop, + ns_client_request_continue, client); + goto cleanup; + } else if (result != ISC_R_SUCCESS) { + ns_client_extendederror(client, DNS_EDE_PROHIBITED, + NULL); + ns_client_error(client, DNS_R_REFUSED); + goto cleanup; + } + } + if (isc_nm_is_proxy_handle(client->handle)) { char fmtbuf[ISC_SOCKADDR_FORMATSIZE] = { 0 }; isc_netaddr_t real_local_addr, real_peer_addr; @@ -2140,8 +2257,8 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, "ACL", fmtbuf); } - isc_nm_bad_request(handle); - return; + isc_nm_bad_request(client->handle); + goto cleanup; } /* allow by default */ @@ -2161,8 +2278,8 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, "'allow-proxy-on' ACL", fmtbuf); } - isc_nm_bad_request(handle); - return; + isc_nm_bad_request(client->handle); + goto cleanup; } } @@ -2255,8 +2372,8 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, if (!(client->message->tsigstatus == dns_tsigerror_badkey && client->message->opcode == dns_opcode_update)) { - ns_client_error(client, sigresult); - return; + ns_client_error(client, client->sigresult); + goto cleanup; } } @@ -2340,25 +2457,25 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, dns_dt_send(client->view, dtmsgtype, &client->peeraddr, &client->destsockaddr, transport_type, NULL, - &client->requesttime, NULL, buffer); + &client->requesttime, NULL, client->buffer); #endif /* HAVE_DNSTAP */ - ns_query_start(client, handle); + ns_query_start(client, client->handle); break; case dns_opcode_update: CTRACE("update"); #ifdef HAVE_DNSTAP dns_dt_send(client->view, DNS_DTTYPE_UQ, &client->peeraddr, &client->destsockaddr, transport_type, NULL, - &client->requesttime, NULL, buffer); + &client->requesttime, NULL, client->buffer); #endif /* HAVE_DNSTAP */ ns_client_settimeout(client, 60); - ns_update_start(client, handle, sigresult); + ns_update_start(client, client->handle, client->sigresult); break; case dns_opcode_notify: CTRACE("notify"); ns_client_settimeout(client, 60); - ns_notify_start(client, handle); + ns_notify_start(client, client->handle); break; case dns_opcode_iquery: CTRACE("iquery"); @@ -2368,6 +2485,15 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, CTRACE("unknown opcode"); ns_client_error(client, DNS_R_NOTIMP); } + +cleanup: + if (client->async) { + /* + * Do not detach, only 'unref' the corresponding 'ref' when + * async was used, because the client can still be reused. + */ + isc_nmhandle_unref(client->handle); + } } isc_result_t diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 67126b01f7..39c1fa0a86 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -167,6 +167,7 @@ struct ns_client { ns_clientmgr_t *manager; ns_clientstate_t state; bool nodetach; + bool async; unsigned int attributes; dns_view_t *view; dns_dispatch_t *dispatch; @@ -192,6 +193,9 @@ struct ns_client { isc_time_t tnow; dns_name_t signername; /*%< [T]SIG key name */ dns_name_t *signer; /*%< NULL if not valid sig */ + isc_result_t sigresult; + isc_buffer_t *buffer; + isc_buffer_t tbuffer; isc_sockaddr_t peeraddr; bool peeraddr_valid; diff --git a/lib/ns/include/ns/server.h b/lib/ns/include/ns/server.h index 84003bf57f..fae2040b74 100644 --- a/lib/ns/include/ns/server.h +++ b/lib/ns/include/ns/server.h @@ -64,9 +64,12 @@ typedef void (*ns_fuzzcb_t)(void); /*% * Type for callback function to get the view that can answer a query. */ -typedef isc_result_t (*ns_matchview_t)( - isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, dns_message_t *message, - dns_aclenv_t *env, isc_result_t *sigresultp, dns_view_t **viewp); +typedef isc_result_t (*ns_matchview_t)(isc_netaddr_t *srcaddr, + isc_netaddr_t *destaddr, + dns_message_t *message, + dns_aclenv_t *env, ns_server_t *sctx, + isc_result_t *sigresultp, + dns_view_t **viewp); /*% * Server context. @@ -88,6 +91,9 @@ struct ns_server { isc_quota_t tcpquota; isc_quota_t xfroutquota; isc_quota_t updquota; + isc_quota_t sig0checksquota; + uint32_t sig0checksquota_maxwaitms; + dns_acl_t *sig0checksquota_exempt; ISC_LIST(isc_quota_t) http_quotas; isc_mutex_t http_quotas_lock; diff --git a/lib/ns/server.c b/lib/ns/server.c index d853848a76..ea4a588c18 100644 --- a/lib/ns/server.c +++ b/lib/ns/server.c @@ -66,6 +66,7 @@ ns_server_create(isc_mem_t *mctx, ns_matchview_t matchingview, isc_quota_init(&sctx->tcpquota, 10); isc_quota_init(&sctx->recursionquota, 100); isc_quota_init(&sctx->updquota, 100); + isc_quota_init(&sctx->sig0checksquota, 1); ISC_LIST_INIT(sctx->http_quotas); isc_mutex_init(&sctx->http_quotas_lock); @@ -134,6 +135,11 @@ ns_server_detach(ns_server_t **sctxp) { isc_mem_put(sctx->mctx, altsecret, sizeof(*altsecret)); } + if (sctx->sig0checksquota_exempt != NULL) { + dns_acl_detach(&sctx->sig0checksquota_exempt); + } + + isc_quota_destroy(&sctx->sig0checksquota); isc_quota_destroy(&sctx->updquota); isc_quota_destroy(&sctx->recursionquota); isc_quota_destroy(&sctx->tcpquota); diff --git a/tests/libtest/ns.c b/tests/libtest/ns.c index 91799a774c..f09b20045c 100644 --- a/tests/libtest/ns.c +++ b/tests/libtest/ns.c @@ -57,12 +57,13 @@ ns_server_t *sctx = NULL; static isc_result_t matchview(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, - dns_message_t *message, dns_aclenv_t *env, isc_result_t *sigresultp, - dns_view_t **viewp) { + dns_message_t *message, dns_aclenv_t *env, ns_server_t *lsctx, + isc_result_t *sigresultp, dns_view_t **viewp) { UNUSED(srcaddr); UNUSED(destaddr); UNUSED(message); UNUSED(env); + UNUSED(lsctx); UNUSED(sigresultp); UNUSED(viewp); From bbc866d0cb9f363dd9eafb87f6ca426db2c25db6 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 27 Mar 2024 15:22:28 +0000 Subject: [PATCH 02/14] Document the SIG(0) signature checking quota options Add documentation entries for the 'sig0checks-quota', 'sig0checks-quota-maxwait-ms', and 'sig0checks-quota-exempt' optoins. --- doc/arm/reference.rst | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 1129dce66c..b0c593e4b4 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4000,6 +4000,50 @@ system. the server will accept for updating local authoritiative zones or forwarding to a primary server. The default is ``100``. +.. namedconf:statement:: sig0checks-quota + :tags: server + :short: Specifies the maximum number of concurrent SIG(0) signature checks that can be processed by the server. + + This is the maximum number of simultaneous SIG(0)-signed messages that + the server will accept. If the quota is reached, then :iscman:`named` waits + for the maximum of :any:`sig0checks-quota-maxwait-ms` time for a quota to + appear or to answer with a status code of REFUSED. The value of ``0`` + disables the quota. The default is ``1``. + + .. note:: + + :any:`sig0checks-quota` protection does not work when there is only one + worker thread available, or when the option is set to a value that is + greater or equal to the worker threads available. See the ``-n #cpus`` + option of :iscman:`named` for more information about the worker threads. + +.. namedconf:statement:: sig0checks-quota-maxwait-ms + :tags: server + :short: Specifies the maximum number of milliseconds to wait for a SIG(0) signature checking quota to appear. + + When :any:`sig0checks-quota` is effective and a client reaches the quota, + then :iscman:`named` waits for the maximum of + :any:`sig0checks-quota-maxwait-ms` time (in milliseconds) for a quota to + appear. If no quota becomes available, then an answer with a status code of + REFUSED is sent. The default is ``1500``. + +.. namedconf:statement:: sig0checks-quota-exempt + :tags: server + :short: Exempts specific clients or client groups from SIG(0) signature checking quota. + + DNS clients can be exempted from SIG(0) signature checking quota with the + :any:`sig0checks-quota-exempt` clause using their IP and/or Network + addresses. The default value is an empty list. + + Example: + + :: + + sig0checks-quota-exempt { + 10.0.0.0/8; + 2001:db8::100; + }; + .. _intervals: Periodic Task Intervals From 7f013ad05d1c81f9a3c54253c3298ee89d60ac76 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 5 Mar 2024 10:11:43 +0000 Subject: [PATCH 03/14] Remove dns_message_rechecksig() This is a tiny helper function which is used only once and can be replaced with two function calls instead. Removing this makes supporting asynchronous signature checking less complicated. --- bin/named/server.c | 3 ++- lib/dns/include/dns/message.h | 21 --------------------- lib/dns/message.c | 6 ------ 3 files changed, 2 insertions(+), 28 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 162b556542..6c4ed095ab 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -10117,7 +10117,8 @@ get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, } /* Check the signature, then release the quota */ - *sigresult = dns_message_rechecksig(message, view); + dns_message_resetsig(message); + *sigresult = dns_message_checksig(message, view); if (sig0_qresult == ISC_R_SUCCESS) { isc_quota_release(&sctx->sig0checksquota); } diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index 677e02826c..ed135979d5 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -1327,27 +1327,6 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view); *\li #DNS_R_TSIGVERIFYFAILURE - The TSIG failed to verify */ -isc_result_t -dns_message_rechecksig(dns_message_t *msg, dns_view_t *view); -/*%< - * Reset the signature state and then if the message was signed, - * verify the message. - * - * Requires: - * - *\li msg is a valid parsed message. - *\li view is a valid view or NULL - * - * Returns: - * - *\li #ISC_R_SUCCESS - the message was unsigned, or the message - * was signed correctly. - * - *\li #DNS_R_EXPECTEDTSIG - A TSIG was expected, but not seen - *\li #DNS_R_UNEXPECTEDTSIG - A TSIG was seen but not expected - *\li #DNS_R_TSIGVERIFYFAILURE - The TSIG failed to verify - */ - void dns_message_resetsig(dns_message_t *msg); /*%< diff --git a/lib/dns/message.c b/lib/dns/message.c index 4f0d7740fa..e99fd5edc9 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3177,12 +3177,6 @@ dns_message_resetsig(dns_message_t *msg) { } } -isc_result_t -dns_message_rechecksig(dns_message_t *msg, dns_view_t *view) { - dns_message_resetsig(msg); - return (dns_message_checksig(msg, view)); -} - #ifdef SKAN_MSG_DEBUG void dns_message_dumpsig(dns_message_t *msg, char *txt1) { From 710bf9b938b55fdc66df7e852ccc5a2ea0ac1ac2 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 6 Mar 2024 11:06:27 +0000 Subject: [PATCH 04/14] Implement asynchronous message signature verification Add support for using the offload threadpool to perform message signature verifications. This should allow check SIG(0)-signed messages without affecting the worker threads. --- lib/dns/include/dns/message.h | 22 ++++++++++++++ lib/dns/message.c | 54 +++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index ed135979d5..81c641dd84 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -352,6 +352,8 @@ struct dns_ednsopt { unsigned char *value; }; +typedef void (*dns_message_cb_t)(void *arg, isc_result_t result); + /*** *** Functions ***/ @@ -1327,6 +1329,26 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view); *\li #DNS_R_TSIGVERIFYFAILURE - The TSIG failed to verify */ +isc_result_t +dns_message_checksig_async(dns_message_t *msg, dns_view_t *view, + isc_loop_t *loop, dns_message_cb_t cb, void *cbarg); +/*%< + * Run dns_message_checksig() in an offloaded thread and return its result + * using the 'cb' callback function, running on the 'loop'. + * + * Requires: + * + *\li msg is a valid parsed message. + *\li view is a valid view or NULL. + *\li loop is a valid loop. + *\li cb is a valid callback function. + * + * Returns: + * + *\li #DNS_R_WAIT + * + */ + void dns_message_resetsig(dns_message_t *msg); /*%< diff --git a/lib/dns/message.c b/lib/dns/message.c index e99fd5edc9..bba68efe54 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -180,6 +181,18 @@ msgblock_allocate(isc_mem_t *, unsigned int, unsigned int); #define msgblock_get(block, type) \ ((type *)msgblock_internalget(block, sizeof(type))) +/* + * A context type to pass information when checking a message signature + * asynchronously. + */ +typedef struct checksig_ctx { + dns_message_t *msg; + dns_view_t *view; + dns_message_cb_t cb; + void *cbarg; + isc_result_t result; +} checksig_ctx_t; + /* * This function differs from public dns_message_puttemprdataset() that it * requires the *rdatasetp to be associated, and it will disassociate and @@ -3205,6 +3218,47 @@ dns_message_dumpsig(dns_message_t *msg, char *txt1) { } #endif /* ifdef SKAN_MSG_DEBUG */ +static void +checksig_run(void *arg) { + checksig_ctx_t *chsigctx = arg; + + chsigctx->result = dns_message_checksig(chsigctx->msg, chsigctx->view); +} + +static void +checksig_cb(void *arg) { + checksig_ctx_t *chsigctx = arg; + dns_message_t *msg = chsigctx->msg; + + chsigctx->cb(chsigctx->cbarg, chsigctx->result); + + dns_view_detach(&chsigctx->view); + isc_mem_put(msg->mctx, chsigctx, sizeof(*chsigctx)); + dns_message_detach(&msg); +} + +isc_result_t +dns_message_checksig_async(dns_message_t *msg, dns_view_t *view, + isc_loop_t *loop, dns_message_cb_t cb, void *cbarg) { + REQUIRE(DNS_MESSAGE_VALID(msg)); + REQUIRE(view != NULL); + REQUIRE(loop != NULL); + REQUIRE(cb != NULL); + + checksig_ctx_t *chsigctx = isc_mem_get(msg->mctx, sizeof(*chsigctx)); + *chsigctx = (checksig_ctx_t){ + .cb = cb, + .cbarg = cbarg, + .result = ISC_R_UNSET, + }; + dns_message_attach(msg, &chsigctx->msg); + dns_view_attach(view, &chsigctx->view); + + isc_work_enqueue(loop, checksig_run, checksig_cb, chsigctx); + + return (DNS_R_WAIT); +} + isc_result_t dns_message_checksig(dns_message_t *msg, dns_view_t *view) { isc_buffer_t b, msgb; From f0cde05e064337cfb4a50a92c11131f6d4a89c1b Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 8 May 2024 18:42:48 +0000 Subject: [PATCH 05/14] Implement asynchronous view matching for SIG(0)-signed queries View matching on an incoming query checks the query's signature, which can be a CPU-heavy task for a SIG(0)-signed message. Implement an asynchronous mode of the view matching function which uses the offloaded signature checking facilities, and use it for the incoming queries. --- bin/delv/delv.c | 8 +- bin/named/server.c | 264 ++++++++++++++++++++++++++++++------- lib/ns/client.c | 206 ++++++++++++++++++----------- lib/ns/include/ns/client.h | 3 + lib/ns/include/ns/server.h | 11 +- tests/libtest/ns.c | 8 +- 6 files changed, 368 insertions(+), 132 deletions(-) diff --git a/bin/delv/delv.c b/bin/delv/delv.c index 5af7c15653..ef078bed09 100644 --- a/bin/delv/delv.c +++ b/bin/delv/delv.c @@ -2118,15 +2118,21 @@ cleanup: static isc_result_t matchview(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, dns_message_t *message, dns_aclenv_t *env, ns_server_t *lsctx, - isc_result_t *sigresultp, dns_view_t **viewp) { + isc_loop_t *loop, isc_job_cb cb, void *cbarg, + isc_result_t *sigresultp, isc_result_t *viewpatchresultp, + dns_view_t **viewp) { UNUSED(srcaddr); UNUSED(destaddr); UNUSED(message); UNUSED(env); UNUSED(lsctx); + UNUSED(loop); + UNUSED(cb); + UNUSED(cbarg); UNUSED(sigresultp); *viewp = view; + *viewpatchresultp = ISC_R_SUCCESS; return (ISC_R_SUCCESS); } diff --git a/bin/named/server.c b/bin/named/server.c index 6c4ed095ab..a4c3269432 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -278,6 +278,26 @@ struct zonelistentry { ISC_LINK(struct zonelistentry) link; }; +/*% + * Message-to-view matching context to run message signature validation + * asynchronously. + */ +typedef struct matching_view_ctx { + isc_netaddr_t *srcaddr; + isc_netaddr_t *destaddr; + dns_message_t *message; + dns_aclenv_t *env; + ns_server_t *sctx; + isc_loop_t *loop; + isc_job_cb cb; + void *cbarg; + isc_result_t *sigresult; + isc_result_t *viewmatchresult; + isc_result_t quota_result; + dns_view_t **viewp; + dns_view_t *view; +} matching_view_ctx_t; + /*% * Configuration context to retain for each view that allows * new zones to be added at runtime. @@ -10059,19 +10079,19 @@ shutdown_server(void *arg) { isc_loopmgr_resume(named_g_loopmgr); } -/*% - * Find a view that matches the source and destination addresses of a query. - */ static isc_result_t -get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, - dns_message_t *message, dns_aclenv_t *env, ns_server_t *sctx, - isc_result_t *sigresult, dns_view_t **viewp) { +get_matching_view_sync(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, + dns_message_t *message, dns_aclenv_t *env, + isc_result_t *sigresult, dns_view_t **viewp) { dns_view_t *view; - REQUIRE(message != NULL); - REQUIRE(sctx != NULL); - REQUIRE(sigresult != NULL); - REQUIRE(viewp != NULL && *viewp == NULL); + /* + * We should not be running synchronous view matching if signature + * checking involves SIG(0). TSIG has priority of SIG(0), so if TSIG + * is set then we proceed anyway. + */ + INSIST(message->tsigkey != NULL || message->tsig != NULL || + message->sig0 == NULL); for (view = ISC_LIST_HEAD(named_g_server->viewlist); view != NULL; view = ISC_LIST_NEXT(view, link)) @@ -10080,48 +10100,9 @@ get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, message->rdclass == dns_rdataclass_any) { const dns_name_t *tsig = NULL; - int exempt_match; - isc_result_t sig0_qresult = ISC_R_UNSET; - if (message->sig0 != NULL) { - /* - * If the message has a SIG0 signature and the - * client is not exempt from the quota, then - * acquire a quota. If quota is reached, then - * return early. - */ - if (sctx->sig0checksquota_exempt != NULL) { - isc_result_t result = dns_acl_match( - srcaddr, NULL, - sctx->sig0checksquota_exempt, - env, &exempt_match, NULL); - if (result == ISC_R_SUCCESS && - exempt_match > 0) - { - sig0_qresult = ISC_R_EXISTS; - } - } - if (sig0_qresult == ISC_R_UNSET) { - sig0_qresult = isc_quota_acquire( - &sctx->sig0checksquota); - } - if (sig0_qresult == ISC_R_SOFTQUOTA) { - isc_quota_release( - &sctx->sig0checksquota); - } - if (sig0_qresult != ISC_R_SUCCESS && - sig0_qresult != ISC_R_EXISTS) - { - return (ISC_R_QUOTA); - } - } - - /* Check the signature, then release the quota */ dns_message_resetsig(message); *sigresult = dns_message_checksig(message, view); - if (sig0_qresult == ISC_R_SUCCESS) { - isc_quota_release(&sctx->sig0checksquota); - } if (*sigresult == ISC_R_SUCCESS) { tsig = dns_tsigkey_identity(message->tsigkey); } @@ -10142,6 +10123,191 @@ get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, return (ISC_R_NOTFOUND); } +static void +get_matching_view_done(void *cbarg) { + matching_view_ctx_t *mvctx = cbarg; + dns_message_t *message = mvctx->message; + + if (*mvctx->viewmatchresult == ISC_R_SUCCESS) { + INSIST(mvctx->view != NULL); + dns_view_attach(mvctx->view, mvctx->viewp); + } + + mvctx->cb(mvctx->cbarg); + + if (mvctx->quota_result == ISC_R_SUCCESS) { + isc_quota_release(&mvctx->sctx->sig0checksquota); + } + if (mvctx->view != NULL) { + dns_view_detach(&mvctx->view); + } + isc_loop_detach(&mvctx->loop); + ns_server_detach(&mvctx->sctx); + isc_mem_put(message->mctx, mvctx, sizeof(*mvctx)); + dns_message_detach(&message); +} + +static dns_view_t * +get_matching_view_next(dns_view_t *view, dns_rdataclass_t rdclass) { + if (view == NULL) { + view = ISC_LIST_HEAD(named_g_server->viewlist); + } else { + view = ISC_LIST_NEXT(view, link); + } + while (true) { + if (view == NULL || rdclass == view->rdclass || + rdclass == dns_rdataclass_any) + { + return (view); + } + view = ISC_LIST_NEXT(view, link); + }; +} + +static void +get_matching_view_continue(void *cbarg, isc_result_t result) { + matching_view_ctx_t *mvctx = cbarg; + dns_view_t *view = NULL; + const dns_name_t *tsig = NULL; + + *mvctx->sigresult = result; + + if (result == ISC_R_SUCCESS) { + tsig = dns_tsigkey_identity(mvctx->message->tsigkey); + } + + if (dns_acl_allowed(mvctx->srcaddr, tsig, mvctx->view->matchclients, + mvctx->env) && + dns_acl_allowed(mvctx->destaddr, tsig, + mvctx->view->matchdestinations, mvctx->env) && + !(mvctx->view->matchrecursiveonly && + (mvctx->message->flags & DNS_MESSAGEFLAG_RD) == 0)) + { + /* + * A matching view is found. + */ + *mvctx->viewmatchresult = ISC_R_SUCCESS; + get_matching_view_done(cbarg); + return; + } + + dns_message_resetsig(mvctx->message); + + view = get_matching_view_next(mvctx->view, mvctx->message->rdclass); + dns_view_detach(&mvctx->view); + if (view != NULL) { + /* + * Try the next view. + */ + dns_view_attach(view, &mvctx->view); + result = dns_message_checksig_async( + mvctx->message, view, mvctx->loop, + get_matching_view_continue, mvctx); + INSIST(result == DNS_R_WAIT); + return; + } + + /* + * No matching view is found. + */ + *mvctx->viewmatchresult = ISC_R_NOTFOUND; + get_matching_view_done(cbarg); +} + +/*% + * Find a view that matches the source and destination addresses of a query. + */ +static isc_result_t +get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, + dns_message_t *message, dns_aclenv_t *env, ns_server_t *sctx, + isc_loop_t *loop, isc_job_cb cb, void *cbarg, + isc_result_t *sigresult, isc_result_t *viewmatchresult, + dns_view_t **viewp) { + dns_view_t *view = NULL; + isc_result_t result; + + REQUIRE(message != NULL); + REQUIRE(sctx != NULL); + REQUIRE(loop == NULL || cb != NULL); + REQUIRE(sigresult != NULL); + REQUIRE(viewmatchresult != NULL); + REQUIRE(viewp != NULL && *viewp == NULL); + + /* No offloading is requested if the loop is unset. */ + if (loop == NULL) { + *viewmatchresult = get_matching_view_sync( + srcaddr, destaddr, message, env, sigresult, viewp); + return (*viewmatchresult); + } + + matching_view_ctx_t *mvctx = isc_mem_get(message->mctx, sizeof(*mvctx)); + *mvctx = (matching_view_ctx_t){ + .srcaddr = srcaddr, + .destaddr = destaddr, + .env = env, + .cb = cb, + .cbarg = cbarg, + .sigresult = sigresult, + .viewmatchresult = viewmatchresult, + .quota_result = ISC_R_UNSET, + .viewp = viewp, + }; + ns_server_attach(sctx, &mvctx->sctx); + isc_loop_attach(loop, &mvctx->loop); + dns_message_attach(message, &mvctx->message); + + dns_message_resetsig(message); + + view = get_matching_view_next(NULL, message->rdclass); + if (view == NULL) { + *mvctx->viewmatchresult = ISC_R_NOTFOUND; + isc_async_run(loop, get_matching_view_done, mvctx); + return (DNS_R_WAIT); + } + + /* + * If the message has a SIG0 signature which we are going to + * check, and the client is not exempt from the SIG(0) quota, + * then acquire a quota. TSIG has priority over SIG(0), so if + * TSIG is set then we don't care. + */ + if (message->tsigkey == NULL && message->tsig == NULL && + message->sig0 != NULL) + { + if (sctx->sig0checksquota_exempt != NULL) { + int exempt_match; + + result = dns_acl_match(srcaddr, NULL, + sctx->sig0checksquota_exempt, + env, &exempt_match, NULL); + if (result == ISC_R_SUCCESS && exempt_match > 0) { + mvctx->quota_result = ISC_R_EXISTS; + } + } + if (mvctx->quota_result == ISC_R_UNSET) { + mvctx->quota_result = + isc_quota_acquire(&sctx->sig0checksquota); + } + if (mvctx->quota_result == ISC_R_SOFTQUOTA) { + isc_quota_release(&sctx->sig0checksquota); + } + if (mvctx->quota_result != ISC_R_SUCCESS && + mvctx->quota_result != ISC_R_EXISTS) + { + *mvctx->viewmatchresult = ISC_R_QUOTA; + isc_async_run(loop, get_matching_view_done, mvctx); + return (DNS_R_WAIT); + } + } + + dns_view_attach(view, &mvctx->view); + result = dns_message_checksig_async(message, view, loop, + get_matching_view_continue, mvctx); + INSIST(result == DNS_R_WAIT); + + return (DNS_R_WAIT); +} + void named_server_create(isc_mem_t *mctx, named_server_t **serverp) { isc_result_t result; diff --git a/lib/ns/client.c b/lib/ns/client.c index 03afc4d25c..1e2229b95e 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1709,6 +1709,15 @@ ns__client_reset_cb(void *client0) { client->keytag_len = 0; } + isc_timer_stop(client->quotatimer); + + if (client->async) { + client->async = false; + if (client->handle != NULL) { + isc_nmhandle_unref(client->handle); + } + } + client->state = NS_CLIENTSTATE_READY; #ifdef WANT_SINGLETRACE @@ -1742,6 +1751,15 @@ ns__client_put_cb(void *client0) { dns_message_puttemprdataset(client->message, &client->opt); } + isc_timer_async_destroy(&client->quotatimer); + + if (client->async) { + client->async = false; + if (client->handle != NULL) { + isc_nmhandle_unref(client->handle); + } + } + dns_message_detach(&client->message); /* @@ -1759,46 +1777,32 @@ static isc_result_t ns_client_setup_view(ns_client_t *client, isc_netaddr_t *netaddr) { isc_result_t result; + client->sigresult = client->viewmatchresult = ISC_R_UNSET; + + if (client->async) { + isc_nmhandle_ref(client->handle); + } + result = client->manager->sctx->matchingview( netaddr, &client->destaddr, client->message, client->manager->aclenv, client->manager->sctx, - &client->sigresult, &client->view); - if (result != ISC_R_SUCCESS) { - if (result == ISC_R_QUOTA) { - if (can_log_sigchecks_quota()) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_INFO, - "SIG(0) checks quota reached"); - ns_client_dumpmessage( - client, "SIG(0) checks quota reached"); - } - } else { - char classname[DNS_RDATACLASS_FORMATSIZE]; - isc_buffer_t b; - isc_region_t *r; + client->async ? client->manager->loop : NULL, + ns_client_request_continue, client, &client->sigresult, + &client->viewmatchresult, &client->view); - /* - * Do a dummy TSIG verification attempt so that the - * response will have a TSIG if the query did, as - * required by RFC2845. - */ - dns_message_resetsig(client->message); - r = dns_message_getrawmessage(client->message); - isc_buffer_init(&b, r->base, r->length); - isc_buffer_add(&b, r->length); - (void)dns_tsig_verify(&b, client->message, NULL, NULL); - - dns_rdataclass_format(client->message->rdclass, - classname, sizeof(classname)); - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "no matching view in class '%s'", - classname); - ns_client_dumpmessage(client, - "no matching view in class"); - } + if (result == DNS_R_WAIT) { + INSIST(client->async == true); + return (DNS_R_WAIT); } + /* + * matchingview() is allowed to return anything other than DNS_R_WAIT + * only in non-async mode, in which case 'result' must be equal to + * 'client->viewmatchresult'. + */ + INSIST(client->async == false); + INSIST(result == client->viewmatchresult); + return (result); } @@ -2116,19 +2120,16 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, client->destsockaddr = isc_nmhandle_localaddr(handle); isc_netaddr_fromsockaddr(&client->destaddr, &client->destsockaddr); + /* + * Offload view matching only if we are going to check a SIG(0) + * signature. + */ + client->async = (client->message->tsigkey == NULL && + client->message->tsig == NULL && + client->message->sig0 != NULL); + result = ns_client_setup_view(client, &netaddr); - if (result == ISC_R_QUOTA) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), - "client is starting to wait for quota"); - client->async = true; - isc_nmhandle_ref(client->handle); - isc_async_run(client->manager->loop, ns_client_request_continue, - client); - return; - } else if (result != ISC_R_SUCCESS) { - ns_client_extendederror(client, DNS_EDE_PROHIBITED, NULL); - ns_client_error(client, notimp ? DNS_R_NOTIMP : DNS_R_REFUSED); + if (result == DNS_R_WAIT) { return; } @@ -2141,7 +2142,7 @@ ns_client_request_continue(void *arg) { isc_netaddr_t netaddr; const dns_name_t *signame = NULL; bool ra; /* Recursion available. */ - isc_result_t result; + isc_result_t result = ISC_R_UNSET; static const char *ra_reasons[] = { "ACLs not processed yet", "no resolver in view", @@ -2165,19 +2166,24 @@ ns_client_request_continue(void *arg) { dns_dtmsgtype_t dtmsgtype; #endif /* ifdef HAVE_DNSTAP */ + INSIST(client->viewmatchresult != ISC_R_UNSET); + + isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); + /* - * This function could be running asynchronously if a quota was reached - * before, and named was waiting for available quota. In that case we + * This function could be running asynchronously when validating a + * SIG(0) signature or waiting for a quota for it. In that case we * need to update the current 'now', and check that named doesn't wait * for too long. */ if (client->async) { - uint64_t wait_us; - uint64_t maxwait_us; - client->tnow = isc_time_now(); client->now = isc_time_seconds(&client->tnow); + } + if (client->async && client->viewmatchresult == ISC_R_QUOTA) { + uint64_t wait_us, maxwait_us; + /* Waiting for a quota. */ wait_us = isc_time_microdiff(&client->tnow, &client->requesttime); maxwait_us = US_PER_MS * @@ -2206,27 +2212,53 @@ ns_client_request_continue(void *arg) { ns_client_error(client, DNS_R_REFUSED); goto cleanup; } + + if (can_log_sigchecks_quota()) { + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_INFO, + "SIG(0) checks quota reached"); + ns_client_dumpmessage(client, + "SIG(0) checks quota reached"); + } + + /* Retry after 10 milliseconds. */ + isc_interval_t interval; + isc_interval_set(&interval, 0, 10 * NS_PER_MS); + isc_nmhandle_ref(client->handle); + isc_timer_start(client->quotatimer, isc_timertype_once, + &interval); + + result = DNS_R_WAIT; + goto cleanup; } - isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); + if (client->viewmatchresult != ISC_R_SUCCESS) { + char classname[DNS_RDATACLASS_FORMATSIZE]; + isc_buffer_t b; + isc_region_t *r; - if (client->view == NULL) { - result = ns_client_setup_view(client, &netaddr); - if (result == ISC_R_QUOTA) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), - "client continues waiting for quota"); - client->async = true; - isc_nmhandle_ref(client->handle); - isc_async_run(client->manager->loop, - ns_client_request_continue, client); - goto cleanup; - } else if (result != ISC_R_SUCCESS) { - ns_client_extendederror(client, DNS_EDE_PROHIBITED, - NULL); - ns_client_error(client, DNS_R_REFUSED); - goto cleanup; - } + /* + * Do a dummy TSIG verification attempt so that the + * response will have a TSIG if the query did, as + * required by RFC2845. + */ + dns_message_resetsig(client->message); + r = dns_message_getrawmessage(client->message); + isc_buffer_init(&b, r->base, r->length); + isc_buffer_add(&b, r->length); + (void)dns_tsig_verify(&b, client->message, NULL, NULL); + + dns_rdataclass_format(client->message->rdclass, classname, + sizeof(classname)); + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), + "no matching view in class '%s'", classname); + ns_client_dumpmessage(client, "no matching view in class"); + + ns_client_extendederror(client, DNS_EDE_PROHIBITED, NULL); + ns_client_error(client, DNS_R_REFUSED); + + goto cleanup; } if (isc_nm_is_proxy_handle(client->handle)) { @@ -2487,12 +2519,19 @@ ns_client_request_continue(void *arg) { } cleanup: + + /* + * If we are running async then 'unref' the handle, and also if there + * are no more async calls scheduled the reset the async flag, so that + * the destructor doesn't try to 'unref' the handle too. + */ if (client->async) { - /* - * Do not detach, only 'unref' the corresponding 'ref' when - * async was used, because the client can still be reused. - */ - isc_nmhandle_unref(client->handle); + if (client->handle != NULL) { + isc_nmhandle_unref(client->handle); + } + if (result != DNS_R_WAIT) { + client->async = false; + } } } @@ -2530,6 +2569,18 @@ ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) { return (ISC_R_SUCCESS); } +static void +client_quotatimer_event(void *arg) { + ns_client_t *client = arg; + isc_netaddr_t netaddr; + isc_result_t result; + + isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); + result = ns_client_setup_view(client, &netaddr); + INSIST(result == DNS_R_WAIT); /* We are in async mode. */ + isc_nmhandle_unref(client->handle); +} + isc_result_t ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { isc_result_t result; @@ -2553,6 +2604,9 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { client->manager->rdspool, DNS_MESSAGE_INTENTPARSE, &client->message); + isc_timer_create(client->manager->loop, client_quotatimer_event, + client, &client->quotatimer); + /* * Set magic earlier than usual because ns_query_init() * and the functions it calls will require it. @@ -2574,6 +2628,7 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { .magic = 0, .manager = client->manager, .message = client->message, + .quotatimer = client->quotatimer, .query = client->query, }; } @@ -2597,6 +2652,7 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { return (ISC_R_SUCCESS); cleanup: + isc_timer_destroy(&client->quotatimer); dns_message_detach(&client->message); ns_clientmgr_detach(&client->manager); diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 39c1fa0a86..14acb4297c 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -194,6 +194,7 @@ struct ns_client { dns_name_t signername; /*%< [T]SIG key name */ dns_name_t *signer; /*%< NULL if not valid sig */ isc_result_t sigresult; + isc_result_t viewmatchresult; isc_buffer_t *buffer; isc_buffer_t tbuffer; @@ -202,6 +203,8 @@ struct ns_client { isc_netaddr_t destaddr; isc_sockaddr_t destsockaddr; + isc_timer_t *quotatimer; + dns_ecs_t ecs; /*%< EDNS client subnet sent by client */ /*% diff --git a/lib/ns/include/ns/server.h b/lib/ns/include/ns/server.h index fae2040b74..9acf83a966 100644 --- a/lib/ns/include/ns/server.h +++ b/lib/ns/include/ns/server.h @@ -64,12 +64,11 @@ typedef void (*ns_fuzzcb_t)(void); /*% * Type for callback function to get the view that can answer a query. */ -typedef isc_result_t (*ns_matchview_t)(isc_netaddr_t *srcaddr, - isc_netaddr_t *destaddr, - dns_message_t *message, - dns_aclenv_t *env, ns_server_t *sctx, - isc_result_t *sigresultp, - dns_view_t **viewp); +typedef isc_result_t (*ns_matchview_t)( + isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, dns_message_t *message, + dns_aclenv_t *env, ns_server_t *sctx, isc_loop_t *loop, isc_job_cb cb, + void *cbarg, isc_result_t *sigresultp, isc_result_t *viewmatchresult, + dns_view_t **viewp); /*% * Server context. diff --git a/tests/libtest/ns.c b/tests/libtest/ns.c index f09b20045c..980ab6b904 100644 --- a/tests/libtest/ns.c +++ b/tests/libtest/ns.c @@ -58,15 +58,21 @@ ns_server_t *sctx = NULL; static isc_result_t matchview(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, dns_message_t *message, dns_aclenv_t *env, ns_server_t *lsctx, - isc_result_t *sigresultp, dns_view_t **viewp) { + isc_loop_t *loop, isc_job_cb cb, void *cbarg, + isc_result_t *sigresultp, isc_result_t *viewmatchresultp, + dns_view_t **viewp) { UNUSED(srcaddr); UNUSED(destaddr); UNUSED(message); UNUSED(env); UNUSED(lsctx); + UNUSED(loop); + UNUSED(cb); + UNUSED(cbarg); UNUSED(sigresultp); UNUSED(viewp); + *viewmatchresultp = ISC_R_NOTIMPLEMENTED; return (ISC_R_NOTIMPLEMENTED); } From ad489c44dff449ea1ef01286fa32d6a0e2326420 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 9 May 2024 17:06:14 +0000 Subject: [PATCH 06/14] Remove sig0checks-quota-maxwait-ms support Waiting for a quota to appear complicates things and wastes rosources on timer management. Just answer with REFUSE if there is no quota. --- bin/named/config.c | 1 - bin/named/server.c | 6 -- doc/arm/reference.rst | 24 +------ doc/misc/options | 1 - lib/isccfg/namedconf.c | 1 - lib/ns/client.c | 125 ++++++++++--------------------------- lib/ns/include/ns/client.h | 2 - lib/ns/include/ns/server.h | 1 - 8 files changed, 36 insertions(+), 125 deletions(-) diff --git a/bin/named/config.c b/bin/named/config.c index 62c05a8314..3ad700a887 100644 --- a/bin/named/config.c +++ b/bin/named/config.c @@ -110,7 +110,6 @@ options {\n\ session-keyname local-ddns;\n\ startup-notify-rate 20;\n\ sig0checks-quota 1;\n\ - sig0checks-quota-maxwait-ms 1500;\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 a4c3269432..dc16659341 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -8463,12 +8463,6 @@ load_configuration(const char *filename, named_server_t *server, INSIST(result == ISC_R_SUCCESS); } - obj = NULL; - result = named_config_get(maps, "sig0checks-quota-maxwait-ms", &obj); - if (result == ISC_R_SUCCESS) { - server->sctx->sig0checksquota_maxwaitms = cfg_obj_asuint32(obj); - } - /* * Set "blackhole". Only legal at options level; there is * no default. diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index b0c593e4b4..1b782b976d 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4005,27 +4005,9 @@ system. :short: Specifies the maximum number of concurrent SIG(0) signature checks that can be processed by the server. This is the maximum number of simultaneous SIG(0)-signed messages that - the server will accept. If the quota is reached, then :iscman:`named` waits - for the maximum of :any:`sig0checks-quota-maxwait-ms` time for a quota to - appear or to answer with a status code of REFUSED. The value of ``0`` - disables the quota. The default is ``1``. - - .. note:: - - :any:`sig0checks-quota` protection does not work when there is only one - worker thread available, or when the option is set to a value that is - greater or equal to the worker threads available. See the ``-n #cpus`` - option of :iscman:`named` for more information about the worker threads. - -.. namedconf:statement:: sig0checks-quota-maxwait-ms - :tags: server - :short: Specifies the maximum number of milliseconds to wait for a SIG(0) signature checking quota to appear. - - When :any:`sig0checks-quota` is effective and a client reaches the quota, - then :iscman:`named` waits for the maximum of - :any:`sig0checks-quota-maxwait-ms` time (in milliseconds) for a quota to - appear. If no quota becomes available, then an answer with a status code of - REFUSED is sent. The default is ``1500``. + the server accepts. If the quota is reached, then :iscman:`named` answers + with a status code of REFUSED. The value of ``0`` disables the quota. The + default is ``1``. .. namedconf:statement:: sig0checks-quota-exempt :tags: server diff --git a/doc/misc/options b/doc/misc/options index d003e9e741..0aec7c5dea 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -279,7 +279,6 @@ options { sig-validity-interval [ ]; // obsolete sig0checks-quota ; sig0checks-quota-exempt { ; ... }; - sig0checks-quota-maxwait-ms ; sortlist { ; ... }; // deprecated stale-answer-client-timeout ( disabled | off | ); stale-answer-enable ; diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index 3a27432f7e..b9f8500d7a 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -1362,7 +1362,6 @@ static cfg_clausedef_t options_clauses[] = { { "session-keyfile", &cfg_type_qstringornone, 0 }, { "session-keyname", &cfg_type_astring, 0 }, { "sig0checks-quota", &cfg_type_uint32, 0 }, - { "sig0checks-quota-maxwait-ms", &cfg_type_uint32, 0 }, { "sig0checks-quota-exempt", &cfg_type_bracketed_aml, 0 }, { "sit-secret", NULL, CFG_CLAUSEFLAG_ANCIENT }, { "stacksize", &cfg_type_size, CFG_CLAUSEFLAG_ANCIENT }, diff --git a/lib/ns/client.c b/lib/ns/client.c index 1e2229b95e..8d91ad07d0 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1709,8 +1709,6 @@ ns__client_reset_cb(void *client0) { client->keytag_len = 0; } - isc_timer_stop(client->quotatimer); - if (client->async) { client->async = false; if (client->handle != NULL) { @@ -1751,8 +1749,6 @@ ns__client_put_cb(void *client0) { dns_message_puttemprdataset(client->message, &client->opt); } - isc_timer_async_destroy(&client->quotatimer); - if (client->async) { client->async = false; if (client->handle != NULL) { @@ -2139,7 +2135,6 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, static void ns_client_request_continue(void *arg) { ns_client_t *client = arg; - isc_netaddr_t netaddr; const dns_name_t *signame = NULL; bool ra; /* Recursion available. */ isc_result_t result = ISC_R_UNSET; @@ -2168,72 +2163,16 @@ ns_client_request_continue(void *arg) { INSIST(client->viewmatchresult != ISC_R_UNSET); - isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); - /* - * This function could be running asynchronously when validating a - * SIG(0) signature or waiting for a quota for it. In that case we - * need to update the current 'now', and check that named doesn't wait - * for too long. + * This function could be running asynchronously, in which case update + * the current 'now' for correct timekeeping. */ if (client->async) { client->tnow = isc_time_now(); client->now = isc_time_seconds(&client->tnow); } - if (client->async && client->viewmatchresult == ISC_R_QUOTA) { - uint64_t wait_us, maxwait_us; - - /* Waiting for a quota. */ - wait_us = isc_time_microdiff(&client->tnow, - &client->requesttime); - maxwait_us = US_PER_MS * - client->manager->sctx->sig0checksquota_maxwaitms; - if (wait_us > maxwait_us) { - isc_buffer_t b; - isc_region_t *r; - - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), - "client reached max wait time for quota"); - - /* - * Do a dummy TSIG verification attempt so that the - * response will have a TSIG if the query did, as - * required by RFC2845. - */ - dns_message_resetsig(client->message); - r = dns_message_getrawmessage(client->message); - isc_buffer_init(&b, r->base, r->length); - isc_buffer_add(&b, r->length); - (void)dns_tsig_verify(&b, client->message, NULL, NULL); - - ns_client_extendederror(client, DNS_EDE_PROHIBITED, - NULL); - ns_client_error(client, DNS_R_REFUSED); - goto cleanup; - } - - if (can_log_sigchecks_quota()) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_INFO, - "SIG(0) checks quota reached"); - ns_client_dumpmessage(client, - "SIG(0) checks quota reached"); - } - - /* Retry after 10 milliseconds. */ - isc_interval_t interval; - isc_interval_set(&interval, 0, 10 * NS_PER_MS); - isc_nmhandle_ref(client->handle); - isc_timer_start(client->quotatimer, isc_timertype_once, - &interval); - - result = DNS_R_WAIT; - goto cleanup; - } if (client->viewmatchresult != ISC_R_SUCCESS) { - char classname[DNS_RDATACLASS_FORMATSIZE]; isc_buffer_t b; isc_region_t *r; @@ -2248,12 +2187,31 @@ ns_client_request_continue(void *arg) { isc_buffer_add(&b, r->length); (void)dns_tsig_verify(&b, client->message, NULL, NULL); - dns_rdataclass_format(client->message->rdclass, classname, - sizeof(classname)); - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "no matching view in class '%s'", classname); - ns_client_dumpmessage(client, "no matching view in class"); + if (client->viewmatchresult == ISC_R_QUOTA) { + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(5), + "SIG(0) checks quota reached"); + + if (can_log_sigchecks_quota()) { + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_INFO, + "SIG(0) checks quota reached"); + ns_client_dumpmessage( + client, "SIG(0) checks quota reached"); + } + } else { + char classname[DNS_RDATACLASS_FORMATSIZE]; + + dns_rdataclass_format(client->message->rdclass, + classname, sizeof(classname)); + + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), + "no matching view in class '%s'", + classname); + ns_client_dumpmessage(client, + "no matching view in class"); + } ns_client_extendederror(client, DNS_EDE_PROHIBITED, NULL); ns_client_error(client, DNS_R_REFUSED); @@ -2460,6 +2418,9 @@ ns_client_request_continue(void *arg) { if (client->udpsize > 512) { dns_peer_t *peer = NULL; uint16_t udpsize = client->view->maxudp; + isc_netaddr_t netaddr; + + isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); (void)dns_peerlist_peerbyaddr(client->view->peers, &netaddr, &peer); if (peer != NULL) { @@ -2521,17 +2482,14 @@ ns_client_request_continue(void *arg) { cleanup: /* - * If we are running async then 'unref' the handle, and also if there - * are no more async calls scheduled the reset the async flag, so that - * the destructor doesn't try to 'unref' the handle too. + * If we are running async then 'unref' the handle and reset the async + * flag, so that the destructor doesn't try to 'unref' the handle too. */ if (client->async) { + client->async = false; if (client->handle != NULL) { isc_nmhandle_unref(client->handle); } - if (result != DNS_R_WAIT) { - client->async = false; - } } } @@ -2569,18 +2527,6 @@ ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) { return (ISC_R_SUCCESS); } -static void -client_quotatimer_event(void *arg) { - ns_client_t *client = arg; - isc_netaddr_t netaddr; - isc_result_t result; - - isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); - result = ns_client_setup_view(client, &netaddr); - INSIST(result == DNS_R_WAIT); /* We are in async mode. */ - isc_nmhandle_unref(client->handle); -} - isc_result_t ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { isc_result_t result; @@ -2604,9 +2550,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { client->manager->rdspool, DNS_MESSAGE_INTENTPARSE, &client->message); - isc_timer_create(client->manager->loop, client_quotatimer_event, - client, &client->quotatimer); - /* * Set magic earlier than usual because ns_query_init() * and the functions it calls will require it. @@ -2628,7 +2571,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { .magic = 0, .manager = client->manager, .message = client->message, - .quotatimer = client->quotatimer, .query = client->query, }; } @@ -2652,7 +2594,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { return (ISC_R_SUCCESS); cleanup: - isc_timer_destroy(&client->quotatimer); dns_message_detach(&client->message); ns_clientmgr_detach(&client->manager); diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 14acb4297c..1213c6ec3b 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -203,8 +203,6 @@ struct ns_client { isc_netaddr_t destaddr; isc_sockaddr_t destsockaddr; - isc_timer_t *quotatimer; - dns_ecs_t ecs; /*%< EDNS client subnet sent by client */ /*% diff --git a/lib/ns/include/ns/server.h b/lib/ns/include/ns/server.h index 9acf83a966..fc01ee2c62 100644 --- a/lib/ns/include/ns/server.h +++ b/lib/ns/include/ns/server.h @@ -91,7 +91,6 @@ struct ns_server { isc_quota_t xfroutquota; isc_quota_t updquota; isc_quota_t sig0checksquota; - uint32_t sig0checksquota_maxwaitms; dns_acl_t *sig0checksquota_exempt; ISC_LIST(isc_quota_t) http_quotas; isc_mutex_t http_quotas_lock; From 70ff4a3f85f0df98cc4db5462f3ea6be0d76e584 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 6 Mar 2024 11:08:52 +0000 Subject: [PATCH 07/14] Run resolver message signature checking asynchronously --- lib/dns/resolver.c | 202 +++++++++++++++++++++++++++------------------ 1 file changed, 120 insertions(+), 82 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index f984dbef67..0c8c85ffeb 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -645,6 +645,8 @@ resquery_send(resquery_t *query); static void resquery_response(isc_result_t eresult, isc_region_t *region, void *arg); static void +resquery_response_continue(void *arg, isc_result_t result); +static void resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg); static void fctx_try(fetchctx_t *fctx, bool retrying, bool badcache); @@ -777,6 +779,7 @@ release_fctx(fetchctx_t *fctx); typedef struct respctx { resquery_t *query; fetchctx_t *fctx; + isc_mem_t *mctx; isc_result_t result; isc_buffer_t buffer; unsigned int retryopts; /* updated options to pass to @@ -7233,7 +7236,7 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { isc_result_t result; resquery_t *query = (resquery_t *)arg; fetchctx_t *fctx = NULL; - respctx_t rctx; + respctx_t *rctx = NULL; if (eresult == ISC_R_CANCELED) { return; @@ -7252,21 +7255,22 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { inc_stats(fctx->res, dns_resstatscounter_responsev6); } - rctx_respinit(query, fctx, eresult, region, &rctx); + rctx = isc_mem_get(fctx->mctx, sizeof(*rctx)); + rctx_respinit(query, fctx, eresult, region, rctx); if (eresult == ISC_R_SHUTTINGDOWN || atomic_load_acquire(&fctx->res->exiting)) { result = ISC_R_SHUTTINGDOWN; FCTXTRACE("resolver shutting down"); - rctx.finish = NULL; - rctx_done(&rctx, result); - return; + rctx->finish = NULL; + rctx_done(rctx, result); + goto cleanup; } - result = rctx_timedout(&rctx); + result = rctx_timedout(rctx); if (result == ISC_R_COMPLETE) { - return; + goto cleanup; } fctx->addrinfo = query->addrinfo; @@ -7276,9 +7280,9 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { /* * Check whether the dispatcher has failed; if so we're done */ - result = rctx_dispfail(&rctx); + result = rctx_dispfail(rctx); if (result == ISC_R_COMPLETE) { - return; + goto cleanup; } if (query->tsig != NULL) { @@ -7290,17 +7294,18 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { query->tsigkey); if (result != ISC_R_SUCCESS) { FCTXTRACE3("unable to set tsig key", result); - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } } dns_message_setclass(query->rmessage, fctx->res->rdclass); - if ((rctx.retryopts & DNS_FETCHOPT_TCP) == 0) { - if ((rctx.retryopts & DNS_FETCHOPT_NOEDNS0) == 0) { - dns_adb_setudpsize(fctx->adb, query->addrinfo, - isc_buffer_usedlength(&rctx.buffer)); + if ((rctx->retryopts & DNS_FETCHOPT_TCP) == 0) { + if ((rctx->retryopts & DNS_FETCHOPT_NOEDNS0) == 0) { + dns_adb_setudpsize( + fctx->adb, query->addrinfo, + isc_buffer_usedlength(&rctx->buffer)); } else { dns_adb_plainresponse(fctx->adb, query->addrinfo); } @@ -7309,38 +7314,39 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { /* * Parse response message. */ - result = rctx_parse(&rctx); + result = rctx_parse(rctx); if (result == ISC_R_COMPLETE) { - return; + goto cleanup; } /* * Log the incoming packet. */ - rctx_logpacket(&rctx); + rctx_logpacket(rctx); if (query->rmessage->rdclass != fctx->res->rdclass) { - rctx.resend = true; + rctx->resend = true; FCTXTRACE("bad class"); - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } /* * Process receive opt record. */ - rctx.opt = dns_message_getopt(query->rmessage); - if (rctx.opt != NULL) { - rctx_opt(&rctx); + rctx->opt = dns_message_getopt(query->rmessage); + if (rctx->opt != NULL) { + rctx_opt(rctx); } - if (query->rmessage->cc_bad && (rctx.retryopts & DNS_FETCHOPT_TCP) == 0) + if (query->rmessage->cc_bad && + (rctx->retryopts & DNS_FETCHOPT_TCP) == 0) { /* * If the COOKIE is bad, assume it is an attack and * keep listening for a good answer. */ - rctx.nextitem = true; + rctx->nextitem = true; if (isc_log_wouldlog(dns_lctx, ISC_LOG_INFO)) { char addrbuf[ISC_SOCKADDR_FORMATSIZE]; isc_sockaddr_format(&query->addrinfo->sockaddr, addrbuf, @@ -7349,8 +7355,8 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { DNS_LOGMODULE_RESOLVER, ISC_LOG_INFO, "bad cookie from %s", addrbuf); } - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } /* @@ -7377,27 +7383,55 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { result = same_question(fctx, query->rmessage); if (result != ISC_R_SUCCESS) { FCTXTRACE3("question section invalid", result); - rctx.nextitem = true; - rctx_done(&rctx, result); - return; + rctx->nextitem = true; + rctx_done(rctx, result); + goto cleanup; } break; } - /* - * If the message is signed, check the signature. If not, this - * returns success anyway. - */ - result = dns_message_checksig(query->rmessage, fctx->res->view); + if (query->rmessage->tsigkey == NULL && query->rmessage->tsig == NULL && + query->rmessage->sig0 != NULL) + { + /* + * If the message is not TSIG-signed (which has priorty) and is + * SIG(0)-signed (which consumes more resources), then run an + * asynchronous check. + */ + result = dns_message_checksig_async( + query->rmessage, fctx->res->view, fctx->loop, + resquery_response_continue, rctx); + INSIST(result == DNS_R_WAIT); + } else { + /* + * If the message is signed, check the signature. If not, this + * returns success anyway. + */ + result = dns_message_checksig(query->rmessage, fctx->res->view); + resquery_response_continue(rctx, result); + } + + return; + +cleanup: + isc_mem_putanddetach(&rctx->mctx, rctx, sizeof(*rctx)); +} + +static void +resquery_response_continue(void *arg, isc_result_t result) { + respctx_t *rctx = arg; + fetchctx_t *fctx = rctx->fctx; + resquery_t *query = rctx->query; + if (result != ISC_R_SUCCESS) { FCTXTRACE3("signature check failed", result); if (result == DNS_R_UNEXPECTEDTSIG || result == DNS_R_EXPECTEDTSIG) { - rctx.nextitem = true; + rctx->nextitem = true; } - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } /* @@ -7415,7 +7449,7 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { */ if (dns_message_gettsig(query->rmessage, NULL) == NULL && !query->rmessage->cc_ok && !query->rmessage->cc_bad && - (rctx.retryopts & DNS_FETCHOPT_TCP) == 0) + (rctx->retryopts & DNS_FETCHOPT_TCP) == 0) { if (dns_adb_getcookie(query->addrinfo, NULL, 0) > CLIENT_COOKIE_SIZE) @@ -7431,10 +7465,10 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { "from %s", addrbuf); } - rctx.retryopts |= DNS_FETCHOPT_TCP; - rctx.resend = true; - rctx_done(&rctx, result); - return; + rctx->retryopts |= DNS_FETCHOPT_TCP; + rctx->resend = true; + rctx_done(rctx, result); + goto cleanup; } else if (fctx->res->view->peers != NULL) { dns_peer_t *peer = NULL; isc_netaddr_t netaddr; @@ -7467,47 +7501,47 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { "from %s", addrbuf); } - rctx.retryopts |= DNS_FETCHOPT_TCP; - rctx.resend = true; - rctx_done(&rctx, result); - return; + rctx->retryopts |= DNS_FETCHOPT_TCP; + rctx->resend = true; + rctx_done(rctx, result); + goto cleanup; } } } } - rctx_edns(&rctx); + rctx_edns(rctx); /* * Deal with truncated responses by retrying using TCP. */ if ((query->rmessage->flags & DNS_MESSAGEFLAG_TC) != 0) { - rctx.truncated = true; + rctx->truncated = true; } - if (rctx.truncated) { + if (rctx->truncated) { inc_stats(fctx->res, dns_resstatscounter_truncated); - if ((rctx.retryopts & DNS_FETCHOPT_TCP) != 0) { - rctx.broken_server = DNS_R_TRUNCATEDTCP; - rctx.next_server = true; + if ((rctx->retryopts & DNS_FETCHOPT_TCP) != 0) { + rctx->broken_server = DNS_R_TRUNCATEDTCP; + rctx->next_server = true; } else { - rctx.retryopts |= DNS_FETCHOPT_TCP; - rctx.resend = true; + rctx->retryopts |= DNS_FETCHOPT_TCP; + rctx->resend = true; } FCTXTRACE3("message truncated", result); - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } /* * Is it a query response? */ if (query->rmessage->opcode != dns_opcode_query) { - rctx.broken_server = DNS_R_UNEXPECTEDOPCODE; - rctx.next_server = true; + rctx->broken_server = DNS_R_UNEXPECTEDOPCODE; + rctx->next_server = true; FCTXTRACE("invalid message opcode"); - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } /* @@ -7543,17 +7577,17 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { /* * Bad server? */ - result = rctx_badserver(&rctx, result); + result = rctx_badserver(rctx, result); if (result == ISC_R_COMPLETE) { - return; + goto cleanup; } /* * Lame server? */ - result = rctx_lameserver(&rctx); + result = rctx_lameserver(rctx); if (result == ISC_R_COMPLETE) { - return; + goto cleanup; } /* @@ -7578,9 +7612,9 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { query->rmessage->rcode == dns_rcode_yxdomain || query->rmessage->rcode == dns_rcode_nxdomain)) { - result = rctx_answer(&rctx); + result = rctx_answer(rctx); if (result == ISC_R_COMPLETE) { - return; + goto cleanup; } } else if (query->rmessage->counts[DNS_SECTION_AUTHORITY] > 0 || query->rmessage->rcode == dns_rcode_noerror || @@ -7590,7 +7624,7 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { * This might be an NXDOMAIN, NXRRSET, or referral. * Call rctx_answer_none() to determine which it is. */ - result = rctx_answer_none(&rctx); + result = rctx_answer_none(rctx); switch (result) { case ISC_R_SUCCESS: case DNS_R_CHASEDSSERVERS: @@ -7609,28 +7643,28 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { * Something has gone wrong. */ if (result == DNS_R_FORMERR) { - rctx.next_server = true; + rctx->next_server = true; } FCTXTRACE3("rctx_answer_none", result); - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } } else { /* * The server is insane. */ /* XXXRTH Log */ - rctx.broken_server = DNS_R_UNEXPECTEDRCODE; - rctx.next_server = true; + rctx->broken_server = DNS_R_UNEXPECTEDRCODE; + rctx->next_server = true; FCTXTRACE("broken server: unexpected rcode"); - rctx_done(&rctx, result); - return; + rctx_done(rctx, result); + goto cleanup; } /* * Follow additional section data chains. */ - rctx_additional(&rctx); + rctx_additional(rctx); /* * Cache the cacheable parts of the message. This may also @@ -7639,21 +7673,24 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { if (WANTCACHE(fctx)) { isc_result_t tresult; tresult = cache_message(fctx, query->rmessage, query->addrinfo, - rctx.now); + rctx->now); if (tresult != ISC_R_SUCCESS) { FCTXTRACE3("cache_message complete", tresult); - rctx_done(&rctx, tresult); - return; + rctx_done(rctx, tresult); + goto cleanup; } } /* * Negative caching */ - rctx_ncache(&rctx); + rctx_ncache(rctx); FCTXTRACE("resquery_response done"); - rctx_done(&rctx, result); + rctx_done(rctx, result); + +cleanup: + isc_mem_putanddetach(&rctx->mctx, rctx, sizeof(*rctx)); } /* @@ -7680,6 +7717,7 @@ rctx_respinit(resquery_t *query, fetchctx_t *fctx, isc_result_t result, rctx->tnow = isc_time_now(); rctx->finish = &rctx->tnow; rctx->now = (isc_stdtime_t)isc_time_seconds(&rctx->tnow); + isc_mem_attach(fctx->mctx, &rctx->mctx); } /* From 7ca9bd6014a8a2962a281a64c512358454d753f8 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 15 May 2024 12:57:56 +0000 Subject: [PATCH 08/14] Limit the number of keys for SIG(0) message verification Check at most two KEY RRs agains a SIG(0) signature. This should limit potential abuse and at the same time allow key rollover. --- lib/dns/message.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/dns/message.c b/lib/dns/message.c index bba68efe54..97ff5ecf18 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3286,6 +3286,12 @@ 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; result = dns_rdataset_first(msg->sig0); INSIST(result == ISC_R_SUCCESS); @@ -3327,8 +3333,9 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { } result = dns_rdataset_first(&keyset); INSIST(result == ISC_R_SUCCESS); - for (; result == ISC_R_SUCCESS; - result = dns_rdataset_next(&keyset)) + + for (n = 0; result == ISC_R_SUCCESS && n < max_keys; + n++, result = dns_rdataset_next(&keyset)) { dst_key_t *key = NULL; @@ -3356,7 +3363,7 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { break; } } - if (result == ISC_R_NOMORE) { + if (result == ISC_R_NOMORE || n == max_keys) { result = DNS_R_KEYUNAUTHORIZED; } From 3bb9241bec4f55b83e1b07c02de2ba2f3f912bd2 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 27 Mar 2024 14:59:37 +0000 Subject: [PATCH 09/14] Add a CHANGES note for [GL #4480] --- CHANGES | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGES b/CHANGES index c8e043e514..828f26d211 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,13 @@ +6402. [security] A malicious DNS client that sends many queries with a + SIG(0)-signed message can cause the server to respond + slowly or not respond at all to other clients. Use the + offload threadpool for SIG(0) signature verifications, + add the 'sig0checks-quota' configuration option to + introduce a quota for SIG(0)-signed queries running in + parallel and add the 'sig0checks-quota-exempt' option to + exempt certain clients by their IP/network addresses. + (CVE-2024-1975) [GL #4480] + 6401. [security] An excessively large number of rrtypes per owner can slow down database query processing, so a limit has been placed on the number of rrtypes that can be stored per From be482311deee68183cd52fbc7f085773a1d86779 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 27 Mar 2024 14:59:57 +0000 Subject: [PATCH 10/14] Add a release note for [GL #4480] --- doc/notes/notes-current.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 88b0fd75fa..5c3a70da38 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -34,6 +34,10 @@ Security Fixes ISC would like to thank Toshifumi Sakaguchi who independently discovered and responsibly reported the issue to ISC. :gl:`#4548` +- A malicious DNS client that sends many queries with a SIG(0)-signed message + can cause server to respond slowly or not respond at all for other clients. + :cve:`2024-1975` :gl:`#4480` + New Features ~~~~~~~~~~~~ From a2b61c0a65c4e034c39632cc23a7a689a7443fd3 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 22 May 2024 13:07:21 +0000 Subject: [PATCH 11/14] Test that named checks maximum two keys for SIG(0)-signed messages Send three updates with three different keys, and expect that one of them should fail. Also retain more artifacts for neighboring nsupdate calls. --- bin/tests/system/upforwd/clean.sh | 1 + bin/tests/system/upforwd/ns1/named.conf.in | 6 +++++ bin/tests/system/upforwd/ns3/named1.conf.in | 7 ++++++ bin/tests/system/upforwd/setup.sh | 11 ++++++++- bin/tests/system/upforwd/tests.sh | 26 +++++++++++++++++++-- 5 files changed, 48 insertions(+), 3 deletions(-) diff --git a/bin/tests/system/upforwd/clean.sh b/bin/tests/system/upforwd/clean.sh index 14b9cc6a44..d56c942069 100644 --- a/bin/tests/system/upforwd/clean.sh +++ b/bin/tests/system/upforwd/clean.sh @@ -29,6 +29,7 @@ rm -f */ans.run rm -f Ksig0.example2.* rm -f keyname keyname.err rm -f ns1/example2.db +rm -f ns1/example2-toomanykeys.db rm -f ns*/managed-keys.bind* rm -f nsupdate.out.* rm -f ns*/named.run.prev diff --git a/bin/tests/system/upforwd/ns1/named.conf.in b/bin/tests/system/upforwd/ns1/named.conf.in index 79d66c2fa4..2c25a371de 100644 --- a/bin/tests/system/upforwd/ns1/named.conf.in +++ b/bin/tests/system/upforwd/ns1/named.conf.in @@ -44,6 +44,12 @@ zone "example2" { allow-update { key sig0.example2.; }; }; +zone "example2-toomanykeys" { + type primary; + file "example2-toomanykeys.db"; + allow-update { key sig0.example2-toomanykeys.; }; +}; + zone "example3" { type primary; file "example3.db"; diff --git a/bin/tests/system/upforwd/ns3/named1.conf.in b/bin/tests/system/upforwd/ns3/named1.conf.in index df51f7bb2c..95feb796aa 100644 --- a/bin/tests/system/upforwd/ns3/named1.conf.in +++ b/bin/tests/system/upforwd/ns3/named1.conf.in @@ -56,6 +56,13 @@ zone "example2" { primaries { 10.53.0.1; }; }; +zone "example2-toomanykeys" { + type secondary; + file "example2-toomanykeys.bk"; + allow-update-forwarding { 10.53.0.1; }; + primaries { 10.53.0.1; }; +}; + zone "example3" { type secondary; file "example3.bk"; diff --git a/bin/tests/system/upforwd/setup.sh b/bin/tests/system/upforwd/setup.sh index cc34d3966b..0df66cb6f2 100644 --- a/bin/tests/system/upforwd/setup.sh +++ b/bin/tests/system/upforwd/setup.sh @@ -32,7 +32,7 @@ else fi # -# SIG(0) required cryptographic support which may not be configured. +# SIG(0) requires cryptographic support which may not be configured. # keyname=$($KEYGEN -q -n HOST -a ${DEFAULT_ALGORITHM} -T KEY sig0.example2 2>keyname.err) if test -n "$keyname"; then @@ -42,3 +42,12 @@ else cat ns1/example1.db >ns1/example2.db fi cat_i ns1/example2-toomanykeys.db +for i in 1 2 3; 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 + echo $keyname >keyname$i + fi +done diff --git a/bin/tests/system/upforwd/tests.sh b/bin/tests/system/upforwd/tests.sh index 6c8e40b5a9..5e1f4550bd 100644 --- a/bin/tests/system/upforwd/tests.sh +++ b/bin/tests/system/upforwd/tests.sh @@ -389,7 +389,7 @@ if test -f keyname; then nextpart_thrice ret=0 keyname=$(cat keyname) - $NSUPDATE -k $keyname.private -- - <nsupdate.out.test$n 2>&1 || ret=1 local 10.53.0.1 server 10.53.0.3 ${PORT} zone example2 @@ -424,7 +424,7 @@ EOF nextpart_thrice ret=0 keyname=$(cat keyname) - $NSUPDATE -k $keyname.private -S -O -- - <nsupdate.out.test$n 2>&1 || ret=1 local 10.53.0.1 server 10.53.0.3 ${TLSPORT} zone example2 @@ -454,6 +454,28 @@ EOF status=$((status + ret)) n=$((n + 1)) fi + + echo_i "checking update forwarding with sig0 with too many keys ($n)" + nextpart_thrice + ret=0 + good=0 + bad=0 + for i in 1 2 3; 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 + server 10.53.0.3 ${PORT} + zone example2-toomanykeys + update add toomanykeys$i.example2-toomanykeys. 600 A 10.10.10.1 + 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 + if [ $ret != 0 ]; then echo_i "failed"; fi + status=$((status + ret)) + n=$((n + 1)) fi echo_i "attempting an update that should be rejected by ACL ($n)" From 54ddd848fedb320497bf6a8dd54a3679990c0a61 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 3 Jun 2024 10:49:46 +0000 Subject: [PATCH 12/14] Avoid running get_matching_view() asynchronously on an error path Also create a new ns_client_async_reset() static function to decrease code duplication. --- bin/named/server.c | 18 +++++++++--------- lib/ns/client.c | 45 +++++++++++++++++++-------------------------- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index dc16659341..fd12287d33 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -10234,6 +10234,15 @@ get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, return (*viewmatchresult); } + /* Also no offloading when there is no view at all to match against. */ + view = get_matching_view_next(NULL, message->rdclass); + if (view == NULL) { + *viewmatchresult = ISC_R_NOTFOUND; + return (*viewmatchresult); + } + + dns_message_resetsig(message); + matching_view_ctx_t *mvctx = isc_mem_get(message->mctx, sizeof(*mvctx)); *mvctx = (matching_view_ctx_t){ .srcaddr = srcaddr, @@ -10250,15 +10259,6 @@ get_matching_view(isc_netaddr_t *srcaddr, isc_netaddr_t *destaddr, isc_loop_attach(loop, &mvctx->loop); dns_message_attach(message, &mvctx->message); - dns_message_resetsig(message); - - view = get_matching_view_next(NULL, message->rdclass); - if (view == NULL) { - *mvctx->viewmatchresult = ISC_R_NOTFOUND; - isc_async_run(loop, get_matching_view_done, mvctx); - return (DNS_R_WAIT); - } - /* * If the message has a SIG0 signature which we are going to * check, and the client is not exempt from the SIG(0) quota, diff --git a/lib/ns/client.c b/lib/ns/client.c index 8d91ad07d0..7351c91923 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1683,6 +1683,16 @@ process_opt(ns_client_t *client, dns_rdataset_t *opt) { return (result); } +static void +ns_client_async_reset(ns_client_t *client) { + if (client->async) { + client->async = false; + if (client->handle != NULL) { + isc_nmhandle_unref(client->handle); + } + } +} + void ns__client_reset_cb(void *client0) { ns_client_t *client = client0; @@ -1709,12 +1719,7 @@ ns__client_reset_cb(void *client0) { client->keytag_len = 0; } - if (client->async) { - client->async = false; - if (client->handle != NULL) { - isc_nmhandle_unref(client->handle); - } - } + ns_client_async_reset(client); client->state = NS_CLIENTSTATE_READY; @@ -1749,12 +1754,7 @@ ns__client_put_cb(void *client0) { dns_message_puttemprdataset(client->message, &client->opt); } - if (client->async) { - client->async = false; - if (client->handle != NULL) { - isc_nmhandle_unref(client->handle); - } - } + ns_client_async_reset(client); dns_message_detach(&client->message); @@ -1786,19 +1786,22 @@ ns_client_setup_view(ns_client_t *client, isc_netaddr_t *netaddr) { ns_client_request_continue, client, &client->sigresult, &client->viewmatchresult, &client->view); + /* Async mode. */ if (result == DNS_R_WAIT) { INSIST(client->async == true); return (DNS_R_WAIT); } /* - * matchingview() is allowed to return anything other than DNS_R_WAIT - * only in non-async mode, in which case 'result' must be equal to + * matchingview() returning anything other than DNS_R_WAIT means it's + * not running in async mode, in which case 'result' must be equal to * 'client->viewmatchresult'. */ - INSIST(client->async == false); INSIST(result == client->viewmatchresult); + /* Non-async mode. */ + ns_client_async_reset(client); + return (result); } @@ -2480,17 +2483,7 @@ ns_client_request_continue(void *arg) { } cleanup: - - /* - * If we are running async then 'unref' the handle and reset the async - * flag, so that the destructor doesn't try to 'unref' the handle too. - */ - if (client->async) { - client->async = false; - if (client->handle != NULL) { - isc_nmhandle_unref(client->handle); - } - } + ns_client_async_reset(client); } isc_result_t From d69fab15306704f529c375e33d73d3ee741fa086 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 3 Jun 2024 10:56:02 +0000 Subject: [PATCH 13/14] Mark SIG(0) quota settings as experimantal A different solution in the future might be adopted depending on feedback and other new information, so it makes sense to mark these options as EXPERIMENTAL until we have more data. --- doc/misc/options | 4 ++-- lib/isccfg/namedconf.c | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/doc/misc/options b/doc/misc/options index 0aec7c5dea..5e5f808704 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -277,8 +277,8 @@ options { sig-signing-signatures ; sig-signing-type ; sig-validity-interval [ ]; // obsolete - sig0checks-quota ; - sig0checks-quota-exempt { ; ... }; + sig0checks-quota ; // experimental + sig0checks-quota-exempt { ; ... }; // experimental sortlist { ; ... }; // deprecated stale-answer-client-timeout ( disabled | off | ); stale-answer-enable ; diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index b9f8500d7a..a69c559f38 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -1361,8 +1361,9 @@ static cfg_clausedef_t options_clauses[] = { { "session-keyalg", &cfg_type_astring, 0 }, { "session-keyfile", &cfg_type_qstringornone, 0 }, { "session-keyname", &cfg_type_astring, 0 }, - { "sig0checks-quota", &cfg_type_uint32, 0 }, - { "sig0checks-quota-exempt", &cfg_type_bracketed_aml, 0 }, + { "sig0checks-quota", &cfg_type_uint32, CFG_CLAUSEFLAG_EXPERIMENTAL }, + { "sig0checks-quota-exempt", &cfg_type_bracketed_aml, + CFG_CLAUSEFLAG_EXPERIMENTAL }, { "sit-secret", NULL, CFG_CLAUSEFLAG_ANCIENT }, { "stacksize", &cfg_type_size, CFG_CLAUSEFLAG_ANCIENT }, { "startup-notify-rate", &cfg_type_uint32, 0 }, From 9370acd3a798b266bcb62d476ef1969934eabeb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Tue, 4 Jun 2024 18:41:44 +0200 Subject: [PATCH 14/14] Require local KEYs for SIG(0) verification This is additional hardening. There is no known use-case for KEY RRs from DNS cache and it potentially allows attackers to put weird keys into cache. --- lib/dns/message.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/dns/message.c b/lib/dns/message.c index 97ff5ecf18..fe8e05c0a0 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3323,11 +3323,9 @@ dns_message_checksig(dns_message_t *msg, dns_view_t *view) { 0, false, &keyset, NULL); if (result != ISC_R_SUCCESS) { - /* XXXBEW Should possibly create a fetch here */ result = DNS_R_KEYUNAUTHORIZED; goto freesig; - } else if (keyset.trust < dns_trust_secure) { - /* XXXBEW Should call a validator here */ + } else if (keyset.trust < dns_trust_ultimate) { result = DNS_R_KEYUNAUTHORIZED; goto freesig; }