sec: usr: Fix crash when reconfiguring zone update policy during active updates

Fixed a crash that could occur when running rndc reconfig to change a zone's update policy (e.g., from allow-update to update-policy) while DNS UPDATE requests were being processed for that zone.

ISC would like to thank Vitaly Simonovich for bringing this issue to our attention.

Fixes #5817

Merge branch '5817-fix-crash-via-SSU-table-desynchronization' into 'main'

See merge request isc-projects/bind9!11707

REUSE.toml

@@ -10,6 +10,7 @@ path = [
 	"**/**.batch",
 	"**/**.before**",
 	"**/**.ccache",
+	"**/**.db**",
 	"**/**.good",
 	"**/**.key",
 	"**/**.key.in",

bin/tests/system/ssutoctou/ns1/example.db.in

@@ -0,0 +1,10 @@
+$TTL 300
+@	IN SOA	ns.example. admin.example. (
+			1	; serial
+			3600	; refresh
+			900	; retry
+			604800	; expire
+			300	; minimum
+		)
+@	IN NS	ns.example.
+ns	IN A	10.53.0.1

bin/tests/system/ssutoctou/ns1/named.conf.j2

@@ -0,0 +1,45 @@
+/*
+ * 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.
+ */
+
+{% set use_ssu = use_ssu | default(False) %}
+
+options {
+	query-source address 10.53.0.1;
+	notify-source 10.53.0.1;
+	transfer-source 10.53.0.1;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.1; };
+	listen-on-v6 { none; };
+	recursion no;
+	dnssec-validation no;
+};
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm @DEFAULT_HMAC@;
+};
+
+controls {
+	inet 10.53.0.1 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+zone "example" {
+	type primary;
+	file "example.db";
+{% if use_ssu %}
+	update-policy { grant * self * A; };
+{% else %}
+	allow-update { any; };
+{% endif %}
+};

bin/tests/system/ssutoctou/setup.sh

@@ -0,0 +1,17 @@
+#!/bin/sh -e
+
+# 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.
+
+# shellcheck source=conf.sh
+. ../conf.sh
+
+cp ns1/example.db.in ns1/example.db

bin/tests/system/ssutoctou/tests_ssutoctou.py

@@ -0,0 +1,104 @@
+# 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.
+
+"""
+Regression test for GL#5006: TOCTOU race in DNS UPDATE SSU table handling.
+
+send_update() and update_action() used to independently read the zone's
+SSU table.  If rndc reconfig changed the zone's update policy between
+these two reads, the values could diverge, causing an assertion failure.
+
+This test races rndc reconfig (toggling between allow-update and
+update-policy) against a stream of DNS UPDATEs to verify that named
+survives without crashing.
+"""
+
+import threading
+import time
+
+import dns.query
+import dns.rdatatype
+import dns.update
+import pytest
+
+import isctest
+
+pytestmark = pytest.mark.extra_artifacts(
+    [
+        "*/*.db",
+        "*/*.jnl",
+    ]
+)
+
+
+def send_updates(ip, port, stop_event):
+    """Send DNS UPDATEs in a tight loop until stopped."""
+    n = 0
+    while not stop_event.is_set():
+        n += 1
+        try:
+            up = dns.update.UpdateMessage("example.")
+            up.add(
+                f"test{n}.example.",
+                300,
+                dns.rdatatype.A,
+                f"10.0.0.{n % 256}",
+            )
+            dns.query.tcp(up, ip, port=port, timeout=2)
+        except Exception:  # pylint: disable=broad-exception-caught
+            pass
+
+
+def toggle_config(ns1, templates, stop_event):
+    """Toggle zone config between allow-update and update-policy."""
+    use_ssu = False
+    while not stop_event.is_set():
+        use_ssu = not use_ssu
+        try:
+            templates.render("ns1/named.conf", {"use_ssu": use_ssu})
+            ns1.rndc("reconfig")
+        except Exception:  # pylint: disable=broad-exception-caught
+            pass
+        time.sleep(0.01)
+
+
+def test_ssu_toctou_race(ns1, templates):
+    """Race rndc reconfig against DNS UPDATEs -- named must not crash."""
+    port = int(isctest.vars.ALL["PORT"])
+    stop = threading.Event()
+
+    update_thread = threading.Thread(
+        target=send_updates,
+        args=("10.53.0.1", port, stop),
+    )
+    reconfig_thread = threading.Thread(
+        target=toggle_config,
+        args=(ns1, templates, stop),
+    )
+
+    update_thread.start()
+    reconfig_thread.start()
+
+    # Let them race for a few seconds
+    time.sleep(5)
+
+    stop.set()
+    update_thread.join(timeout=10)
+    reconfig_thread.join(timeout=10)
+
+    # Restore original config
+    templates.render("ns1/named.conf", {"use_ssu": False})
+    ns1.rndc("reconfig")
+
+    # Verify named is still alive
+    msg = isctest.query.create("ns.example.", "A")
+    res = isctest.query.udp(msg, "10.53.0.1")
+    isctest.check.noerror(res)

lib/ns/update.c

@@ -202,6 +202,7 @@ struct update {
 	ns_client_t *client;
 	isc_result_t result;
 	dns_message_t *answer;
+	dns_ssutable_t *ssutable;
 	unsigned int *maxbytype;
 	size_t maxbytypelen;
 };
@@ -1802,14 +1803,14 @@ send_update(ns_client_t *client, dns_zone_t *zone) {
 	*uev = (update_t){
 		.zone = zone,
 		.client = client,
-		.maxbytype = maxbytype,
+		.ssutable = MOVE_OWNERSHIP(ssutable),
+		.maxbytype = MOVE_OWNERSHIP(maxbytype),
 		.maxbytypelen = maxbytypelen,
 		.result = ISC_R_SUCCESS,
 	};
 
 	isc_nmhandle_attach(client->inner.handle, &client->inner.updatehandle);
 	isc_async_run(dns_zone_getloop(zone), update_action, uev);
-	maxbytype = NULL;
 
 cleanup:
 	if (db != NULL) {
@@ -2608,6 +2609,7 @@ update_action(void *arg) {
 	update_t *uev = (update_t *)arg;
 	dns_zone_t *zone = uev->zone;
 	ns_client_t *client = uev->client;
+	dns_ssutable_t *ssutable = uev->ssutable;
 	unsigned int *maxbytype = uev->maxbytype;
 	size_t update = 0, maxbytypelen = uev->maxbytypelen;
 	isc_result_t result;
@@ -2622,7 +2624,6 @@ update_action(void *arg) {
 	dns_message_t *request = client->message;
 	dns_rdataclass_t zoneclass;
 	dns_name_t *zonename = NULL;
-	dns_ssutable_t *ssutable = NULL;
 	dns_fixedname_t tmpnamefixed;
 	dns_name_t *tmpname = NULL;
 	dns_zoneopt_t options;
@@ -2639,7 +2640,6 @@ update_action(void *arg) {
 	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);
 
 	is_inline = (!dns_zone_israw(zone) && dns_zone_issecure(zone));