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 1a316b282a0d6206739019e522391e7c01802b36..208a3c26d70bc2524c344329de7a45115f5f3ee3 100644 (file)
--- 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 37116ca1a843a0677ddddd89801697c6b3f1a9a1..0f10604f1c2a4429e4e046f3e6d43b4fff0f95bf 100644 (file)
--- 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 182b62696d2044e94799d9f17e9e88e01515bd1f..8c085e278360cda29a40e71b4daa9c8b13991d1a 100644 (file)
--- 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++)