From: Neil Horman <nhorman@openssl.org>
Date: Tue, 21 Jan 2025 21:55:15 +0000 (-0500)
Subject: Attempt to use NULL listeners to avoid use after free
X-Git-Tag: openssl-3.5.0-alpha1~259
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=56b6ab094ee7353e86ce13793ce44d3ed5d87cc8;p=thirdparty%2Fopenssl.git

Attempt to use NULL listeners to avoid use after free

As per @sashan suggestion, try pre-creating user ssls with a NULL
listener

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/doc/man3/SSL_new_listener.pod b/doc/man3/SSL_new_listener.pod
index 01fd2fca85e..5355749a0f3 100644
--- a/doc/man3/SSL_new_listener.pod
+++ b/doc/man3/SSL_new_listener.pod
@@ -109,7 +109,9 @@ by a listener object, that listener object is returned. An SSL object which is
 not a listener object and which is not descended from a listener object (e.g. a
 connection obtained using SSL_accept_connection()) or indirectly from a listener
 object (e.g. a QUIC stream SSL object obtained using SSL_accept_stream() called
-on a connection obtained using SSL_accept_connection()) returns NULL.
+on a connection obtained using SSL_accept_connection()) returns NULL.  Also note
+that pending SSL connections on a QUIC listeners accept queue have some caveats,
+see NOTES below.
 
 The SSL_listen() function begins listening after a listener has been created.
 Appropriate BIOs must have been configured before calling SSL_listen(), along
@@ -212,6 +214,17 @@ called on an unsupported SSL object type.
 SSL_new_from_listener() returns a pointer to a new SSL object on success or NULL
 on failure. On success, the caller assumes ownership of the reference.
 
+=head1 NOTES
+
+SSL_get0_listener() behaves somewhat differently in SSL callbacks for QUIC
+connections.  As QUIC connections begin TLS handshake operations prior to them
+being accepted via SSL_accept_connection(), an application may receive callbacks
+for such pending connection prior to acceptance via SSL_accept_connection().  As
+listner association takes place during the accept process, prior to being
+returned from SSL_accept_connection(), calls to SSL_get0_listener() made from
+such SSL callbacks will return NULL.  This can be used as an indicator within
+the callback that the referenced SSL object has not yet been accepted.
+
 =head1 SEE ALSO
 
 L<OSSL_QUIC_server_method(3)>, L<SSL_free(3)>, L<SSL_set_bio(3)>,
diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index 7689879c246..ec9c2452579 100644
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -660,7 +660,7 @@ static void qc_cleanup(QUIC_CONNECTION *qc, int have_lock)
     ossl_quic_channel_free(qc->ch);
     qc->ch = NULL;
 
-    if (qc->port != NULL && qc->listener == NULL) { /* TODO */
+    if (qc->port != NULL && qc->listener == NULL && qc->pending == 0) { /* TODO */
         quic_unref_port_bios(qc->port);
         ossl_quic_port_free(qc->port);
         qc->port = NULL;
@@ -674,7 +674,7 @@ static void qc_cleanup(QUIC_CONNECTION *qc, int have_lock)
         /* tsan doesn't like freeing locked mutexes */
         ossl_crypto_mutex_unlock(qc->mutex);
 
-    if (qc->listener == NULL)
+    if (qc->listener == NULL && qc->pending == 0)
         ossl_crypto_mutex_free(&qc->mutex);
 #endif
 }
@@ -4578,7 +4578,8 @@ SSL *ossl_quic_accept_connection(SSL *ssl, uint64_t flags)
     conn_ssl = SSL_CONNECTION_GET_USER_SSL(SSL_CONNECTION_FROM_SSL(conn_ssl));
     qc = (QUIC_CONNECTION *)conn_ssl;
     qc->accepted = 1;
-
+    qc->listener = ctx.ql;
+    qc->pending = 0;
     if (!SSL_up_ref(&ctx.ql->obj.ssl)) {
         SSL_free(conn_ssl);
         SSL_free(ossl_quic_channel_get0_tls(new_ch));
@@ -4608,7 +4609,8 @@ static QUIC_CONNECTION *create_qc_from_incoming_conn(QUIC_LISTENER *ql, QUIC_CHA
 
     ossl_quic_channel_get_peer_addr(ch, &qc->init_peer_addr); /* best effort */
     qc->accepted = 0;
-    qc->listener                = ql;
+    qc->listener                = NULL;
+    qc->pending                 = 1;
     qc->engine                  = ql->engine;
     qc->port                    = ql->port;
     qc->ch                      = ch;
diff --git a/ssl/quic/quic_local.h b/ssl/quic/quic_local.h
index 83b8f2b4c88..14da9c4aa32 100644
--- a/ssl/quic/quic_local.h
+++ b/ssl/quic/quic_local.h
@@ -208,6 +208,9 @@ struct quic_conn_st {
     /* Flag to indicate if this connection has been accepted */
     unsigned int                    accepted                : 1;
 
+    /* Flag to indicate waiting on accept queue */
+    unsigned int                    pending                 : 1;
+
     /* Default stream type. Defaults to SSL_DEFAULT_STREAM_MODE_AUTO_BIDI. */
     uint32_t                        default_stream_mode;