Fix memory leak in pre-allocated listeners

We have a chicken and egg problem.

Normally when we create a connection object in quic, we associate it
with a listener, and up the ref on the parent listener, which is fine.

However, now that we are pre-allocating user_ssl objects for incomming
connections we have a situation in which:
1) The pre-alocated connection object holds a ref on the listener
2) The application has no awareness of the quic connection object (and
   so can't free it)
3) The freeing of the listener object never calls into the quic stack,
   because its reference count may hold references from connections that
   haven't been accepted yet

We could require that applications register a function for the
new_pending_conn callback, and track/free these pending connections, but
that seems like alot of extra unneeded work to place on the application

Instead:
a) add a quic_conn_st flag named accepted
b) When pre-allocating connections, clear the flag in (a) and _dont_
   hold a reference to the parent listener
c) in SSL_accept_connection, set the accepted flag and reference the
listener
d) in ossl_quic_free drop the listener reference only if the accepted
flag is set
c) expressly free all user_ssl objects in ossl_quic_port_drop_incoming

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

diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index 735481634bb749a6cbc33b8668b40c1c8ddda005..7689879c2460c3897f52c5223bdc94ec26b609ab 100644 (file)
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -805,7 +805,13 @@ void ossl_quic_free(SSL *s)
     qc_cleanup(ctx.qc, /*have_lock=*/1);
     /* Note: SSL_free calls OPENSSL_free(qc) for us */
 
-    if (ctx.qc->listener != NULL)
+    /*
+     * Just because we have a listener doesn't mean we have a ref on it
+     * QUIC pre-creates user ssls to return to applications.
+     * Those don't hold a reference until they are accepted, so only drop
+     * the count if the application has accepted them
+     */
+    if (ctx.qc->accepted == 1 && ctx.qc->listener != NULL)
         SSL_free(&ctx.qc->listener->obj.ssl);
     if (ctx.qc->domain != NULL)
         SSL_free(&ctx.qc->domain->obj.ssl);
@@ -4455,6 +4461,7 @@ SSL *ossl_quic_new_from_listener(SSL *ssl, uint64_t flags)
 
     qctx_unlock(&ctx);
 
+    qc->accepted = 1;
     return &qc->obj.ssl;
 
 err:
@@ -4524,6 +4531,7 @@ SSL *ossl_quic_accept_connection(SSL *ssl, uint64_t flags)
     QCTX ctx;
     SSL *conn_ssl = NULL;
     QUIC_CHANNEL *new_ch = NULL;
+    QUIC_CONNECTION *qc;
     int no_block = ((flags & SSL_ACCEPT_CONNECTION_NO_BLOCK) != 0);
 
     if (!expect_quic_listener(ssl, &ctx))
@@ -4568,6 +4576,15 @@ SSL *ossl_quic_accept_connection(SSL *ssl, uint64_t flags)
      */
     conn_ssl = ossl_quic_channel_get0_tls(new_ch);
     conn_ssl = SSL_CONNECTION_GET_USER_SSL(SSL_CONNECTION_FROM_SSL(conn_ssl));
+    qc = (QUIC_CONNECTION *)conn_ssl;
+    qc->accepted = 1;
+
+    if (!SSL_up_ref(&ctx.ql->obj.ssl)) {
+        SSL_free(conn_ssl);
+        SSL_free(ossl_quic_channel_get0_tls(new_ch));
+        conn_ssl = NULL;
+    }
+
 out:
     qctx_unlock(&ctx);
     return conn_ssl;
@@ -4577,14 +4594,8 @@ static QUIC_CONNECTION *create_qc_from_incoming_conn(QUIC_LISTENER *ql, QUIC_CHA
 {
     QUIC_CONNECTION *qc = NULL;
 
-    if (!SSL_up_ref(&ql->obj.ssl)) {
-        QUIC_RAISE_NON_NORMAL_ERROR(NULL, ERR_R_INTERNAL_ERROR, NULL);
-        goto err;
-    }
-
     if ((qc = OPENSSL_zalloc(sizeof(*qc))) == NULL) {
         QUIC_RAISE_NON_NORMAL_ERROR(NULL, ERR_R_CRYPTO_LIB, NULL);
-        SSL_free(&ql->obj.ssl);
         goto err;
     }
 
@@ -4592,11 +4603,11 @@ static QUIC_CONNECTION *create_qc_from_incoming_conn(QUIC_LISTENER *ql, QUIC_CHA
                             SSL_TYPE_QUIC_CONNECTION,
                             &ql->obj.ssl, NULL, NULL)) {
         QUIC_RAISE_NON_NORMAL_ERROR(NULL, ERR_R_INTERNAL_ERROR, NULL);
-        SSL_free(&ql->obj.ssl);
         goto err;
     }
 
     ossl_quic_channel_get_peer_addr(ch, &qc->init_peer_addr); /* best effort */
+    qc->accepted = 0;
     qc->listener                = ql;
     qc->engine                  = ql->engine;
     qc->port                    = ql->port;

diff --git a/ssl/quic/quic_local.h b/ssl/quic/quic_local.h
index dbd5c8cb4745fccb44d51f3b8c8f8a1d5c8b3b55..83b8f2b4c88a4b6cac8fb833867170b86e9021f4 100644 (file)
--- a/ssl/quic/quic_local.h
+++ b/ssl/quic/quic_local.h
@@ -205,6 +205,9 @@ struct quic_conn_st {
     unsigned int                    addressed_mode_w        : 1;
     unsigned int                    addressed_mode_r        : 1;
 
+    /* Flag to indicate if this connection has been accepted */
+    unsigned int                    accepted                : 1;
+
     /* Default stream type. Defaults to SSL_DEFAULT_STREAM_MODE_AUTO_BIDI. */
     uint32_t                        default_stream_mode;
 

diff --git a/ssl/quic/quic_port.c b/ssl/quic/quic_port.c
index 36877ab8063591e74c2380e01c04b150727d63e4..01dfbb84ef09f9c0cdc7f4c07a13310d04b168c1 100644 (file)
--- a/ssl/quic/quic_port.c
+++ b/ssl/quic/quic_port.c
@@ -462,8 +462,10 @@ static SSL *port_new_handshake_layer(QUIC_PORT *port, QUIC_CHANNEL *ch)
     }
 
     tls = ossl_ssl_connection_new_int(port->channel_ctx, user_ssl, TLS_method());
-    if (tls == NULL || (tls_conn = SSL_CONNECTION_FROM_SSL(tls)) == NULL)
+    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
@@ -476,6 +478,8 @@ static SSL *port_new_handshake_layer(QUIC_PORT *port, QUIC_CHANNEL *ch)
         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);
+            qc->tls = NULL;
             return NULL;
         }
 
@@ -535,8 +539,7 @@ static QUIC_CHANNEL *port_make_channel(QUIC_PORT *port, SSL *tls, int is_server)
      * And finally init the channel struct
      */
     if (!ossl_quic_channel_init(ch)) {
-        if (ch->tls == NULL)
-            SSL_free(ch->tls);
+        SSL_free(ch->tls);
         OPENSSL_free(ch);
         return NULL;
     }
@@ -583,6 +586,7 @@ void ossl_quic_port_drop_incoming(QUIC_PORT *port)
 {
     QUIC_CHANNEL *ch;
     SSL *tls;
+    SSL *user_ssl;
 
     for (;;) {
         ch = ossl_quic_port_pop_incoming(port);
@@ -590,8 +594,21 @@ void ossl_quic_port_drop_incoming(QUIC_PORT *port)
             break;
 
         tls = ossl_quic_channel_get0_tls(ch);
-        ossl_quic_channel_free(ch);
-        SSL_free(tls);
+        /*
+         * The user ssl may or may not have been created via the
+         * get_conn_user_ssl callback in the QUIC stack.  The
+         * differentiation being if the user_ssl pointer and tls pointer
+         * are different.  If they are, then the user_ssl needs freeing here
+         * which sends us through ossl_quic_free, which then drops the actual
+         * ch->tls ref and frees the channel
+         */
+        user_ssl = SSL_CONNECTION_GET_USER_SSL(SSL_CONNECTION_FROM_SSL(tls));
+        if (user_ssl == tls) {
+            ossl_quic_channel_free(ch);
+            SSL_free(tls);
+        } else {
+            SSL_free(user_ssl);
+        }
     }
 }