From c409b9a9393d5c8392664554d787da48c982cde5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 18 Mar 2026 03:55:51 +0100 Subject: [PATCH 1/2] Fix TOCTOU race in DNS UPDATE SSU table handling Pass the SSU table through the update event struct from send_update() to update_action() instead of reading it from the zone twice. If rndc reconfig changed the zone's update policy between the two reads (e.g., from allow-update to update-policy), send_update() would skip the maxbytype allocation but update_action() would see a non-NULL ssutable, triggering INSIST(ssutable == NULL || maxbytype != NULL) and crashing named. The ssutable reference is now taken once in send_update() and transferred to update_action() via the event struct, ensuring both functions see the same value. (cherry picked from commit c172416559e62a31de27061648db7ffe3b1b7f63) --- lib/ns/update.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/ns/update.c b/lib/ns/update.c index bd5cac2623..1314889d7d 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -201,6 +201,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; }; @@ -1857,14 +1858,14 @@ send_update(ns_client_t *client, dns_zone_t *zone) { *uev = (update_t){ .zone = zone, .client = client, - .maxbytype = maxbytype, + .ssutable = TAKE_OWNERSHIP(ssutable), + .maxbytype = TAKE_OWNERSHIP(maxbytype), .maxbytypelen = maxbytypelen, .result = ISC_R_SUCCESS, }; isc_nmhandle_attach(client->handle, &client->updatehandle); isc_async_run(dns_zone_getloop(zone), update_action, uev); - maxbytype = NULL; cleanup: if (db != NULL) { @@ -2697,6 +2698,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; @@ -2711,7 +2713,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; @@ -2728,7 +2729,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)); From feb5dc7f984da1ddd0185a7dd3c21bdeb791f516 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 18 Mar 2026 04:09:50 +0100 Subject: [PATCH 2/2] Add regression test for TOCTOU race in DNS UPDATE SSU handling Race rndc reconfig (toggling between allow-update and update-policy) against a stream of DNS UPDATEs for 5 seconds and verify that named does not crash. Before the fix, the race between send_update() and update_action() reading the SSU table independently could trigger an assertion failure (INSIST) when the zone's update policy changed between the two reads. (cherry picked from commit c503b6eee8e0b7ae9c12af1b6ab2630926f6648d) --- REUSE.toml | 1 + bin/tests/system/ssutoctou/ns1/example.db.in | 10 ++ bin/tests/system/ssutoctou/ns1/named.conf.j2 | 45 ++++++++ bin/tests/system/ssutoctou/setup.sh | 17 +++ bin/tests/system/ssutoctou/tests_ssutoctou.py | 104 ++++++++++++++++++ lib/ns/update.c | 4 +- 6 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 bin/tests/system/ssutoctou/ns1/example.db.in create mode 100644 bin/tests/system/ssutoctou/ns1/named.conf.j2 create mode 100755 bin/tests/system/ssutoctou/setup.sh create mode 100644 bin/tests/system/ssutoctou/tests_ssutoctou.py diff --git a/REUSE.toml b/REUSE.toml index 493ee5ecd2..dad0e26e65 100644 --- a/REUSE.toml +++ b/REUSE.toml @@ -10,6 +10,7 @@ path = [ "**/**.batch", "**/**.before**", "**/**.ccache", + "**/**.db**", "**/**.good", "**/**.key", "**/**.keytab", diff --git a/bin/tests/system/ssutoctou/ns1/example.db.in b/bin/tests/system/ssutoctou/ns1/example.db.in new file mode 100644 index 0000000000..9b0498c821 --- /dev/null +++ b/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 diff --git a/bin/tests/system/ssutoctou/ns1/named.conf.j2 b/bin/tests/system/ssutoctou/ns1/named.conf.j2 new file mode 100644 index 0000000000..87b867a5b7 --- /dev/null +++ b/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 %} +}; diff --git a/bin/tests/system/ssutoctou/setup.sh b/bin/tests/system/ssutoctou/setup.sh new file mode 100755 index 0000000000..24a0026665 --- /dev/null +++ b/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 diff --git a/bin/tests/system/ssutoctou/tests_ssutoctou.py b/bin/tests/system/ssutoctou/tests_ssutoctou.py new file mode 100644 index 0000000000..9e7b32298b --- /dev/null +++ b/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) diff --git a/lib/ns/update.c b/lib/ns/update.c index 1314889d7d..547927b08c 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -1858,8 +1858,8 @@ send_update(ns_client_t *client, dns_zone_t *zone) { *uev = (update_t){ .zone = zone, .client = client, - .ssutable = TAKE_OWNERSHIP(ssutable), - .maxbytype = TAKE_OWNERSHIP(maxbytype), + .ssutable = MOVE_OWNERSHIP(ssutable), + .maxbytype = MOVE_OWNERSHIP(maxbytype), .maxbytypelen = maxbytypelen, .result = ISC_R_SUCCESS, };