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 01fd2fca85e10983085395d0d50954e995752631..5355749a0f3d6c8e36213e61f742181949f79ccc 100644 (file)
--- 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 7689879c2460c3897f52c5223bdc94ec26b609ab..ec9c245257916f010f4dd49931832f0a8f1cf7d1 100644 (file)
--- 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 83b8f2b4c88a4b6cac8fb833867170b86e9021f4..14da9c4aa322c02b5400e8a9149b582a680b7e2a 100644 (file)
--- 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;