Fix memory management in port_make_channel

Also make port_new_handshake_layer processing clearer.

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27562)

(cherry picked from commit d56f9b4d894d1a3ce92f1c308ef42398495943e7)

diff --git a/ssl/quic/quic_channel_local.h b/ssl/quic/quic_channel_local.h
index cdd0969586dff3b4e4cbde5b45720fc25fa974e1..816ccf694b8f4d0ff268353e2f9ec311c4516120 100644 (file)
--- a/ssl/quic/quic_channel_local.h
+++ b/ssl/quic/quic_channel_local.h
@@ -41,7 +41,10 @@ struct quic_channel_st {
     /*
      * The associated TLS 1.3 connection data. Used to provide the handshake
      * layer; its 'network' side is plugged into the crypto stream for each EL
-     * (other than the 0-RTT EL).
+     * (other than the 0-RTT EL). Note that the `tls` SSL object is not "owned"
+     * by this channel. It is created and managed elsewhere and is guaranteed
+     * to be valid for the lifetime of the channel. Therefore we do not free it
+     * when we free the channel.
      */
     QUIC_TLS                        *qtls;
     SSL                             *tls;

diff --git a/ssl/quic/quic_port.c b/ssl/quic/quic_port.c
index 9097f56aa1c316f74d1402fa30ef0e9de5294659..684c088c08c050c446898ef2292ebbb80b67382f 100644 (file)
--- a/ssl/quic/quic_port.c
+++ b/ssl/quic/quic_port.c
@@ -458,34 +458,38 @@ static SSL *port_new_handshake_layer(QUIC_PORT *port, QUIC_CHANNEL *ch)
     QUIC_CONNECTION *qc = NULL;
     QUIC_LISTENER *ql = NULL;
 
-    if (port->get_conn_user_ssl != NULL) {
-        user_ssl = port->get_conn_user_ssl(ch, port->user_ssl_arg);
-        if (user_ssl == NULL)
-            return NULL;
-        qc = (QUIC_CONNECTION *)user_ssl;
-        ql = (QUIC_LISTENER *)port->user_ssl_arg;
+    /*
+     * It only makes sense to call this function if we know how to associate
+     * the handshake layer we are about to create with some user_ssl object.
+     */
+    if (!ossl_assert(port->get_conn_user_ssl != NULL))
+        return NULL;
+    user_ssl = port->get_conn_user_ssl(ch, port->user_ssl_arg);
+    if (user_ssl == NULL)
+        return NULL;
+    qc = (QUIC_CONNECTION *)user_ssl;
+    ql = (QUIC_LISTENER *)port->user_ssl_arg;
+
+    /*
+     * We expect the user_ssl to be newly created so it must not have an
+     * existing qc->tls
+     */
+    if (!ossl_assert(qc->tls == NULL)) {
+        SSL_free(user_ssl);
+        return NULL;
     }
 
     tls = ossl_ssl_connection_new_int(port->channel_ctx, user_ssl, TLS_method());
+    qc->tls = tls;
     if (tls == NULL || (tls_conn = SSL_CONNECTION_FROM_SSL(tls)) == NULL) {
         SSL_free(user_ssl);
         return NULL;
     }
 
-    /*
-     * If we got a user ssl, which will be embedded in a quic connection
-     * we need to fix up the connections tls pointer here
-     */
-    if (qc != NULL)
-        qc->tls = tls;
-
     if (ql != NULL && ql->obj.ssl.ctx->new_pending_conn_cb != NULL)
         if (!ql->obj.ssl.ctx->new_pending_conn_cb(ql->obj.ssl.ctx, user_ssl,
                                                   ql->obj.ssl.ctx->new_pending_conn_arg)) {
-            SSL_free(tls);
             SSL_free(user_ssl);
-            if (qc != NULL)
-                qc->tls = NULL;
             return NULL;
         }
 
@@ -531,6 +535,11 @@ static QUIC_CHANNEL *port_make_channel(QUIC_PORT *port, SSL *tls, OSSL_QRX *qrx,
      */
     ch->tls = (tls != NULL) ? tls : port_new_handshake_layer(port, ch);
 
+    if (ch->tls == NULL) {
+        OPENSSL_free(ch);
+        return NULL;
+    }
+
 #ifndef OPENSSL_NO_QLOG
     /*
      * If we're using qlog, make sure the tls get further configured properly
@@ -548,7 +557,6 @@ static QUIC_CHANNEL *port_make_channel(QUIC_PORT *port, SSL *tls, OSSL_QRX *qrx,
      * And finally init the channel struct
      */
     if (!ossl_quic_channel_init(ch)) {
-        SSL_free(ch->tls);
         OPENSSL_free(ch);
         return NULL;
     }