From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Tue, 28 Sep 2021 07:51:23 +0000 (+0200)
Subject: MINOR: quic: Fix SSL error issues (do not use ssl_bio_and_sess_init())
X-Git-Tag: v2.5-dev9~41
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=75dd2b7987f0c9bede8a73e61e89bf784a2ea593;p=thirdparty%2Fhaproxy.git

MINOR: quic: Fix SSL error issues (do not use ssl_bio_and_sess_init())

It seems it was a bad idea to use the same function as for TCP ssl sockets
to initialize the SSL session objects for QUIC with ssl_bio_and_sess_init().
Indeed, this had as very bad side effects to generate SSL errors due
to the fact that such BIOs initialized for QUIC could not finally be controlled
via the BIO_ctrl*() API, especially BIO_ctrl() function used by very much other
internal OpenSSL functions (BIO_push(), BIO_pop() etc).
Others OpenSSL base QUIC implementation do not use at all BIOs to configure
QUIC connections. So, we decided to proceed the same way as ngtcp2 for instance:
only initialize an SSL object and call SSL_set_quic_method() to set its
underlying method. Note that calling this function silently disable this option:
SSL_OP_ENABLE_MIDDLEBOX_COMPAT.
We implement qc_ssl_sess_init() to initialize SSL sessions for QUIC connections
to do so with a retry in case of allocation failure as this is done by
ssl_bio_and_sess_init(). We also modify the code part for haproxy servers.
---

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 16d66f35bd..8501e3fafe 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -4446,6 +4446,51 @@ static int quic_conn_unsubscribe(struct connection *conn, void *xprt_ctx, int ev
 	return conn_unsubscribe(conn, xprt_ctx, event_type, es);
 }
 
+/* Try to allocate the <*ssl> SSL session object for <qc> QUIC connection
+ * with <ssl_ctx> as SSL context inherited settings. Also set the transport
+ * parameters of this session.
+ * This is the responsibility of the caller to check the validity of all the
+ * pointers passed as parameter to this function.
+ * Return 0 if succeeded, -1 if not. If failed, sets the ->err_code member of <qc->conn> to
+ * CO_ER_SSL_NO_MEM.
+ */
+static int qc_ssl_sess_init(struct quic_conn *qc, SSL_CTX *ssl_ctx, SSL **ssl,
+                            unsigned char *params, size_t params_len)
+{
+	int retry;
+
+	retry = 1;
+ retry:
+	*ssl = SSL_new(ssl_ctx);
+	if (!*ssl) {
+		if (!retry--)
+			goto err;
+
+		pool_gc(NULL);
+		goto retry;
+	}
+
+	if (!SSL_set_quic_method(*ssl, &ha_quic_method) ||
+	    !SSL_set_ex_data(*ssl, ssl_app_data_index, qc->conn) ||
+	    !SSL_set_quic_transport_params(*ssl, qc->enc_params, qc->enc_params_len)) {
+			goto err;
+
+		SSL_free(*ssl);
+		*ssl = NULL;
+		if (!retry--)
+			goto err;
+
+		pool_gc(NULL);
+		goto retry;
+	}
+
+	return 0;
+
+ err:
+	qc->conn->err_code = CO_ER_SSL_NO_MEM;
+	return -1;
+}
+
 /* Initialize a QUIC connection (quic_conn struct) to be attached to <conn>
  * connection with <xprt_ctx> as address of the xprt context.
  * Returns 1 if succeeded, 0 if not.
@@ -4505,10 +4550,6 @@ static int qc_conn_init(struct connection *conn, void **xprt_ctx)
 		                  dcid, sizeof dcid, 0))
 			goto err;
 
-		if (ssl_bio_and_sess_init(conn, srv->ssl_ctx.ctx,
-		                          &ctx->ssl, &ctx->bio, ha_quic_meth, ctx) == -1) 
-			goto err;
-
 		qc->rx.params = srv->quic_params;
 		/* Copy the initial source connection ID. */
 		quic_cid_cpy(&qc->rx.params.initial_source_connection_id, &qc->scid);
@@ -4518,6 +4559,10 @@ static int qc_conn_init(struct connection *conn, void **xprt_ctx)
 		if (!qc->enc_params_len)
 			goto err;
 
+		if (qc_ssl_sess_init(qc, srv->ssl_ctx.ctx, &ctx->ssl,
+		                     qc->enc_params, qc->enc_params_len) == -1)
+			goto err;
+
 		SSL_set_quic_transport_params(ctx->ssl, qc->enc_params, qc->enc_params_len);
 		SSL_set_connect_state(ctx->ssl);
 		ssl_err = SSL_do_handshake(ctx->ssl);
@@ -4541,11 +4586,10 @@ static int qc_conn_init(struct connection *conn, void **xprt_ctx)
 		struct quic_conn *qc = ctx->conn->qc;
 
 		ctx->wait_event.tasklet->tid = quic_get_cid_tid(&qc->scid);
-		if (ssl_bio_and_sess_init(conn, bc->initial_ctx,
-		                          &ctx->ssl, &ctx->bio, ha_quic_meth, ctx) == -1)
+		if (qc_ssl_sess_init(qc, bc->initial_ctx, &ctx->ssl,
+		                     qc->enc_params, qc->enc_params_len) == -1)
 			goto err;
 
-		SSL_set_quic_transport_params(ctx->ssl, qc->enc_params, qc->enc_params_len);
 		SSL_set_accept_state(ctx->ssl);
 	}