From bd41100295ac4ddd194885acc5889152e1ccbc78 Mon Sep 17 00:00:00 2001
From: Artem Boldariev <artem@boldariev.com>
Date: Tue, 22 Mar 2022 20:24:46 +0200
Subject: [PATCH] Fix a crash by avoiding destroying TLS stream socket too
 early

This commit fixes a crash in generic TLS stream code, which could be
reproduced during some runs of the 'sslyze' tool.

The intention of this commit is twofold.

Firstly, it ensures that the TLS socket object cannot be destroyed too
early. Now it is being deleted alongside the underlying TCP socket
object.

Secondly, it ensures that the TLS socket object cannot be destroyed as
a result of calling 'tls_do_bio()' (the primary function which
performs encryption/decryption during the IO) as the code did not
expect that. This code path is fixed now.

(cherry picked from commit a696be6a2db0a6dedb87ba37959112ad394989b7)
---
 lib/isc/netmgr/netmgr-int.h |  1 +
 lib/isc/netmgr/tlsstream.c  | 43 ++++++++++++++++++++-----------------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 3c31b2cc76..650ee7aa49 100644
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -972,6 +972,7 @@ struct isc_nmsocket {
 						    worker */
 		size_t n_listener_tls_ctx;
 		isc_nmsocket_t *tlslistener;
+		isc_nmsocket_t *tlssocket;
 		atomic_bool result_updated;
 		enum {
 			TLS_INIT,
diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c
index 11b895bab1..4fd909d228 100644
--- a/lib/isc/netmgr/tlsstream.c
+++ b/lib/isc/netmgr/tlsstream.c
@@ -214,7 +214,6 @@ tls_failed_read_cb(isc_nmsocket_t *sock, const isc_result_t result) {
 
 	if (destroy) {
 		isc__nmsocket_prep_destroy(sock);
-		isc__nmsocket_detach(&sock);
 	}
 }
 
@@ -416,21 +415,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
 				send_data->cb.send(send_data->handle, result,
 						   send_data->cbarg);
 				send_data = NULL;
-				/* This situation might occur only when SSL
-				 * shutdown was already sent (see
-				 * tls_send_outgoing()), and we are in the
-				 * process of shutting down the connection (in
-				 * this case tls_senddone() will be called), but
-				 * some code tries to send data over the
-				 * connection and called isc_tls_send(). The
-				 * socket will be detached there, in
-				 * tls_senddone().*/
-				if (sent_shutdown || received_shutdown) {
-					return;
-				} else {
-					isc__nmsocket_detach(&sock);
-					return;
-				}
+				return;
 			}
 		}
 
@@ -634,6 +619,12 @@ tlslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 	tlssock->read_timeout = atomic_load(&handle->sock->mgr->init);
 	tlssock->tid = tid;
 
+	/*
+	 * Hold a reference to tlssock in the TCP socket: it will
+	 * detached in isc__nm_tls_cleanup_data().
+	 */
+	handle->sock->tlsstream.tlssocket = tlssock;
+
 	result = initialize_tls(tlssock, true);
 	RUNTIME_CHECK(result == ISC_R_SUCCESS);
 	/* TODO: catch failure code, detach tlssock, and log the error */
@@ -834,7 +825,7 @@ tls_close_direct(isc_nmsocket_t *sock) {
 		isc__nmsocket_detach(&sock->listener);
 	}
 
-	/* further cleanup performed in isc__nm_tls_cleanup_data() */
+	/* Further cleanup performed in isc__nm_tls_cleanup_data() */
 	atomic_store(&sock->closed, true);
 	atomic_store(&sock->active, false);
 	sock->tlsstream.state = TLS_CLOSED;
@@ -958,6 +949,12 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 	isc_nmhandle_attach(handle, &tlssock->outerhandle);
 	atomic_store(&tlssock->active, true);
 
+	/*
+	 * Hold a reference to tlssock in the TCP socket: it will
+	 * detached in isc__nm_tls_cleanup_data().
+	 */
+	handle->sock->tlsstream.tlssocket = tlssock;
+
 	tls_do_bio(tlssock, NULL, NULL, false);
 	return;
 error:
@@ -1025,8 +1022,9 @@ void
 isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) {
 	if (sock->type == isc_nm_tcplistener &&
 	    sock->tlsstream.tlslistener != NULL) {
-		REQUIRE(VALID_NMSOCK(sock->tlsstream.tlslistener));
 		isc__nmsocket_detach(&sock->tlsstream.tlslistener);
+	} else if (sock->type == isc_nm_tlslistener) {
+		tls_cleanup_listener_tlsctx(sock);
 	} else if (sock->type == isc_nm_tlssocket) {
 		if (sock->tlsstream.ctx != NULL) {
 			isc_tlsctx_free(&sock->tlsstream.ctx);
@@ -1037,8 +1035,13 @@ isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) {
 			sock->tlsstream.bio_out = NULL;
 			sock->tlsstream.bio_in = NULL;
 		}
-	} else if (sock->type == isc_nm_tlslistener) {
-		tls_cleanup_listener_tlsctx(sock);
+	} else if (sock->type == isc_nm_tcpsocket &&
+		   sock->tlsstream.tlssocket != NULL) {
+		/*
+		 * The TLS socket can't be destroyed until its underlying TCP
+		 * socket is, to avoid possible use-after-free errors.
+		 */
+		isc__nmsocket_detach(&sock->tlsstream.tlssocket);
 	}
 }