From 7c47254a140c3e9cf383cda73c7b6a55c4782826 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 1 Sep 2022 16:05:04 -0700 Subject: [PATCH 01/18] add an update quota limit the number of simultaneous DNS UPDATE events that can be processed by adding a quota for update and update forwarding. this quota currently, arbitrarily, defaults to 100. also add a statistics counter to record when the update quota has been exceeded. --- bin/named/bind9.xsl | 2 +- bin/named/statschannel.c | 5 +++-- doc/arm/reference.rst | 5 +++++ lib/ns/include/ns/server.h | 1 + lib/ns/include/ns/stats.h | 4 +++- lib/ns/server.c | 2 ++ lib/ns/update.c | 38 +++++++++++++++++++++++++++++++++++++- 7 files changed, 52 insertions(+), 5 deletions(-) diff --git a/bin/named/bind9.xsl b/bin/named/bind9.xsl index 27161860d0..9dda61deea 100644 --- a/bin/named/bind9.xsl +++ b/bin/named/bind9.xsl @@ -15,7 +15,7 @@ - + diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index 20b88a86cd..4f45b1be30 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -56,11 +56,11 @@ #include "xsl_p.h" #define STATS_XML_VERSION_MAJOR "3" -#define STATS_XML_VERSION_MINOR "12" +#define STATS_XML_VERSION_MINOR "13" #define STATS_XML_VERSION STATS_XML_VERSION_MAJOR "." STATS_XML_VERSION_MINOR #define STATS_JSON_VERSION_MAJOR "1" -#define STATS_JSON_VERSION_MINOR "6" +#define STATS_JSON_VERSION_MINOR "7" #define STATS_JSON_VERSION STATS_JSON_VERSION_MAJOR "." STATS_JSON_VERSION_MINOR #define CHECK(m) \ @@ -352,6 +352,7 @@ init_desc(void) { SET_NSSTATDESC(reclimitdropped, "queries dropped due to recursive client limit", "RecLimitDropped"); + SET_NSSTATDESC(updatequota, "Update quota exceeded", "UpdateQuota"); INSIST(i == ns_statscounter_max); diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index c229f15a61..a106b5e10d 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -7838,6 +7838,11 @@ Name Server Statistics Counters ``UpdateBadPrereq`` This indicates the number of dynamic updates rejected due to a prerequisite failure. +``UpdateQuota`` + This indicates the number of times a dynamic update or update + forwarding request was rejected because the number of pending + requests exceeded :any:`update-quota`. + ``RateDropped`` This indicates the number of responses dropped due to rate limits. diff --git a/lib/ns/include/ns/server.h b/lib/ns/include/ns/server.h index 51c2c0a1d5..2d8f19139f 100644 --- a/lib/ns/include/ns/server.h +++ b/lib/ns/include/ns/server.h @@ -84,6 +84,7 @@ struct ns_server { isc_quota_t recursionquota; isc_quota_t tcpquota; isc_quota_t xfroutquota; + isc_quota_t updquota; ISC_LIST(isc_quota_t) http_quotas; isc_mutex_t http_quotas_lock; diff --git a/lib/ns/include/ns/stats.h b/lib/ns/include/ns/stats.h index 2de556423b..c235384f48 100644 --- a/lib/ns/include/ns/stats.h +++ b/lib/ns/include/ns/stats.h @@ -107,7 +107,9 @@ enum { ns_statscounter_reclimitdropped = 66, - ns_statscounter_max = 67, + ns_statscounter_updatequota = 67, + + ns_statscounter_max = 68, }; void diff --git a/lib/ns/server.c b/lib/ns/server.c index 5c80a7c52e..1cc7864a85 100644 --- a/lib/ns/server.c +++ b/lib/ns/server.c @@ -61,6 +61,7 @@ ns_server_create(isc_mem_t *mctx, ns_matchview_t matchingview, isc_quota_init(&sctx->xfroutquota, 10); isc_quota_init(&sctx->tcpquota, 10); isc_quota_init(&sctx->recursionquota, 100); + isc_quota_init(&sctx->updquota, 100); ISC_LIST_INIT(sctx->http_quotas); isc_mutex_init(&sctx->http_quotas_lock); @@ -133,6 +134,7 @@ ns_server_detach(ns_server_t **sctxp) { isc_mem_put(sctx->mctx, altsecret, sizeof(*altsecret)); } + isc_quota_destroy(&sctx->updquota); isc_quota_destroy(&sctx->recursionquota); isc_quota_destroy(&sctx->tcpquota); isc_quota_destroy(&sctx->xfroutquota); diff --git a/lib/ns/update.c b/lib/ns/update.c index b15bfc4f86..715a4a1866 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -1644,6 +1644,19 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) { update_event_t *event = NULL; isc_task_t *zonetask = NULL; + result = isc_quota_attach(&client->manager->sctx->updquota, + &(isc_quota_t *){ NULL }); + if (result != ISC_R_SUCCESS) { + update_log(client, zone, LOGLEVEL_PROTOCOL, + "update failed: too many DNS UPDATEs queued (%s)", + isc_result_totext(result)); + ns_stats_increment(client->manager->sctx->nsstats, + ns_statscounter_updatequota); + ns_client_drop(client, result); + isc_nmhandle_detach(&client->reqhandle); + return (DNS_R_DROP); + } + event = (update_event_t *)isc_event_allocate( client->manager->mctx, client, DNS_EVENT_UPDATE, update_action, NULL, sizeof(*event)); @@ -1780,12 +1793,19 @@ failure: dns_zone_gettype(zone) == dns_zone_mirror); inc_stats(client, zone, ns_statscounter_updaterej); } + /* * We failed without having sent an update event to the zone. * We are still in the client task context, so we can * simply give an error response without switching tasks. */ - respond(client, result); + if (result == DNS_R_DROP) { + ns_client_drop(client, result); + isc_nmhandle_detach(&client->reqhandle); + } else { + respond(client, result); + } + if (zone != NULL) { dns_zone_detach(&zone); } @@ -3582,6 +3602,7 @@ updatedone_action(isc_task_t *task, isc_event_t *event) { respond(client, uev->result); + isc_quota_detach(&(isc_quota_t *){ &client->manager->sctx->updquota }); isc_event_free(&event); isc_nmhandle_detach(&client->updatehandle); } @@ -3596,6 +3617,8 @@ forward_fail(isc_task_t *task, isc_event_t *event) { UNUSED(task); respond(client, DNS_R_SERVFAIL); + + isc_quota_detach(&(isc_quota_t *){ &client->manager->sctx->updquota }); isc_event_free(&event); isc_nmhandle_detach(&client->updatehandle); } @@ -3631,6 +3654,8 @@ forward_done(isc_task_t *task, isc_event_t *event) { ns_client_sendraw(client, uev->answer); dns_message_detach(&uev->answer); + + isc_quota_detach(&(isc_quota_t *){ &client->manager->sctx->updquota }); isc_event_free(&event); isc_nmhandle_detach(&client->reqhandle); isc_nmhandle_detach(&client->updatehandle); @@ -3666,6 +3691,17 @@ send_forward_event(ns_client_t *client, dns_zone_t *zone) { update_event_t *event = NULL; isc_task_t *zonetask = NULL; + result = isc_quota_attach(&client->manager->sctx->updquota, + &(isc_quota_t *){ NULL }); + if (result != ISC_R_SUCCESS) { + update_log(client, zone, LOGLEVEL_PROTOCOL, + "update failed: too many DNS UPDATEs queued (%s)", + isc_result_totext(result)); + ns_stats_increment(client->manager->sctx->nsstats, + ns_statscounter_updatequota); + return (DNS_R_DROP); + } + event = (update_event_t *)isc_event_allocate( client->manager->mctx, client, DNS_EVENT_UPDATE, forward_action, NULL, sizeof(*event)); From f57758a7303ad0034ff2ff08eaaf2ef899630f19 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 1 Sep 2022 16:22:46 -0700 Subject: [PATCH 02/18] add a configuration option for the update quota add an "update-quota" option to configure the update quota. --- bin/named/config.c | 1 + bin/named/server.c | 1 + bin/tests/system/checkconf/good.conf.in | 1 + doc/arm/reference.rst | 8 ++++++++ doc/man/named.conf.5in | 1 + doc/misc/options | 1 + lib/isccfg/namedconf.c | 1 + 7 files changed, 14 insertions(+) diff --git a/bin/named/config.c b/bin/named/config.c index bd45384a24..5051f3c1ac 100644 --- a/bin/named/config.c +++ b/bin/named/config.c @@ -131,6 +131,7 @@ options {\n\ trust-anchor-telemetry yes;\n\ udp-receive-buffer 0;\n\ udp-send-buffer 0;\n\ + update-quota 100;\n\ \n\ /* view */\n\ allow-new-zones no;\n\ diff --git a/bin/named/server.c b/bin/named/server.c index 3be36685ae..f6b95ccdf0 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -8518,6 +8518,7 @@ load_configuration(const char *filename, named_server_t *server, configure_server_quota(maps, "tcp-clients", &server->sctx->tcpquota); configure_server_quota(maps, "recursive-clients", &server->sctx->recursionquota); + configure_server_quota(maps, "update-quota", &server->sctx->updquota); max = isc_quota_getmax(&server->sctx->recursionquota); if (max > 1000) { diff --git a/bin/tests/system/checkconf/good.conf.in b/bin/tests/system/checkconf/good.conf.in index 8ecf392063..080914a98e 100644 --- a/bin/tests/system/checkconf/good.conf.in +++ b/bin/tests/system/checkconf/good.conf.in @@ -69,6 +69,7 @@ options { recursive-clients 3000; serial-query-rate 100; server-id none; + update-quota 200; check-names primary warn; check-names secondary ignore; max-cache-size 20000000000000; diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index a106b5e10d..cea98357b0 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -3896,6 +3896,14 @@ system. value as :any:`tcp-keepalive-timeout`. This value can be updated at runtime by using :option:`rndc tcp-timeouts`. +.. namedconf:statement:: update-quota + :tags: server + :short: Specifies the maximum number of concurrent DNS UPDATE messages that can be processed by the server. + + This is the maximum number of simultaneous DNS UPDATE messages that + the server will accept for updating local authoritiative zones or + forwarding to a primary server. The default is ``100``. + .. _intervals: Periodic Task Intervals diff --git a/doc/man/named.conf.5in b/doc/man/named.conf.5in index 16ea9e48e7..6c3f026a13 100644 --- a/doc/man/named.conf.5in +++ b/doc/man/named.conf.5in @@ -360,6 +360,7 @@ options { udp\-receive\-buffer ; udp\-send\-buffer ; update\-check\-ksk ; + update\-quota ; use\-v4\-udp\-ports { ; ... }; use\-v6\-udp\-ports { ; ... }; v6\-bias ; diff --git a/doc/misc/options b/doc/misc/options index ed8cdace58..f6a8c3cc73 100644 --- a/doc/misc/options +++ b/doc/misc/options @@ -303,6 +303,7 @@ options { udp-receive-buffer ; udp-send-buffer ; update-check-ksk ; + update-quota ; use-v4-udp-ports { ; ... }; use-v6-udp-ports { ; ... }; v6-bias ; diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index 6178b81f4e..0ceead9e19 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -1349,6 +1349,7 @@ static cfg_clausedef_t options_clauses[] = { { "treat-cr-as-space", NULL, CFG_CLAUSEFLAG_ANCIENT }, { "udp-receive-buffer", &cfg_type_uint32, 0 }, { "udp-send-buffer", &cfg_type_uint32, 0 }, + { "update-quota", &cfg_type_uint32, 0 }, { "use-id-pool", NULL, CFG_CLAUSEFLAG_ANCIENT }, { "use-ixfr", NULL, CFG_CLAUSEFLAG_ANCIENT }, { "use-v4-udp-ports", &cfg_type_bracketed_portlist, 0 }, From 964f559edb5036880b8e463b8f190b9007ee055d Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 8 Nov 2022 17:32:41 -0800 Subject: [PATCH 03/18] move update ACL and update-policy checks before quota check allow-update, update-policy, and allow-update-forwarding before consuming quota slots, so that unauthorized clients can't fill the quota. (this moves the access check before the prerequisite check, which violates the precise wording of RFC 2136. however, RFC co-author Paul Vixie has stated that the RFC is mistaken on this point; it should have said that access checking must happen *no later than* the completion of prerequisite checks, not that it must happen exactly then.) --- lib/ns/update.c | 520 ++++++++++++++++++++++++++---------------------- 1 file changed, 278 insertions(+), 242 deletions(-) diff --git a/lib/ns/update.c b/lib/ns/update.c index 715a4a1866..368538f555 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -230,6 +230,8 @@ struct update_event { dns_zone_t *zone; isc_result_t result; dns_message_t *answer; + const dns_ssurule_t **rules; + size_t ruleslen; }; /*% @@ -269,6 +271,9 @@ static void forward_done(isc_task_t *task, isc_event_t *event); static isc_result_t add_rr_prepare_action(void *data, rr_t *rr); +static isc_result_t +rr_exists(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, + const dns_rdata_t *rdata, bool *flag); /**************************************************************************/ @@ -341,25 +346,26 @@ inc_stats(ns_client_t *client, dns_zone_t *zone, isc_statscounter_t counter) { static isc_result_t checkqueryacl(ns_client_t *client, dns_acl_t *queryacl, dns_name_t *zonename, dns_acl_t *updateacl, dns_ssutable_t *ssutable) { + isc_result_t result; char namebuf[DNS_NAME_FORMATSIZE]; char classbuf[DNS_RDATACLASS_FORMATSIZE]; - int level; - isc_result_t result; + bool update_possible = + ((updateacl != NULL && !dns_acl_isnone(updateacl)) || + ssutable != NULL); result = ns_client_checkaclsilent(client, NULL, queryacl, true); if (result != ISC_R_SUCCESS) { + int level = update_possible ? ISC_LOG_ERROR : ISC_LOG_INFO; + dns_name_format(zonename, namebuf, sizeof(namebuf)); dns_rdataclass_format(client->view->rdclass, classbuf, sizeof(classbuf)); - level = (updateacl == NULL && ssutable == NULL) ? ISC_LOG_INFO - : ISC_LOG_ERROR; - ns_client_log(client, NS_LOGCATEGORY_UPDATE_SECURITY, NS_LOGMODULE_UPDATE, level, "update '%s/%s' denied due to allow-query", namebuf, classbuf); - } else if (updateacl == NULL && ssutable == NULL) { + } else if (!update_possible) { dns_name_format(zonename, namebuf, sizeof(namebuf)); dns_rdataclass_format(client->view->rdclass, classbuf, sizeof(classbuf)); @@ -1643,6 +1649,240 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) { isc_result_t result = ISC_R_SUCCESS; update_event_t *event = NULL; isc_task_t *zonetask = NULL; + dns_ssutable_t *ssutable = NULL; + dns_message_t *request = client->message; + isc_mem_t *mctx = client->manager->mctx; + dns_aclenv_t *env = client->manager->aclenv; + dns_rdataclass_t zoneclass; + dns_rdatatype_t covers; + dns_name_t *zonename = NULL; + const dns_ssurule_t **rules = NULL; + size_t rule = 0, ruleslen = 0; + dns_zoneopt_t options; + dns_db_t *db = NULL; + dns_dbversion_t *ver = NULL; + + CHECK(dns_zone_getdb(zone, &db)); + zonename = dns_db_origin(db); + zoneclass = dns_db_class(db); + dns_zone_getssutable(zone, &ssutable); + options = dns_zone_getoptions(zone); + dns_db_currentversion(db, &ver); + + /* + * Update message processing can leak record existence information + * so check that we are allowed to query this zone. Additionally, + * if we would refuse all updates for this zone, we bail out here. + */ + CHECK(checkqueryacl(client, dns_zone_getqueryacl(zone), + dns_zone_getorigin(zone), + dns_zone_getupdateacl(zone), ssutable)); + + /* + * Check requestor's permissions. + */ + if (ssutable == NULL) { + CHECK(checkupdateacl(client, dns_zone_getupdateacl(zone), + "update", dns_zone_getorigin(zone), false, + false)); + } else if (client->signer == NULL && !TCPCLIENT(client)) { + CHECK(checkupdateacl(client, NULL, "update", + dns_zone_getorigin(zone), false, true)); + } + + if (dns_zone_getupdatedisabled(zone)) { + FAILC(DNS_R_REFUSED, "dynamic update temporarily disabled " + "because the zone is frozen. Use " + "'rndc thaw' to re-enable updates."); + } + + /* + * Prescan the update section, checking for updates that + * are illegal or violate policy. + */ + if (ssutable != NULL) { + ruleslen = request->counts[DNS_SECTION_UPDATE]; + rules = isc_mem_getx(mctx, sizeof(*rules) * ruleslen, + ISC_MEM_ZERO); + } + + for (rule = 0, + result = dns_message_firstname(request, DNS_SECTION_UPDATE); + result == ISC_R_SUCCESS; + rule++, result = dns_message_nextname(request, DNS_SECTION_UPDATE)) + { + dns_name_t *name = NULL; + dns_rdata_t rdata = DNS_RDATA_INIT; + dns_ttl_t ttl; + dns_rdataclass_t update_class; + + INSIST(ssutable == NULL || rule < ruleslen); + + get_current_rr(request, DNS_SECTION_UPDATE, zoneclass, &name, + &rdata, &covers, &ttl, &update_class); + + if (!dns_name_issubdomain(name, zonename)) { + FAILC(DNS_R_NOTZONE, "update RR is outside zone"); + } + if (update_class == zoneclass) { + /* + * Check for meta-RRs. The RFC2136 pseudocode says + * check for ANY|AXFR|MAILA|MAILB, but the text adds + * "or any other QUERY metatype" + */ + if (dns_rdatatype_ismeta(rdata.type)) { + FAILC(DNS_R_FORMERR, "meta-RR in update"); + } + result = dns_zone_checknames(zone, name, &rdata); + if (result != ISC_R_SUCCESS) { + FAIL(DNS_R_REFUSED); + } + if ((options & DNS_ZONEOPT_CHECKSVCB) != 0 && + rdata.type == dns_rdatatype_svcb) + { + result = dns_rdata_checksvcb(name, &rdata); + if (result != ISC_R_SUCCESS) { + const char *reason = + isc_result_totext(result); + FAILNT(DNS_R_REFUSED, name, rdata.type, + reason); + } + } + } else if (update_class == dns_rdataclass_any) { + if (ttl != 0 || rdata.length != 0 || + (dns_rdatatype_ismeta(rdata.type) && + rdata.type != dns_rdatatype_any)) + { + FAILC(DNS_R_FORMERR, "meta-RR in update"); + } + } else if (update_class == dns_rdataclass_none) { + if (ttl != 0 || dns_rdatatype_ismeta(rdata.type)) { + FAILC(DNS_R_FORMERR, "meta-RR in update"); + } + } else { + update_log(client, zone, ISC_LOG_WARNING, + "update RR has incorrect class %d", + update_class); + FAIL(DNS_R_FORMERR); + } + + /* + * draft-ietf-dnsind-simple-secure-update-01 says + * "Unlike traditional dynamic update, the client + * is forbidden from updating NSEC records." + */ + if (rdata.type == dns_rdatatype_nsec3) { + FAILC(DNS_R_REFUSED, "explicit NSEC3 updates are not " + "allowed " + "in secure zones"); + } else if (rdata.type == dns_rdatatype_nsec) { + FAILC(DNS_R_REFUSED, "explicit NSEC updates are not " + "allowed " + "in secure zones"); + } else if (rdata.type == dns_rdatatype_rrsig && + !dns_name_equal(name, zonename)) + { + FAILC(DNS_R_REFUSED, "explicit RRSIG updates are " + "currently " + "not supported in secure zones " + "except " + "at the apex"); + } + + if (ssutable != NULL) { + isc_netaddr_t netaddr; + dns_name_t *target = NULL; + dst_key_t *tsigkey = NULL; + dns_rdata_ptr_t ptr; + dns_rdata_in_srv_t srv; + + isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); + + if (client->message->tsigkey != NULL) { + tsigkey = client->message->tsigkey->key; + } + + if ((update_class == dns_rdataclass_in || + update_class == dns_rdataclass_none) && + rdata.type == dns_rdatatype_ptr) + { + result = dns_rdata_tostruct(&rdata, &ptr, NULL); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + target = &ptr.ptr; + } + + if ((update_class == dns_rdataclass_in || + update_class == dns_rdataclass_none) && + rdata.type == dns_rdatatype_srv) + { + result = dns_rdata_tostruct(&rdata, &srv, NULL); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + target = &srv.target; + } + + if (update_class == dns_rdataclass_any && + zoneclass == dns_rdataclass_in && + (rdata.type == dns_rdatatype_ptr || + rdata.type == dns_rdatatype_srv)) + { + ssu_check_t ssuinfo; + + ssuinfo.name = name; + ssuinfo.table = ssutable; + ssuinfo.signer = client->signer; + ssuinfo.addr = &netaddr; + ssuinfo.aclenv = env; + ssuinfo.tcp = TCPCLIENT(client); + ssuinfo.key = tsigkey; + + result = foreach_rr(db, ver, name, rdata.type, + dns_rdatatype_none, + ssu_checkrr, &ssuinfo); + if (result != ISC_R_SUCCESS) { + FAILC(DNS_R_REFUSED, + "rejected by secure update"); + } + } else if (target != NULL && + update_class == dns_rdataclass_none) + { + bool flag; + CHECK(rr_exists(db, ver, name, &rdata, &flag)); + if (flag && + !dns_ssutable_checkrules( + ssutable, client->signer, name, + &netaddr, TCPCLIENT(client), env, + rdata.type, target, tsigkey, + &rules[rule])) + { + FAILC(DNS_R_REFUSED, + "rejected by secure update"); + } + } else if (rdata.type != dns_rdatatype_any) { + if (!dns_ssutable_checkrules( + ssutable, client->signer, name, + &netaddr, TCPCLIENT(client), env, + rdata.type, target, tsigkey, + &rules[rule])) + { + FAILC(DNS_R_REFUSED, "rejected by " + "secure update"); + } + } else { + if (!ssu_checkall(db, ver, name, ssutable, + client->signer, &netaddr, env, + TCPCLIENT(client), tsigkey)) + { + FAILC(DNS_R_REFUSED, "rejected by " + "secure update"); + } + } + } + } + if (result != ISC_R_NOMORE) { + FAIL(result); + } + + update_log(client, zone, LOGLEVEL_DEBUG, "update section prescan OK"); result = isc_quota_attach(&client->manager->sctx->updquota, &(isc_quota_t *){ NULL }); @@ -1652,23 +1892,36 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) { isc_result_totext(result)); ns_stats_increment(client->manager->sctx->nsstats, ns_statscounter_updatequota); - ns_client_drop(client, result); - isc_nmhandle_detach(&client->reqhandle); - return (DNS_R_DROP); + CHECK(DNS_R_DROP); } event = (update_event_t *)isc_event_allocate( client->manager->mctx, client, DNS_EVENT_UPDATE, update_action, - NULL, sizeof(*event)); + client, sizeof(*event)); event->zone = zone; event->result = ISC_R_SUCCESS; - - event->ev_arg = client; + event->rules = rules; + event->ruleslen = ruleslen; + rules = NULL; isc_nmhandle_attach(client->handle, &client->updatehandle); dns_zone_gettask(zone, &zonetask); isc_task_send(zonetask, ISC_EVENT_PTR(&event)); +failure: + if (db != NULL) { + dns_db_closeversion(db, &ver, false); + dns_db_detach(&db); + } + + if (rules != NULL) { + isc_mem_put(mctx, rules, sizeof(*rules) * ruleslen); + } + + if (ssutable != NULL) { + dns_ssutable_detach(&ssutable); + } + return (result); } @@ -1776,9 +2029,6 @@ ns_update_start(ns_client_t *client, isc_nmhandle_t *handle, break; case dns_zone_secondary: case dns_zone_mirror: - CHECK(checkupdateacl(client, dns_zone_getforwardacl(zone), - "update forwarding", zonename, true, - false)); dns_message_clonebuffer(client->message); CHECK(send_forward_event(client, zone)); break; @@ -1789,8 +2039,6 @@ ns_update_start(ns_client_t *client, isc_nmhandle_t *handle, failure: if (result == DNS_R_REFUSED) { - INSIST(dns_zone_gettype(zone) == dns_zone_secondary || - dns_zone_gettype(zone) == dns_zone_mirror); inc_stats(client, zone, ns_statscounter_updaterej); } @@ -2639,6 +2887,8 @@ update_action(isc_task_t *task, isc_event_t *event) { update_event_t *uev = (update_event_t *)event; dns_zone_t *zone = uev->zone; ns_client_t *client = (ns_client_t *)event->ev_arg; + const dns_ssurule_t **rules = uev->rules; + size_t rule = 0, ruleslen = uev->ruleslen; isc_result_t result; dns_db_t *db = NULL; dns_dbversion_t *oldver = NULL; @@ -2650,7 +2900,7 @@ update_action(isc_task_t *task, isc_event_t *event) { dns_rdatatype_t covers; dns_message_t *request = client->message; dns_rdataclass_t zoneclass; - dns_name_t *zonename; + dns_name_t *zonename = NULL; dns_ssutable_t *ssutable = NULL; dns_fixedname_t tmpnamefixed; dns_name_t *tmpname = NULL; @@ -2660,10 +2910,6 @@ update_action(isc_task_t *task, isc_event_t *event) { dns_ttl_t maxttl = 0; uint32_t maxrecords; uint64_t records; - dns_aclenv_t *env = client->manager->aclenv; - size_t ruleslen = 0; - size_t rule; - const dns_ssurule_t **rules = NULL; INSIST(event->ev_type == DNS_EVENT_UPDATE); @@ -2674,14 +2920,7 @@ update_action(isc_task_t *task, isc_event_t *event) { zonename = dns_db_origin(db); zoneclass = dns_db_class(db); dns_zone_getssutable(zone, &ssutable); - - /* - * Update message processing can leak record existence information - * so check that we are allowed to query this zone. Additionally - * if we would refuse all updates for this zone we bail out here. - */ - CHECK(checkqueryacl(client, dns_zone_getqueryacl(zone), zonename, - dns_zone_getupdateacl(zone), ssutable)); + options = dns_zone_getoptions(zone); /* * Get old and new versions now that queryacl has been checked. @@ -2816,217 +3055,10 @@ update_action(isc_task_t *task, isc_event_t *event) { update_log(client, zone, LOGLEVEL_DEBUG, "prerequisites are OK"); - /* - * Check Requestor's Permissions. It seems a bit silly to do this - * only after prerequisite testing, but that is what RFC2136 says. - */ - if (ssutable == NULL) { - CHECK(checkupdateacl(client, dns_zone_getupdateacl(zone), - "update", zonename, false, false)); - } else if (client->signer == NULL && !TCPCLIENT(client)) { - CHECK(checkupdateacl(client, NULL, "update", zonename, false, - true)); - } - - if (dns_zone_getupdatedisabled(zone)) { - FAILC(DNS_R_REFUSED, "dynamic update temporarily disabled " - "because the zone is frozen. Use " - "'rndc thaw' to re-enable updates."); - } - - /* - * Perform the Update Section Prescan. - */ - if (ssutable != NULL) { - ruleslen = request->counts[DNS_SECTION_UPDATE]; - rules = isc_mem_getx(mctx, sizeof(*rules) * ruleslen, - ISC_MEM_ZERO); - } - - options = dns_zone_getoptions(zone); - - for (rule = 0, - result = dns_message_firstname(request, DNS_SECTION_UPDATE); - result == ISC_R_SUCCESS; - rule++, result = dns_message_nextname(request, DNS_SECTION_UPDATE)) - { - dns_name_t *name = NULL; - dns_rdata_t rdata = DNS_RDATA_INIT; - dns_ttl_t ttl; - dns_rdataclass_t update_class; - - INSIST(ssutable == NULL || rule < ruleslen); - - get_current_rr(request, DNS_SECTION_UPDATE, zoneclass, &name, - &rdata, &covers, &ttl, &update_class); - - if (!dns_name_issubdomain(name, zonename)) { - FAILC(DNS_R_NOTZONE, "update RR is outside zone"); - } - if (update_class == zoneclass) { - /* - * Check for meta-RRs. The RFC2136 pseudocode says - * check for ANY|AXFR|MAILA|MAILB, but the text adds - * "or any other QUERY metatype" - */ - if (dns_rdatatype_ismeta(rdata.type)) { - FAILC(DNS_R_FORMERR, "meta-RR in update"); - } - result = dns_zone_checknames(zone, name, &rdata); - if (result != ISC_R_SUCCESS) { - FAIL(DNS_R_REFUSED); - } - if ((options & DNS_ZONEOPT_CHECKSVCB) != 0 && - rdata.type == dns_rdatatype_svcb) - { - result = dns_rdata_checksvcb(name, &rdata); - if (result != ISC_R_SUCCESS) { - const char *reason = - isc_result_totext(result); - FAILNT(DNS_R_REFUSED, name, rdata.type, - reason); - } - } - } else if (update_class == dns_rdataclass_any) { - if (ttl != 0 || rdata.length != 0 || - (dns_rdatatype_ismeta(rdata.type) && - rdata.type != dns_rdatatype_any)) - { - FAILC(DNS_R_FORMERR, "meta-RR in update"); - } - } else if (update_class == dns_rdataclass_none) { - if (ttl != 0 || dns_rdatatype_ismeta(rdata.type)) { - FAILC(DNS_R_FORMERR, "meta-RR in update"); - } - } else { - update_log(client, zone, ISC_LOG_WARNING, - "update RR has incorrect class %d", - update_class); - FAIL(DNS_R_FORMERR); - } - - /* - * draft-ietf-dnsind-simple-secure-update-01 says - * "Unlike traditional dynamic update, the client - * is forbidden from updating NSEC records." - */ - if (rdata.type == dns_rdatatype_nsec3) { - FAILC(DNS_R_REFUSED, "explicit NSEC3 updates are not " - "allowed " - "in secure zones"); - } else if (rdata.type == dns_rdatatype_nsec) { - FAILC(DNS_R_REFUSED, "explicit NSEC updates are not " - "allowed " - "in secure zones"); - } else if (rdata.type == dns_rdatatype_rrsig && - !dns_name_equal(name, zonename)) - { - FAILC(DNS_R_REFUSED, "explicit RRSIG updates are " - "currently " - "not supported in secure zones " - "except " - "at the apex"); - } - - if (ssutable != NULL) { - isc_netaddr_t netaddr; - dns_name_t *target = NULL; - dst_key_t *tsigkey = NULL; - dns_rdata_ptr_t ptr; - dns_rdata_in_srv_t srv; - - isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr); - - if (client->message->tsigkey != NULL) { - tsigkey = client->message->tsigkey->key; - } - - if ((update_class == dns_rdataclass_in || - update_class == dns_rdataclass_none) && - rdata.type == dns_rdatatype_ptr) - { - result = dns_rdata_tostruct(&rdata, &ptr, NULL); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - target = &ptr.ptr; - } - - if ((update_class == dns_rdataclass_in || - update_class == dns_rdataclass_none) && - rdata.type == dns_rdatatype_srv) - { - result = dns_rdata_tostruct(&rdata, &srv, NULL); - RUNTIME_CHECK(result == ISC_R_SUCCESS); - target = &srv.target; - } - - if (update_class == dns_rdataclass_any && - zoneclass == dns_rdataclass_in && - (rdata.type == dns_rdatatype_ptr || - rdata.type == dns_rdatatype_srv)) - { - ssu_check_t ssuinfo; - - ssuinfo.name = name; - ssuinfo.table = ssutable; - ssuinfo.signer = client->signer; - ssuinfo.addr = &netaddr; - ssuinfo.aclenv = env; - ssuinfo.tcp = TCPCLIENT(client); - ssuinfo.key = tsigkey; - - result = foreach_rr(db, ver, name, rdata.type, - dns_rdatatype_none, - ssu_checkrr, &ssuinfo); - if (result != ISC_R_SUCCESS) { - FAILC(DNS_R_REFUSED, - "rejected by secure update"); - } - } else if (target != NULL && - update_class == dns_rdataclass_none) - { - bool flag; - CHECK(rr_exists(db, ver, name, &rdata, &flag)); - if (flag && - !dns_ssutable_checkrules( - ssutable, client->signer, name, - &netaddr, TCPCLIENT(client), env, - rdata.type, target, tsigkey, - &rules[rule])) - { - FAILC(DNS_R_REFUSED, - "rejected by secure update"); - } - } else if (rdata.type != dns_rdatatype_any) { - if (!dns_ssutable_checkrules( - ssutable, client->signer, name, - &netaddr, TCPCLIENT(client), env, - rdata.type, target, tsigkey, - &rules[rule])) - { - FAILC(DNS_R_REFUSED, "rejected by " - "secure update"); - } - } else { - if (!ssu_checkall(db, ver, name, ssutable, - client->signer, &netaddr, env, - TCPCLIENT(client), tsigkey)) - { - FAILC(DNS_R_REFUSED, "rejected by " - "secure update"); - } - } - } - } - if (result != ISC_R_NOMORE) { - FAIL(result); - } - - update_log(client, zone, LOGLEVEL_DEBUG, "update section prescan OK"); - /* * Process the Update Section. */ - + INSIST(ssutable == NULL || rules != NULL); for (rule = 0, result = dns_message_firstname(request, DNS_SECTION_UPDATE); result == ISC_R_SUCCESS; @@ -3474,10 +3506,7 @@ update_action(isc_task_t *task, isc_event_t *event) { if (result == ISC_R_SUCCESS && records > maxrecords) { update_log(client, zone, ISC_LOG_ERROR, "records in zone (%" PRIu64 ") " - "exceeds" - " max-" - "records" - " (%u)", + "exceeds max-records (%u)", records, maxrecords); result = DNS_R_TOOMANYRECORDS; goto failure; @@ -3691,6 +3720,13 @@ send_forward_event(ns_client_t *client, dns_zone_t *zone) { update_event_t *event = NULL; isc_task_t *zonetask = NULL; + result = checkupdateacl(client, dns_zone_getforwardacl(zone), + "update forwarding", dns_zone_getorigin(zone), + true, false); + if (result != ISC_R_SUCCESS) { + return (result); + } + result = isc_quota_attach(&client->manager->sctx->updquota, &(isc_quota_t *){ NULL }); if (result != ISC_R_SUCCESS) { From b91339b80e5b82a56622c93cc1e3cca2d0c11bc0 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 9 Nov 2022 21:56:16 -0800 Subject: [PATCH 04/18] test failure conditions verify that updates are refused when the client is disallowed by allow-query, and update forwarding is refused when the client is is disallowed by update-forwarding. verify that "too many DNS UPDATEs" appears in the log file when too many simultaneous updates are processing. --- bin/tests/system/nsupdate/ns1/named.conf.in | 2 + bin/tests/system/nsupdate/tests.sh | 28 +++++++++++ bin/tests/system/upforwd/clean.sh | 2 + .../ns3/{named.conf.in => named1.conf.in} | 12 ++--- bin/tests/system/upforwd/ns3/named2.conf.in | 43 +++++++++++++++++ bin/tests/system/upforwd/setup.sh | 2 +- bin/tests/system/upforwd/tests.sh | 46 ++++++++++++++++++- 7 files changed, 126 insertions(+), 9 deletions(-) rename bin/tests/system/upforwd/ns3/{named.conf.in => named1.conf.in} (84%) create mode 100644 bin/tests/system/upforwd/ns3/named2.conf.in diff --git a/bin/tests/system/nsupdate/ns1/named.conf.in b/bin/tests/system/nsupdate/ns1/named.conf.in index bdab9e8621..52ea741802 100644 --- a/bin/tests/system/nsupdate/ns1/named.conf.in +++ b/bin/tests/system/nsupdate/ns1/named.conf.in @@ -57,6 +57,7 @@ options { recursion no; notify yes; minimal-responses no; + update-quota 1; }; acl named-acl { @@ -117,6 +118,7 @@ zone "other.nil" { check-integrity no; check-mx warn; update-policy local; + allow-query { !10.53.0.2; any; }; allow-query-on { 10.53.0.1; 127.0.0.1; }; allow-transfer { any; }; }; diff --git a/bin/tests/system/nsupdate/tests.sh b/bin/tests/system/nsupdate/tests.sh index 5adc99ce5c..7333609645 100755 --- a/bin/tests/system/nsupdate/tests.sh +++ b/bin/tests/system/nsupdate/tests.sh @@ -1558,6 +1558,34 @@ $DIG $DIGOPTS +tcp @10.53.0.3 _dns.ns.relaxed SVCB > dig.out.ns3.test$n grep '1 ns.relaxed. alpn="h2"' dig.out.ns3.test$n || ret=1 [ $ret = 0 ] || { echo_i "failed"; status=1; } +n=$((n + 1)) +ret=0 +echo_i "check that update is rejected if query is not allowed ($n)" +{ + $NSUPDATE -d < nsupdate.out.test$n 2>&1 +grep 'failed: REFUSED' nsupdate.out.test$n > /dev/null || ret=1 +[ $ret = 0 ] || { echo_i "failed"; status=1; } + +n=$((n + 1)) +ret=0 +echo_i "check that update is rejected if quota is exceeded ($n)" +for loop in 1 2 3 4 5 6 7 8 9 10; do +{ + $NSUPDATE -4 -l -p ${PORT} -k ns1/session.key > /dev/null 2>&1 < DoT) ($n)" ret=0 $NSUPDATE -y "${DEFAULT_HMAC}:update.example:c3Ryb25nIGVub3VnaCBmb3IgYSBtYW4gYnV0IG1hZGUgZm9yIGEgd29tYW4K" -- - < DoT) ($n)" ret=0 $NSUPDATE -y "${DEFAULT_HMAC}:update.example:c3Ryb25nIGVub3VnaCBmb3IgYSBtYW4gYnV0IG1hZGUgZm9yIGEgd29tYW4K" -S -O -- - < DoT) ($n)" ret=0 $NSUPDATE -y "${DEFAULT_HMAC}:update.example:c3Ryb25nIGVub3VnaCBmb3IgYSBtYW4gYnV0IG1hZGUgZm9yIGEgd29tYW4K" -- - < Do53) ($n)" + echo_i "checking update forwarding with sig0 (Do53 -> Do53) ($n)" ret=0 keyname=`cat keyname` $NSUPDATE -k $keyname.private -- - < Do53) ($n)" + echo_i "checking update forwarding with sig0 (DoT -> Do53) ($n)" ret=0 keyname=`cat keyname` $NSUPDATE -k $keyname.private -S -O -- - < nsupdate.out.$n 2>&1 +grep REFUSED nsupdate.out.$n > /dev/null || ret=1 +if [ $ret != 0 ] ; then echo_i "failed"; status=`expr $status + $ret`; fi +n=`expr $n + 1` + +n=$((n + 1)) +ret=0 +echo_i "attempting updates that should exceed quota ($n)" +# lower the update quota to 1. +copy_setports ns3/named2.conf.in ns3/named.conf +rndc_reconfig ns3 10.53.0.3 +nextpart ns3/named.run > /dev/null +for loop in 1 2 3 4 5 6 7 8 9 10; do +{ + $NSUPDATE -- - > /dev/null 2>&1 < Date: Thu, 1 Sep 2022 16:34:21 -0700 Subject: [PATCH 05/18] CHANGES and release notes for [GL #3523] --- CHANGES | 9 ++++++++- doc/notes/notes-current.rst | 17 +++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/CHANGES b/CHANGES index f774c51152..f0ead49230 100644 --- a/CHANGES +++ b/CHANGES @@ -4,7 +4,14 @@ 6065. [placeholder] -6064. [placeholder] +6064. [security] An UPDATE message flood could cause named to exhaust all + available memory. This flaw was addressed by adding a + new "update-quota" statement that controls the number of + simultaneous UPDATE messages that can be processed or + forwarded. The default is 100. A stats counter has been + added to record events when the update quota is + exceeded, and the XML and JSON statistics version + numbers have been updated. (CVE-2022-3094) [GL #3523] 6063. [cleanup] The RSA and ECDSA parts of the DNSSEC has been refactored for a better OpenSSL 3.x integration and diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 078aa901ff..470a7443f6 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -15,12 +15,25 @@ Notes for BIND 9.19.9 Security Fixes ~~~~~~~~~~~~~~ -- None. +- An UPDATE message flood could cause :iscman:`named` to exhaust all + available memory. This flaw was addressed by adding a new + :any:`update-quota` option that controls the maximum number of + outstanding DNS UPDATE messages that :iscman:`named` can hold in a + queue at any given time (default: 100). (CVE-2022-3094) + + ISC would like to thank Rob Schulhof from Infoblox for bringing this + vulnerability to our attention. :gl:`#3523` New Features ~~~~~~~~~~~~ -- None. +- The new :any:`update-quota` option can be used to control the number + of simultaneous DNS UPDATE messages that can be processed to update an + authoritative zone on a primary server, or forwarded to the primary + server by a secondary server. The default is 100. A new statistics + counter has also been added to record events when this quota is + exceeded, and the version numbers for the XML and JSON statistics + schemas have been updated. :gl:`#3523` Removed Features ~~~~~~~~~~~~~~~~ From 56eae064183488bcf7ff08c3edf59f2e1742c1b6 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 27 Oct 2022 13:22:11 +1100 Subject: [PATCH 06/18] Move the mapping of SIG and RRSIG to ANY dns_db_findext() asserts if RRSIG is passed to it and query_lookup_stale() failed to map RRSIG to ANY to prevent this. To avoid cases like this in the future, move the mapping of SIG and RRSIG to ANY for qctx->type to qctx_init(). --- lib/ns/query.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index faab1340ed..d2dcafe683 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -5356,6 +5356,15 @@ qctx_init(ns_client_t *client, dns_fetchevent_t **eventp, dns_rdatatype_t qtype, qctx->result = ISC_R_SUCCESS; qctx->findcoveringnsec = qctx->view->synthfromdnssec; + /* + * If it's an RRSIG or SIG query, we'll iterate the node. + */ + if (qctx->qtype == dns_rdatatype_rrsig || + qctx->qtype == dns_rdatatype_sig) + { + qctx->type = dns_rdatatype_any; + } + CALL_HOOK_NORETURN(NS_QUERY_QCTX_INITIALIZED, qctx); } @@ -5523,15 +5532,6 @@ query_setup(ns_client_t *client, dns_rdatatype_t qtype) { CALL_HOOK(NS_QUERY_SETUP, &qctx); - /* - * If it's a SIG query, we'll iterate the node. - */ - if (qctx.qtype == dns_rdatatype_rrsig || - qctx.qtype == dns_rdatatype_sig) - { - qctx.type = dns_rdatatype_any; - } - /* * Check SERVFAIL cache */ From 8ca018b5ec2c25bbfc4b951aa9826a46d01fba51 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 28 Oct 2022 11:26:59 +1100 Subject: [PATCH 07/18] Add CHANGES note for [GL #3622] --- CHANGES | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index f0ead49230..e9d3c8ac45 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,7 @@ 6067. [placeholder] -6066. [placeholder] +6066. [security] Handle RRSIG lookups when serve-stale is active. + (CVE-2022-3736) [GL #3622] 6065. [placeholder] From 42c42be9a997a30dcf83c8a77a2f57811757a72d Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 28 Oct 2022 11:31:19 +1100 Subject: [PATCH 08/18] Add release note for [GL #3622] --- doc/notes/notes-current.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 470a7443f6..53f5cd4533 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -24,6 +24,14 @@ Security Fixes ISC would like to thank Rob Schulhof from Infoblox for bringing this vulnerability to our attention. :gl:`#3523` +- :iscman:`named` could crash with an assertion failure when an RRSIG + query was received and :any:`stale-answer-client-timeout` was set to a + non-zero value. This has been fixed. (CVE-2022-3736) + + ISC would like to thank Borja Marcos from Sarenet (with assistance by + Iratxe Niño from Fundación Sarenet) for bringing this vulnerability to + our attention. :gl:`#3622` + New Features ~~~~~~~~~~~~ From ec2098ca35039e4f81fd0aa7c525eb960b8f47bf Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 14 Nov 2022 12:18:06 +0000 Subject: [PATCH 09/18] Cancel all fetch events in dns_resolver_cancelfetch() Although 'dns_fetch_t' fetch can have two associated events, one for each of 'DNS_EVENT_FETCHDONE' and 'DNS_EVENT_TRYSTALE' types, the dns_resolver_cancelfetch() function is designed in a way that it expects only one existing event, which it must cancel, and when it happens so that 'stale-answer-client-timeout' is enabled and there are two events, only one of them is canceled, and it results in an assertion in dns_resolver_destroyfetch(), when it finds a dangling event. Change the logic of dns_resolver_cancelfetch() function so that it cancels both the events (if they exist), and in the right order. --- lib/dns/resolver.c | 65 +++++++++++++++++++++++++++++++++++++++------- lib/ns/query.c | 4 ++- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 973337f2ac..411b0af6d4 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -10701,6 +10701,8 @@ fail: void dns_resolver_cancelfetch(dns_fetch_t *fetch) { fetchctx_t *fctx = NULL; + dns_fetchevent_t *event_trystale = NULL; + dns_fetchevent_t *event_fetchdone = NULL; REQUIRE(DNS_FETCH_VALID(fetch)); fctx = fetch->private; @@ -10711,24 +10713,69 @@ dns_resolver_cancelfetch(dns_fetch_t *fetch) { LOCK(&fctx->lock); /* - * Find the completion event for this fetch (as opposed - * to those for other fetches that have joined the same - * fctx) and send it with result = ISC_R_CANCELED. + * Find all the events associated with the provided 'fetch' (as opposed + * to those for other fetches that have joined the same fctx). The + * event(s) found are only sent and removed from the fctx->events list + * after this loop is finished (i.e. the fctx->events list is not + * modified during iteration). */ for (dns_fetchevent_t *event = ISC_LIST_HEAD(fctx->events); event != NULL; event = ISC_LIST_NEXT(event, ev_link)) { - if (event->fetch == fetch) { - isc_task_t *task = event->ev_sender; - ISC_LIST_UNLINK(fctx->events, event, ev_link); - event->ev_sender = fctx; - event->result = ISC_R_CANCELED; + /* + * Only process events associated with the provided 'fetch'. + */ + if (event->fetch != fetch) { + continue; + } - isc_task_sendanddetach(&task, ISC_EVENT_PTR(&event)); + /* + * Track various events associated with the provided 'fetch' in + * separate variables as they will need to be sent in a + * specific order. + */ + switch (event->ev_type) { + case DNS_EVENT_TRYSTALE: + INSIST(event_trystale == NULL); + event_trystale = event; + break; + case DNS_EVENT_FETCHDONE: + INSIST(event_fetchdone == NULL); + event_fetchdone = event; + break; + default: + UNREACHABLE(); + } + + /* + * Break out of the loop once all possible types of events + * associated with the provided 'fetch' are found. + */ + if (event_trystale != NULL && event_fetchdone != NULL) { break; } } + /* + * The "trystale" event must be sent before the "fetchdone" event, + * because the latter clears the "recursing" query attribute, which is + * required by both events (handled by the same callback function). + */ + if (event_trystale != NULL) { + isc_task_t *etask = event_trystale->ev_sender; + ISC_LIST_UNLINK(fctx->events, event_trystale, ev_link); + event_trystale->ev_sender = fctx; + event_trystale->result = ISC_R_CANCELED; + isc_task_sendanddetach(&etask, ISC_EVENT_PTR(&event_trystale)); + } + if (event_fetchdone != NULL) { + isc_task_t *etask = event_fetchdone->ev_sender; + ISC_LIST_UNLINK(fctx->events, event_fetchdone, ev_link); + event_fetchdone->ev_sender = fctx; + event_fetchdone->result = ISC_R_CANCELED; + isc_task_sendanddetach(&etask, ISC_EVENT_PTR(&event_fetchdone)); + } + /* * The fctx continues running even if no fetches remain; * the answer is still cached. diff --git a/lib/ns/query.c b/lib/ns/query.c index d2dcafe683..eddbcd98ac 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6257,7 +6257,9 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { CTRACE(ISC_LOG_DEBUG(3), "fetch_callback"); if (event->ev_type == DNS_EVENT_TRYSTALE) { - query_lookup_stale(client); + if (devent->result != ISC_R_CANCELED) { + query_lookup_stale(client); + } isc_event_free(ISC_EVENT_PTR(&event)); return; } From d08a478b4219163bcba3f31641f8f1d4e77681ff Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 14 Nov 2022 12:30:49 +0000 Subject: [PATCH 10/18] Add CHANGES and release notes for [GL #3619] --- CHANGES | 3 ++- doc/notes/notes-current.rst | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index e9d3c8ac45..375dea546f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,5 @@ -6067. [placeholder] +6067. [security] Fix serve-stale crash when recursive clients soft quota + is reached. (CVE-2022-3924) [GL #3619] 6066. [security] Handle RRSIG lookups when serve-stale is active. (CVE-2022-3736) [GL #3622] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 53f5cd4533..3d44ce7067 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -32,6 +32,15 @@ Security Fixes Iratxe Niño from Fundación Sarenet) for bringing this vulnerability to our attention. :gl:`#3622` +- :iscman:`named` running as a resolver with the + :any:`stale-answer-client-timeout` option set to any value greater + than ``0`` could crash with an assertion failure, when the + :any:`recursive-clients` soft quota was reached. This has been fixed. + (CVE-2022-3924) + + ISC would like to thank Maksym Odinintsev from AWS for bringing this + vulnerability to our attention. :gl:`#3619` + New Features ~~~~~~~~~~~~ From b70313d96da8000941a00ac1b465d35c34de3972 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 21:39:37 +0100 Subject: [PATCH 11/18] Fix a typo in the DNSSEC Guide --- doc/dnssec-guide/introduction.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/dnssec-guide/introduction.rst b/doc/dnssec-guide/introduction.rst index 818c7e2681..f4ec793dd0 100644 --- a/doc/dnssec-guide/introduction.rst +++ b/doc/dnssec-guide/introduction.rst @@ -250,7 +250,7 @@ at a very high level, looking up the name ``www.isc.org`` : Let's take a quick break here and look at what we've got so far... how can our server trust this answer? If a clever attacker had taken over - the ``isc.org`` name server(s), or course she would send matching + the ``isc.org`` name server(s), of course she would send matching keys and signatures. We need to ask someone else to have confidence that we are really talking to the real ``isc.org`` name server. This is a critical part of DNSSEC: at some point, the DNS administrators From ad57bbb411d20a403edfb715715b940c667fa9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 21:39:37 +0100 Subject: [PATCH 12/18] Update documentation for GL #3212 --- CHANGES | 4 +++- doc/notes/notes-9.19.0.rst | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 375dea546f..33f05c95c6 100644 --- a/CHANGES +++ b/CHANGES @@ -952,7 +952,9 @@ when the resent query's result was SERVFAIL. [GL #3020] 5830. [func] Implement incremental resizing of isc_ht hash tables to - perform the rehashing gradually. [GL #3212] + perform the rehashing gradually. The catalog zone + implementation has been optimized to work with hundreds + of thousands of member zones. [GL #3212] [GL #3744] 5829. [func] Refactor and simplify isc_timer API in preparation for further refactoring on top of network manager diff --git a/doc/notes/notes-9.19.0.rst b/doc/notes/notes-9.19.0.rst index a6f92bfe48..f6363e680c 100644 --- a/doc/notes/notes-9.19.0.rst +++ b/doc/notes/notes-9.19.0.rst @@ -56,3 +56,6 @@ Feature Changes threads. This should increase the responsiveness of :iscman:`named` when RPZ updates are being applied after an RPZ zone has been successfully transferred. :gl:`#3190` + +- The catalog zone implementation has been optimized to work with + hundreds of thousands of member zones. :gl:`#3212` :gl:`#3744` From ac18df05910d0106cebd8b88d857587f4a88ff50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 21:39:37 +0100 Subject: [PATCH 13/18] Prepare release notes for BIND 9.19.9 --- doc/arm/notes.rst | 2 +- doc/notes/{notes-current.rst => notes-9.19.9.rst} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename doc/notes/{notes-current.rst => notes-9.19.9.rst} (100%) diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index f720d72907..b0f4f11abd 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -38,7 +38,7 @@ information about each release, and source code. .. include:: ../notes/notes-known-issues.rst -.. include:: ../notes/notes-current.rst +.. include:: ../notes/notes-9.19.9.rst .. include:: ../notes/notes-9.19.8.rst .. include:: ../notes/notes-9.19.7.rst .. include:: ../notes/notes-9.19.6.rst diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-9.19.9.rst similarity index 100% rename from doc/notes/notes-current.rst rename to doc/notes/notes-9.19.9.rst From 950870dd9ee6e1103870838f30c59896c7928407 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 21:39:37 +0100 Subject: [PATCH 14/18] Tweak and reword release notes --- doc/notes/notes-9.19.8.rst | 4 +-- doc/notes/notes-9.19.9.rst | 68 ++++++++++++++++++++------------------ 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/doc/notes/notes-9.19.8.rst b/doc/notes/notes-9.19.8.rst index 97530147b6..a0fed9bc86 100644 --- a/doc/notes/notes-9.19.8.rst +++ b/doc/notes/notes-9.19.8.rst @@ -44,8 +44,8 @@ Feature Changes - Setting alternate local addresses for inbound zone transfers has been deprecated. The relevant options (``alt-transfer-source``, - ``alt-transfer-source-v6``, and ``use-alt-transfer-source``) - will be removed in a future BIND 9.19.x release. :gl:`#3694` + ``alt-transfer-source-v6``, and ``use-alt-transfer-source``) will be + removed in a future BIND 9.19.x release. :gl:`#3694` - On startup, :iscman:`named` now sets the limit on the number of open files to the maximum allowed by the operating system, instead of diff --git a/doc/notes/notes-9.19.9.rst b/doc/notes/notes-9.19.9.rst index 3d44ce7067..465b04bc81 100644 --- a/doc/notes/notes-9.19.9.rst +++ b/doc/notes/notes-9.19.9.rst @@ -55,53 +55,57 @@ New Features Removed Features ~~~~~~~~~~~~~~~~ -- The options to set alternate local addresses for inbound zone transfers - are removed (``alt-transfer-source``, ``alt-transfer-source-v6``, - ``use-alt-transfer-source``). :gl:`#3694` +- The statements setting alternate local addresses for inbound zone + transfers (``alt-transfer-source``, ``alt-transfer-source-v6``, and + ``use-alt-transfer-source``) have been removed. :gl:`#3714` -- The Differentiated Services Code Point (DSCP) feature in BIND - has been non-operational since the new Network Manager was introduced - in BIND 9.16. It is now marked as obsolete, and vestigial code - implementing it has been removed. Configuring DSCP values in - ``named.conf`` will cause a warning to be logged. :gl:`#3773` +- The Differentiated Services Code Point (DSCP) feature in BIND has been + non-operational since the new Network Manager was introduced in BIND + 9.16. It is now marked as obsolete, and vestigial code implementing it + has been removed. Configuring DSCP values in ``named.conf`` now causes + a warning to be logged. :gl:`#3773` Feature Changes ~~~~~~~~~~~~~~~ -- Add the ability to configure the preferred source address when talking to - remote servers such as :any:`primaries` and any:`parental-agents`. - :gl:`!7110` +- A new way of configuring the preferred source address when talking to + remote servers, such as :any:`primaries` and :any:`parental-agents`, + has been added: setting the ``source`` and/or ``source-v6`` arguments + for a given statement is now possible. This new approach is intended + to eventually replace statements such as :any:`parental-source`, + :any:`parental-source-v6`, :any:`transfer-source`, etc. :gl:`#3762` -- Replace DNS over TCP and DNS over TLS transports code with a new, - unified transport implementation. :gl:`#3374` +- The code for DNS over TCP and DNS over TLS transports has been + replaced with a new, unified transport implementation. :gl:`#3374` Bug Fixes ~~~~~~~~~ -- TLS session resumption might lead to handshake failures when client - certificates are used for authentication (Mutual TLS). This has - been fixed. :gl:`#3725` +- Previously, TLS session resumption could have led to handshake + failures when client certificates were used for authentication (Mutual + TLS). This has been fixed. :gl:`#3725` -- When an outgoing request timed out, the ``named`` would retry up to three - times with the same server instead of trying a next available name server. - This has been fixed. :gl:`#3637` +- When an outgoing request timed out, :iscman:`named` would retry up to + three times with the same server instead of trying the next available + name server. This has been fixed. :gl:`#3637` -- Recently used ADB names and ADB entries (IP addresses) could get cleaned when - ADB would be under memory pressure. To mitigate this, count only actual ADB - names and ADB entries into the overmem memory limit (exclude internal memory - structures used for "housekeeping") and exclude recently used (<= 10 seconds) - ADB names and entries from the overmem memory cleaner. :gl:`#3739` +- Recently used ADB names and ADB entries (IP addresses) could get + cleaned when ADB was under memory pressure. To mitigate this, only + actual ADB names and ADB entries are now counted (excluding internal + memory structures used for "housekeeping") and recently used (<= 10 + seconds) ADB names and entries are excluded from the overmem memory + cleaner. :gl:`#3739` -- Fix a rare assertion failure in the outgoing TCP DNS connection handling. - :gl:`#3178` :gl:`#3636` +- A rare assertion failure was fixed in outgoing TCP DNS connection + handling. :gl:`#3178` :gl:`#3636` -- In addition to a previously fixed bug, another similar issue was discovered - where quotas could be erroneously reached for servers, including any - configured forwarders, resulting in SERVFAIL answers being sent to clients. - This has been fixed. :gl:`#3752` +- In addition to a previously fixed bug, another similar issue was + discovered where quotas could be erroneously reached for servers, + including any configured forwarders, resulting in SERVFAIL answers + being sent to clients. This has been fixed. :gl:`#3752` -- Clients may see an unexpected "Prohibited" extended DNS error when ``named`` - is configured with :any:`allow-recursion`). :gl:`#3743` +- The "Prohibited" Extended DNS Error was inadvertently set in some + NOERROR responses. This has been fixed. :gl:`#3743` Known Issues ~~~~~~~~~~~~ From fcd490500777acd585cc0d8ac65149e3763185c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 21:39:37 +0100 Subject: [PATCH 15/18] Reorder release notes --- doc/notes/notes-9.19.9.rst | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/notes/notes-9.19.9.rst b/doc/notes/notes-9.19.9.rst index 465b04bc81..2ee82bcd78 100644 --- a/doc/notes/notes-9.19.9.rst +++ b/doc/notes/notes-9.19.9.rst @@ -81,9 +81,13 @@ Feature Changes Bug Fixes ~~~~~~~~~ -- Previously, TLS session resumption could have led to handshake - failures when client certificates were used for authentication (Mutual - TLS). This has been fixed. :gl:`#3725` +- A rare assertion failure was fixed in outgoing TCP DNS connection + handling. :gl:`#3178` :gl:`#3636` + +- In addition to a previously fixed bug, another similar issue was + discovered where quotas could be erroneously reached for servers, + including any configured forwarders, resulting in SERVFAIL answers + being sent to clients. This has been fixed. :gl:`#3752` - When an outgoing request timed out, :iscman:`named` would retry up to three times with the same server instead of trying the next available @@ -96,17 +100,13 @@ Bug Fixes seconds) ADB names and entries are excluded from the overmem memory cleaner. :gl:`#3739` -- A rare assertion failure was fixed in outgoing TCP DNS connection - handling. :gl:`#3178` :gl:`#3636` - -- In addition to a previously fixed bug, another similar issue was - discovered where quotas could be erroneously reached for servers, - including any configured forwarders, resulting in SERVFAIL answers - being sent to clients. This has been fixed. :gl:`#3752` - - The "Prohibited" Extended DNS Error was inadvertently set in some NOERROR responses. This has been fixed. :gl:`#3743` +- Previously, TLS session resumption could have led to handshake + failures when client certificates were used for authentication (Mutual + TLS). This has been fixed. :gl:`#3725` + Known Issues ~~~~~~~~~~~~ From fadbbb94b3ed3b5e5ab89ccefc27f7d1c511b949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 21:39:37 +0100 Subject: [PATCH 16/18] Add release note for GL #3678 --- doc/notes/notes-9.19.9.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/notes/notes-9.19.9.rst b/doc/notes/notes-9.19.9.rst index 2ee82bcd78..14a773f508 100644 --- a/doc/notes/notes-9.19.9.rst +++ b/doc/notes/notes-9.19.9.rst @@ -89,6 +89,11 @@ Bug Fixes including any configured forwarders, resulting in SERVFAIL answers being sent to clients. This has been fixed. :gl:`#3752` +- In certain query resolution scenarios (e.g. when following CNAME + records), :iscman:`named` configured to answer from stale cache could + return a SERVFAIL response despite a usable, non-stale answer being + present in the cache. This has been fixed. :gl:`#3678` + - When an outgoing request timed out, :iscman:`named` would retry up to three times with the same server instead of trying the next available name server. This has been fixed. :gl:`#3637` From a48b5a2935528bdfbc9389be9d7fa6d4a58e2d95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 22:43:01 +0100 Subject: [PATCH 17/18] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 33f05c95c6..c68ec98b3a 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.19.9 released --- + 6067. [security] Fix serve-stale crash when recursive clients soft quota is reached. (CVE-2022-3924) [GL #3619] From a752887dce93b705a62b6f3769679c94fb43fbc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Thu, 12 Jan 2023 22:43:01 +0100 Subject: [PATCH 18/18] Update BIND version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index c5d0cd54fc..616b76c5eb 100644 --- a/configure.ac +++ b/configure.ac @@ -17,7 +17,7 @@ m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 19)dnl m4_define([bind_VERSION_PATCH], 9)dnl -m4_define([bind_VERSION_EXTRA], -dev)dnl +m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Development Release)])dnl m4_define([bind_SRCID], [m4_esyscmd_s([git rev-parse --short HEAD | cut -b1-7])])dnl m4_define([bind_PKG_VERSION], [[bind_VERSION_MAJOR.bind_VERSION_MINOR.bind_VERSION_PATCH]bind_VERSION_EXTRA])dnl