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)

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 3c31b2cc76c62fedc675e2ef33464083bcabebfb..650ee7aa496db5eef3a7209d1b934cdddc4323df 100644 (file)
--- 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 11b895bab11f17a255540c4f269d22f799e0d6eb..4fd909d228996ddca4138c3bdc858caf5a1a0aeb 100644 (file)
--- 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);
        }
 }