From: Neil Horman Date: Tue, 14 Jan 2025 22:52:20 +0000 (-0500) Subject: Fix memory leak in pre-allocated listeners X-Git-Tag: openssl-3.5.0-alpha1~261 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6f38c59850105f5bf6f6780c73da6f0333f87582;p=thirdparty%2Fopenssl.git 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 Reviewed-by: Tomas Mraz Reviewed-by: Saša Nedvědický (Merged from https://github.com/openssl/openssl/pull/26361) --- diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index 735481634bb..7689879c246 100644 --- 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 dbd5c8cb474..83b8f2b4c88 100644 --- 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 36877ab8063..01dfbb84ef0 100644 --- 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); + } } }