From: Artem Boldariev <artem@boldariev.com>
Date: Sat, 2 Jul 2022 00:31:35 +0000 (+0300)
Subject: TLS: do not ignore accept callback result
X-Git-Tag: v9.19.4~40^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ffcb54211e148ecc7e145f6ea79a0ab2a58a2287;p=thirdparty%2Fbind9.git

TLS: do not ignore accept callback result

Before this change the TLS code would ignore the accept callback result,
and would not try to gracefully close the connection. This had not been
noticed, as it is not really required for DoH. Now the code tries to
shut down the TLS connection gracefully when accepting it is not
successful.
---

diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c
index 7ba09d8f450..169b3910028 100644
--- a/lib/isc/netmgr/tlsstream.c
+++ b/lib/isc/netmgr/tlsstream.c
@@ -323,7 +323,7 @@ tls_process_outgoing(isc_nmsocket_t *sock, bool finish,
 }
 
 static int
-tls_try_handshake(isc_nmsocket_t *sock) {
+tls_try_handshake(isc_nmsocket_t *sock, isc_result_t *presult) {
 	int rv = 0;
 	isc_nmhandle_t *tlshandle = NULL;
 
@@ -341,13 +341,18 @@ tls_try_handshake(isc_nmsocket_t *sock) {
 		isc__nmsocket_log_tls_session_reuse(sock, sock->tlsstream.tls);
 		tlshandle = isc__nmhandle_get(sock, &sock->peer, &sock->iface);
 		if (sock->tlsstream.server) {
-			sock->listener->accept_cb(tlshandle, result,
-						  sock->listener->accept_cbarg);
+			result = sock->listener->accept_cb(
+				tlshandle, result,
+				sock->listener->accept_cbarg);
 		} else {
 			tls_call_connect_cb(sock, tlshandle, result);
 		}
 		isc_nmhandle_detach(&tlshandle);
 		sock->tlsstream.state = TLS_IO;
+
+		if (presult != NULL) {
+			*presult = result;
+		}
 	}
 
 	return (rv);
@@ -396,7 +401,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
 			SSL_set_connect_state(sock->tlsstream.tls);
 		}
 		sock->tlsstream.state = TLS_HANDSHAKE;
-		rv = tls_try_handshake(sock);
+		rv = tls_try_handshake(sock, NULL);
 		INSIST(SSL_is_init_finished(sock->tlsstream.tls) == 0);
 	} else if (sock->tlsstream.state == TLS_CLOSED) {
 		return;
@@ -419,7 +424,21 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
 			 * handshake is done.
 			 */
 			if (sock->tlsstream.state == TLS_HANDSHAKE) {
-				rv = tls_try_handshake(sock);
+				isc_result_t hs_result = ISC_R_UNSET;
+				rv = tls_try_handshake(sock, &hs_result);
+				if (sock->tlsstream.state == TLS_IO &&
+				    hs_result != ISC_R_SUCCESS) {
+					/*
+					 * The accept callback has been called
+					 * unsuccessfully. Let's try to shut
+					 * down the TLS connection gracefully.
+					 */
+					INSIST(SSL_is_init_finished(
+						       sock->tlsstream.tls) ==
+					       1);
+					INSIST(!atomic_load(&sock->client));
+					finish = true;
+				}
 			}
 		} else if (send_data != NULL) {
 			INSIST(received_data == NULL);
@@ -447,7 +466,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
 		/* Decrypt and pass data from network to client */
 		if (sock->tlsstream.state >= TLS_IO && sock->recv_cb != NULL &&
 		    !atomic_load(&sock->readpaused) &&
-		    sock->statichandle != NULL)
+		    sock->statichandle != NULL && !finish)
 		{
 			uint8_t recv_buf[TLS_BUF_SIZE];
 			INSIST(sock->tlsstream.state > TLS_HANDSHAKE);
@@ -466,9 +485,9 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
 				 * nullified (it happens in netmgr.c). If it is
 				 * the case, then it means that we are not
 				 * interested in keeping the connection alive
-				 * anymore. Let's shutdown the SSL session, send
-				 * what we have in the SSL buffers, and close
-				 * the connection.
+				 * anymore. Let's shut down the SSL session,
+				 * send what we have in the SSL buffers,
+				 * and close the connection.
 				 */
 				if (sock->statichandle == NULL) {
 					finish = true;
@@ -526,8 +545,13 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
 			return;
 		}
 
+		if (sock->outerhandle == NULL) {
+			return;
+		}
+
+		INSIST(VALID_NMHANDLE(sock->outerhandle));
+
 		if (sock->tlsstream.reading) {
-			INSIST(VALID_NMHANDLE(sock->outerhandle));
 			isc_nm_resumeread(sock->outerhandle);
 		} else if (sock->tlsstream.state == TLS_HANDSHAKE) {
 			sock->tlsstream.reading = true;
@@ -1080,8 +1104,8 @@ isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) {
 	} else if (sock->type == isc_nm_tlssocket) {
 		if (sock->tlsstream.tls != NULL) {
 			/*
-			 * Let's shutdown the TLS session properly so that the
-			 * session will remain resumable, if required.
+			 * Let's shut down the TLS session properly so that
+			 * the session will remain resumable, if required.
 			 */
 			tls_try_shutdown(sock->tlsstream.tls, true);
 			tls_keep_client_tls_session(sock);