From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Wed, 11 Oct 2023 07:28:36 +0000 (+0200)
Subject: BUG/MINOR: quic: Avoid crashing with unsupported cryptographic algos
X-Git-Tag: v2.9-dev8~83
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=bd83b6effb8a15c24509c462cc19e029d6db5893;p=thirdparty%2Fhaproxy.git

BUG/MINOR: quic: Avoid crashing with unsupported cryptographic algos

This bug was detected when compiling haproxy against aws-lc TLS stack
during QUIC interop runner tests. Some algorithms could be negotiated by haproxy
through the TLS stack but not fully supported by haproxy QUIC implentation.
This leaded tls_aead() to return NULL (same thing for tls_md(), tls_hp()).
As these functions returned values were never checked, they could triggered
segfaults.

To fix this, one closes the connection as soon as possible with a
handshake_failure(40) TLS alert. Note that as the TLS stack successfully
negotiates an algorithm, it provides haproxy with CRYPTO data before entering
->set_encryption_secrets() callback. This is why this callback
(ha_set_encryption_secrets() on haproxy side) is modified to release all
the CRYPTO frames before triggering a CONNECTION_CLOSE with a TLS alert. This is
done calling qc_release_pktns_frms() for all the packet number spaces.
Modify some quic_tls_keys_hexdump to avoid crashes when the ->aead or ->hp EVP_CIPHER
are NULL.
Modify qc_release_pktns_frms() to do nothing if the packet number space passed
as parameter is not intialized.

This bug does not impact the QUIC TLS compatibily mode (USE_QUIC_OPENSSL_COMPAT).

Thank you to @ilia-shipitsin for having reported this issue in GH #2309.

Must be backported as far as 2.6.
---

diff --git a/include/haproxy/quic_tls.h b/include/haproxy/quic_tls.h
index 1a316b282a..208a3c26d7 100644
--- a/include/haproxy/quic_tls.h
+++ b/include/haproxy/quic_tls.h
@@ -552,9 +552,13 @@ static inline void qc_release_pktns_frms(struct quic_conn *qc,
 
 	TRACE_ENTER(QUIC_EV_CONN_PHPKTS, qc);
 
+	if (!pktns)
+		goto leave;
+
 	list_for_each_entry_safe(frm, frmbak, &pktns->tx.frms, list)
 		qc_frm_free(qc, &frm);
 
+ leave:
 	TRACE_LEAVE(QUIC_EV_CONN_PHPKTS, qc);
 }
 
diff --git a/src/quic_ssl.c b/src/quic_ssl.c
index 37116ca1a8..0f10604f1c 100644
--- a/src/quic_ssl.c
+++ b/src/quic_ssl.c
@@ -192,15 +192,17 @@ static int ha_quic_set_encryption_secrets(SSL *ssl, enum ssl_encryption_level_t
 		goto write;
 
 	rx = &tls_ctx->rx;
+	rx->aead = tls_aead(cipher);
+	rx->md   = tls_md(cipher);
+	rx->hp   = tls_hp(cipher);
+	if (!rx->aead || !rx->md || !rx->hp)
+		goto leave;
+
 	if (!quic_tls_secrets_keys_alloc(rx)) {
 		TRACE_ERROR("RX keys allocation failed", QUIC_EV_CONN_RWSEC, qc);
 		goto leave;
 	}
 
-	rx->aead = tls_aead(cipher);
-	rx->md   = tls_md(cipher);
-	rx->hp   = tls_hp(cipher);
-
 	if (!quic_tls_derive_keys(rx->aead, rx->hp, rx->md, ver, rx->key, rx->keylen,
 	                          rx->iv, rx->ivlen, rx->hp_key, sizeof rx->hp_key,
 	                          read_secret, secret_len)) {
@@ -233,15 +235,17 @@ write:
 		goto keyupdate_init;
 
 	tx = &tls_ctx->tx;
+	tx->aead = tls_aead(cipher);
+	tx->md   = tls_md(cipher);
+	tx->hp   = tls_hp(cipher);
+	if (!tx->aead || !tx->md || !tx->hp)
+		goto leave;
+
 	if (!quic_tls_secrets_keys_alloc(tx)) {
 		TRACE_ERROR("TX keys allocation failed", QUIC_EV_CONN_RWSEC, qc);
 		goto leave;
 	}
 
-	tx->aead = tls_aead(cipher);
-	tx->md   = tls_md(cipher);
-	tx->hp   = tls_hp(cipher);
-
 	if (!quic_tls_derive_keys(tx->aead, tx->hp, tx->md, ver, tx->key, tx->keylen,
 	                          tx->iv, tx->ivlen, tx->hp_key, sizeof tx->hp_key,
 	                          write_secret, secret_len)) {
@@ -298,6 +302,16 @@ write:
  out:
 	ret = 1;
  leave:
+	if (!ret) {
+		/* Release the CRYPTO frames which have been provided by the TLS stack
+		 * to prevent the transmission of ack-eliciting packets.
+		 */
+		qc_release_pktns_frms(qc, qc->ipktns);
+		qc_release_pktns_frms(qc, qc->hpktns);
+		qc_release_pktns_frms(qc, qc->apktns);
+		quic_set_tls_alert(qc, SSL_AD_HANDSHAKE_FAILURE);
+	}
+
 	TRACE_LEAVE(QUIC_EV_CONN_RWSEC, qc, &level);
 	return ret;
 }
diff --git a/src/quic_tls.c b/src/quic_tls.c
index 182b62696d..8c085e2783 100644
--- a/src/quic_tls.c
+++ b/src/quic_tls.c
@@ -47,9 +47,16 @@ void quic_tls_keys_hexdump(struct buffer *buf,
                            const struct quic_tls_secrets *secs)
 {
 	int i;
-	size_t aead_keylen = (size_t)EVP_CIPHER_key_length(secs->aead);
-	size_t aead_ivlen = (size_t)EVP_CIPHER_iv_length(secs->aead);
-	size_t hp_len = (size_t)EVP_CIPHER_key_length(secs->hp);
+	size_t aead_keylen;
+	size_t aead_ivlen;
+	size_t hp_len;
+
+	if (!secs->aead || !secs->hp)
+		return;
+
+	aead_keylen = (size_t)EVP_CIPHER_key_length(secs->aead);
+	aead_ivlen = (size_t)EVP_CIPHER_iv_length(secs->aead);
+	hp_len = (size_t)EVP_CIPHER_key_length(secs->hp);
 
 	chunk_appendf(buf, "\n          key=");
 	for (i = 0; i < aead_keylen; i++)