From b109fa919283cc7bdd992e94d94b66e13d44f1a1 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 4 Dec 2023 14:28:28 +0200 Subject: [PATCH 1/3] Fix TLS certs store deletion on concurrent access During initialisation or reconfiguration, it is possible that multiple threads are trying to create a TLS context and associated data (like TLS certs store) concurrently. In some cases, a thread might be too late to add newly created data to the TLS contexts cache, in which case it needs to be discarded. In the code that handles that case, it was not taken into account that, in some cases, the TLS certs store could not have been created or should not be deleted, as it is being managed by the TLS contexts cache already. Deleting the store in such cases might lead to crashes. This commit fixes the issue. --- lib/dns/transport.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/dns/transport.c b/lib/dns/transport.c index c7e01c0af4..fb0c4ffb3d 100644 --- a/lib/dns/transport.c +++ b/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; From 10e626111fccb8eeff1cce5943867811c757fc5a Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 4 Dec 2023 15:12:57 +0200 Subject: [PATCH 2/3] doth test: add a secondary NS instance that reuses a 'tls' entry This commit extends the 'doth' system tests with additional secondary NS instance that reuses the same 'tls' entry for connecting the the primary to download zones. This configurations were known to crash secondaries in some cases. --- bin/tests/system/doth/ns1/named.conf.in | 25 ++++++++ bin/tests/system/doth/ns5/named.conf.in | 83 +++++++++++++++++++++++++ bin/tests/system/doth/setup.sh | 1 + bin/tests/system/doth/tests.sh | 32 ++++++++++ 4 files changed, 141 insertions(+) create mode 100644 bin/tests/system/doth/ns5/named.conf.in diff --git a/bin/tests/system/doth/ns1/named.conf.in b/bin/tests/system/doth/ns1/named.conf.in index b0247dc2d4..6a8bcdbda5 100644 --- a/bin/tests/system/doth/ns1/named.conf.in +++ b/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; }; +}; diff --git a/bin/tests/system/doth/ns5/named.conf.in b/bin/tests/system/doth/ns5/named.conf.in new file mode 100644 index 0000000000..6808618882 --- /dev/null +++ b/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; }; +}; diff --git a/bin/tests/system/doth/setup.sh b/bin/tests/system/doth/setup.sh index 6672a6194d..775dd33a18 100644 --- a/bin/tests/system/doth/setup.sh +++ b/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 diff --git a/bin/tests/system/doth/tests.sh b/bin/tests/system/doth/tests.sh index 2080bd6053..aad23527a2 100644 --- a/bin/tests/system/doth/tests.sh +++ b/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 From f7de76616803a87c9426af9790bf05fe92f6f850 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 4 Dec 2023 14:42:30 +0200 Subject: [PATCH 3/3] Update CHANGES [GL #4464] Mention that BIND crashing due to a `tls` multithreaded entry initialisation attempts has been fixed. --- CHANGES | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGES b/CHANGES index ffa2b28ed1..145917df93 100644 --- a/CHANGES +++ b/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