Commit 0ed06337e3 for openssl.org
commit 0ed06337e38ec70e5beb043d5a1da9a6b6e8c57e
Author: Neil Horman <nhorman@openssl.org>
Date: Sun Mar 8 16:49:09 2026 -0400
Fix use after free in quic_connection freeing if up ref fails
Issue https://github.com/openssl/openssl/issues/3030
Found a use after free case in ossl_quic_accept_connection in the event
that we fail to up_ref the associated quic listener object.
If we fail to take the up ref on the listener object in this function,
we free the SSL object, which calls into
SSL_free->ossl_quic_free->qc_cleanup, which because we have an
associated listener, we free the mutex for, and then get a use-afer-free
when we try to unlock that mutex shortly thereafter.
We really need to fix 3 problems here:
1) The use after free. Handle this bt ensuring that the listener is
assigned first.
2) A deadlock, since we already hold the associated mutex, we need to
defer the free operation until after we unlock the mutex.
3) Don't drop the refcount on the listener object in ossl_quic_cleanup
(since we failed to up-ref it here). Handle this by adding a flag to
indicate up-ref failure in the quic-connection object.
Problem was confirmed by synthetically failing the up ref in local
testing, and this patch was confirmed to fix the issue.
Also, we need to adjust some of the tests in quicapitest here, as
several tests just assume that SSL_accept_connection will return a
non-null value.
Fixes #30307
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
MergeDate: Wed Mar 11 09:22:35 2026
(Merged from https://github.com/openssl/openssl/pull/30311)
diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index fd4e0ced3f..ea76430009 100644
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -4736,9 +4736,10 @@ SSL *ossl_quic_accept_connection(SSL *ssl, uint64_t flags)
int ret;
QCTX ctx;
SSL *conn_ssl = NULL;
+ SSL *conn_ssl_tmp = NULL;
SSL_CONNECTION *conn = NULL;
QUIC_CHANNEL *new_ch = NULL;
- QUIC_CONNECTION *qc;
+ QUIC_CONNECTION *qc = NULL;
int no_block = ((flags & SSL_ACCEPT_CONNECTION_NO_BLOCK) != 0);
if (!expect_quic_listener(ssl, &ctx))
@@ -4793,28 +4794,38 @@ SSL *ossl_quic_accept_connection(SSL *ssl, uint64_t flags)
* bound to new_ch. If channel constructor fails to create any item here
* it just fails to create channel.
*/
- if (!ossl_assert((conn_ssl = ossl_quic_channel_get0_tls(new_ch)) != NULL)
- || !ossl_assert((conn = SSL_CONNECTION_FROM_SSL(conn_ssl)) != NULL)
- || !ossl_assert((conn_ssl = SSL_CONNECTION_GET_USER_SSL(conn)) != NULL))
+ if (!ossl_assert((conn_ssl_tmp = ossl_quic_channel_get0_tls(new_ch)) != NULL)
+ || !ossl_assert((conn = SSL_CONNECTION_FROM_SSL(conn_ssl_tmp)) != NULL)
+ || !ossl_assert((conn_ssl_tmp = SSL_CONNECTION_GET_USER_SSL(conn)) != NULL))
goto out;
- qc = (QUIC_CONNECTION *)conn_ssl;
- qc->pending = 0;
- if (!SSL_up_ref(&ctx.ql->obj.ssl)) {
- /*
- * You might expect ossl_quic_channel_free() to be called here. Be
- * assured it happens, The process goes as follows:
- * - The SSL_free() here is being handled by ossl_quic_free().
- * - The very last step of ossl_quic_free() is call to qc_cleanup()
- * where channel gets freed.
- */
- SSL_free(conn_ssl);
+ qc = (QUIC_CONNECTION *)conn_ssl_tmp;
+ if (SSL_up_ref(&ctx.ql->obj.ssl)) {
+ qc->listener = ctx.ql;
+ conn_ssl = conn_ssl_tmp;
+ conn_ssl_tmp = NULL;
+ qc->pending = 0;
}
- qc->listener = ctx.ql;
out:
qctx_unlock(&ctx);
+ /*
+ * You might expect ossl_quic_channel_free() to be called here. Be
+ * assured it happens, The process goes as follows:
+ * - The SSL_free() here is being handled by ossl_quic_free().
+ * - The very last step of ossl_quic_free() is call to qc_cleanup()
+ * where channel gets freed.
+ * NOTE: We defer this SSL_free until after the call to qctx_unlock above
+ * to avoid the deadlock that would occur when ossl_quic_free attempts to
+ * re-acquire this mutex. We also do the gymnastics with conn_ssl and
+ * conn_ssl_tmp above so that we only actually do the free on the SSL
+ * object if the up-ref above fails, in such a way that we don't unbalance
+ * the listener refcount (i.e. if the up-ref fails above, we don't set the
+ * listener pointer so that we don't then drop the ref-count erroneously
+ * during the free operation.
+ */
+ SSL_free(conn_ssl_tmp);
return conn_ssl;
}
diff --git a/test/quicapitest.c b/test/quicapitest.c
index c322aed510..ad2c8ca2a9 100644
--- a/test/quicapitest.c
+++ b/test/quicapitest.c
@@ -2894,8 +2894,7 @@ static int test_ssl_listen_ex(void)
goto err;
/* Call SSL_accept() and SSL_connect() until we are connected */
- if (!TEST_true(create_bare_ssl_connection(serverssl, clientssl,
- SSL_ERROR_NONE, 0, 0)))
+ if (!TEST_true(create_bare_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE, 0, 0)))
/*
* Ensure that, now that we have used SSL_listen_ex, SSL_accept_connection
@@ -3012,8 +3011,8 @@ static int test_ssl_set_verify(void)
serverssl = SSL_accept_connection(qlistener, 0);
/* Call SSL_accept() and SSL_connect() until we are connected */
- if (!TEST_true(create_bare_ssl_connection(serverssl, clientssl,
- SSL_ERROR_NONE, 0, 0)))
+ if (!TEST_ptr(serverssl)
+ || !TEST_true(create_bare_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE, 0, 0)))
goto err;
testresult = 1;
@@ -3225,8 +3224,8 @@ static int test_client_hello_retry(void)
serverssl = SSL_accept_connection(qlistener, 0);
/* Call SSL_accept() and SSL_connect() until we are connected */
- if (!TEST_true(create_bare_ssl_connection(serverssl, clientssl,
- SSL_ERROR_NONE, 0, 0)))
+ if (!TEST_ptr(serverssl)
+ || !TEST_true(create_bare_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE, 0, 0)))
goto err;
testresult = 1;