Merge branch '4464-multithread-tls-initialisation-crash-fix' into 'main'

Fix TLS certs store deletion on concurrent access

Closes #4464

See merge request isc-projects/bind9!8531

CHANGES

@@ -1,3 +1,9 @@
+6294.	[bug]		BIND might sometimes crash after startup or
+			re-configuration when one 'tls' entry is used multiple
+			times to connect to remote servers due to initialisation
+			attempts from contexts of multiple threads. That has
+			been fixed. [GL #4464]
+
 6293.	[func]		Initial support for accepting the PROXYv2 protocol in
 			all currently implemented DNS transports in BIND and
 			complementary support for sending it in dig are included

bin/tests/system/doth/ns1/named.conf.in

@@ -89,6 +89,7 @@ options {
 	listen-on port @EXTRAPORT4@ tls tls-expired { 10.53.0.1; };             // DoT
 	listen-on port @EXTRAPORT5@ tls tls-forward-secrecy-mutual-tls { 10.53.0.1; }; // DoT
 	listen-on port @EXTRAPORT6@ tls tls-forward-secrecy-mutual-tls http local { 10.53.0.1; }; // DoH
+	listen-on port @EXTRAPORT7@ tls tls-forward-secrecy { 10.53.0.1; }; // DoT
 	recursion no;
 	notify explicit;
 	also-notify { 10.53.0.2 port @PORT@; };
@@ -170,3 +171,27 @@ zone "example11" {
 	file "example.db";
 	allow-transfer port @EXTRAPORT5@ transport tls { any; };
 };
+
+zone "example12" {
+	type primary;
+	file "example.db";
+	allow-transfer port @EXTRAPORT7@ transport tls { any; };
+};
+
+zone "example13" {
+	type primary;
+	file "example.db";
+	allow-transfer port @EXTRAPORT7@ transport tls { any; };
+};
+
+zone "example14" {
+	type primary;
+	file "example.db";
+	allow-transfer port @EXTRAPORT7@ transport tls { any; };
+};
+
+zone "example15" {
+	type primary;
+	file "example.db";
+	allow-transfer port @EXTRAPORT7@ transport tls { any; };
+};

bin/tests/system/doth/ns5/named.conf.in

@@ -0,0 +1,83 @@
+/*
+ * 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.
+ */
+
+# We need a separate instance for the "rndc reconfig" test in order to
+# ensure that it does not use ephemeral keys (these are costly to
+# generate) and creates a minimal amount of TLS contexts, reducing the
+# time needed for startup/reconfiguration. Long
+# startup/reconfiguration was known to cause timeout issues in the CI
+# system, where many tests run in parallel.
+
+include "../../_common/rndc.key";
+
+controls {
+	inet 10.53.0.5 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+options {
+	query-source address 10.53.0.5;
+	notify-source 10.53.0.5;
+	transfer-source 10.53.0.5;
+	port @PORT@;
+	tls-port @TLSPORT@;
+	https-port @HTTPSPORT@;
+	http-port @HTTPPORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.5; };
+	listen-on-v6 { none; };
+	recursion no;
+	notify no;
+	ixfr-from-differences yes;
+	check-integrity no;
+	dnssec-validation yes;
+};
+
+zone "." {
+	type hint;
+	file "../../_common/root.hint";
+};
+
+# Let's reuse the same entry multiple times to see if transfers will succeed
+
+tls tls-v1.2 {
+	protocols { TLSv1.2; };
+	prefer-server-ciphers yes;
+};
+
+zone "example12" {
+	type secondary;
+	primaries { 10.53.0.1 port @EXTRAPORT7@ tls tls-v1.2; };
+	file "example12.db";
+	allow-transfer { any; };
+};
+
+zone "example13" {
+	type secondary;
+	primaries { 10.53.0.1 port @EXTRAPORT7@ tls tls-v1.2; };
+	file "example13.db";
+	allow-transfer { any; };
+};
+
+zone "example14" {
+	type secondary;
+	primaries { 10.53.0.1 port @EXTRAPORT7@ tls tls-v1.2; };
+	file "example14.db";
+	allow-transfer { any; };
+};
+
+zone "example15" {
+	type secondary;
+	primaries { 10.53.0.1 port @EXTRAPORT7@ tls tls-v1.2; };
+	file "example15.db";
+	allow-transfer { any; };
+};

bin/tests/system/doth/setup.sh

@@ -30,3 +30,4 @@ copy_setports ns1/named.conf.in ns1/named.conf
 copy_setports ns2/named.conf.in ns2/named.conf
 copy_setports ns3/named.conf.in ns3/named.conf
 copy_setports ns4/named.conf.in ns4/named.conf
+copy_setports ns5/named.conf.in ns5/named.conf

bin/tests/system/doth/tests.sh

@@ -884,5 +884,37 @@ if [ -n "$testcurl" ]; then
   status=$((status + ret))
 fi
 
+n=$((n + 1))
+echo_i "checking Do53 query to NS5 for zone \"example12\" (verifying successful client TLS context reuse by the NS5 server instance during XoT) ($n)"
+ret=0
+dig_with_opts +comm @10.53.0.5 example12 SOA >dig.out.test$n || ret=1
+grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+
+n=$((n + 1))
+echo_i "checking Do53 query to NS5 for zone \"example13\" (verifying successful client TLS context reuse by the NS5 server instance during XoT) ($n)"
+ret=0
+dig_with_opts +comm @10.53.0.5 example13 SOA >dig.out.test$n || ret=1
+grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+
+n=$((n + 1))
+echo_i "checking Do53 query to NS5 for zone \"example14\" (verifying successful client TLS context reuse by the NS5 server instance during XoT) ($n)"
+ret=0
+dig_with_opts +comm @10.53.0.5 example14 SOA >dig.out.test$n || ret=1
+grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+
+n=$((n + 1))
+echo_i "checking Do53 query to NS5 for zone \"example15\" (verifying successful client TLS context reuse by the NS5 server instance during XoT) ($n)"
+ret=0
+dig_with_opts +comm @10.53.0.5 example15 SOA >dig.out.test$n || ret=1
+grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

lib/dns/transport.c

@@ -524,7 +524,24 @@ dns_transport_get_tlsctx(dns_transport_t *transport, const isc_sockaddr_t *peer,
 			 */
 			INSIST(found != NULL);
 			isc_tlsctx_free(&tlsctx);
-			isc_tls_cert_store_free(&store);
+			/*
+			 * The 'store' variable can be 'NULL' when remote server
+			 * verification is not enabled (that is, when Strict or
+			 * Mutual TLS are not used).
+			 *
+			 * The 'found_store' might be equal to 'store' as there
+			 * is one-to-many relation between a store and
+			 * per-transport TLS contexts. In that case, the call to
+			 * 'isc_tlsctx_cache_find()' above could have returned a
+			 * store via the 'found_store' variable, whose value we
+			 * can assign to 'store' later. In that case,
+			 * 'isc_tlsctx_cache_add()' will return the same value.
+			 * When that happens, we should not free the store
+			 * object, as it is managed by the TLS context cache.
+			 */
+			if (store != NULL && store != found_store) {
+				isc_tls_cert_store_free(&store);
+			}
 			isc_tlsctx_client_session_cache_detach(&sess_cache);
 			/* Let's return the data from the cache. */
 			*psess_cache = found_sess_cache;