BUG/MAJOR: quic: use ncbmbuf for CRYPTO handling

In QUIC, TLS handshake messages such as ClientHello are encapsulated in
CRYPTO frames. Each QUIC implementation can split the content in several
frames of random sizes. In fact, this feature is now used by several
clients, based on chrome so-called "Chaos protection" mechanism :

https://quiche.googlesource.com/quiche/+/cb6b51054274cb2c939264faf34a1776e0a5bab7

To support this, haproxy uses a ncbuf storage to store received CRYPTO
frames before passing it to the SSL library. However, this storage
suffers from a limitation as gaps between two filled blocks cannot be
smaller than 8 bytes. Thus, depending on the size of received CRYPTO
frames and their order, ncbuf may not be sufficient. Over time, several
mechanisms were implemented in haproxy QUIC frames parsing to overcome
the ncbuf limitation.

However, reports recently highlight that with some clients haproxy is
not able to deal with CRYPTO frames reception. In particular, this is
the case with the latest ngtcp2 release, which implements a similar
chaos protection mechanism via the following patch. It also seems that
this impacts haproxy interaction with firefox.

commit 89c29fd8611d5e6d2f6b1f475c5e3494c376028c
Author: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date:   Mon Aug 4 22:48:06 2025 +0900

    Crumble Client Initial CRYPTO (aka chaos protection)

To fix haproxy CRYPTO frames buffering once and for all, an alternative
non-contiguous buffer named ncbmbuf has been recently implemented. This
type does not suffer from gaps size limitation, albeit at the cost of a
small reduction in the size available for data storage.

Thus, the purpose of this current patch is to replace ncbuf with the
newer ncbmbuf for QUIC CRYPTO frames parsing. Now, ncbmb_add() is used
to buffer received frames which is guaranteed to suceed. The only
remaining case of error is if a received frame offset and length exceed
the ncbmbuf data storage, which would result in a CRYPTO_BUFFER_EXCEEDED
error code.

A notable behavior change when switching to ncbmbuf implementation is
that NCB_ADD_COMPARE mode cannot be used anymore during add. Instead,
crypto frame content received at a similar offset will be overwritten.

A final note regarding STREAM frames parsing. For now, it is considered
unnecessary to switch from ncbuf in this case. Indeed, QUIC clients does
not perform aggressive fragmentation for them. Keeping ncbuf ensure that
the data storage size is bigger than the equivalent ncbmbuf area.

This should fix github issue #3141.

This patch must be backported up to 2.6. It is first necessary to pick
the relevant commits for ncbmbuf implementation prior to it.

diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h
index 78517753de43998f94b96ee1b25beb4221626f05..db32934ef57050b23af668de2ed2153ebe12d611 100644 (file)
--- a/include/haproxy/quic_conn.h
+++ b/include/haproxy/quic_conn.h
@@ -32,7 +32,7 @@
 
 #include <haproxy/chunk.h>
 #include <haproxy/dynbuf.h>
-#include <haproxy/ncbuf.h>
+#include <haproxy/ncbmbuf.h>
 #include <haproxy/net_helper.h>
 #include <haproxy/openssl-compat.h>
 #include <haproxy/ticks.h>
@@ -135,35 +135,35 @@ static inline void quic_conn_mv_cids_to_cc_conn(struct quic_conn_closed *cc_conn
  *
  * Returns the buffer instance or NULL on allocation failure.
  */
-static inline struct ncbuf *quic_get_ncbuf(struct ncbuf *ncbuf)
+static inline struct ncbmbuf *quic_get_ncbuf(struct ncbmbuf *ncbuf)
 {
        struct buffer buf = BUF_NULL;
 
-       if (!ncb_is_null(ncbuf))
+       if (!ncbmb_is_null(ncbuf))
                return ncbuf;
 
        if (!b_alloc(&buf, DB_MUX_RX))
                return NULL;
 
-       *ncbuf = ncb_make(buf.area, buf.size, 0);
-       ncb_init(ncbuf, 0);
+       *ncbuf = ncbmb_make(buf.area, buf.size, 0);
+       ncbmb_init(ncbuf, 0);
 
        return ncbuf;
 }
 
 /* Release the underlying memory use by <ncbuf> non-contiguous buffer */
-static inline void quic_free_ncbuf(struct ncbuf *ncbuf)
+static inline void quic_free_ncbuf(struct ncbmbuf *ncbuf)
 {
        struct buffer buf;
 
-       if (ncb_is_null(ncbuf))
+       if (ncbmb_is_null(ncbuf))
                return;
 
        buf = b_make(ncbuf->area, ncbuf->size, 0, 0);
        b_free(&buf);
        offer_buffers(NULL, 1);
 
-       *ncbuf = NCBUF_NULL;
+       *ncbuf = NCBMBUF_NULL;
 }
 
 /* Return the address of the QUIC counters attached to the proxy of

diff --git a/include/haproxy/quic_tls-t.h b/include/haproxy/quic_tls-t.h
index 1a39919b6d8f15c0cbe3be0604d0a9becf4ec677..15ae8edebfaf705c6937270444e0d74897a9dbfc 100644 (file)
--- a/include/haproxy/quic_tls-t.h
+++ b/include/haproxy/quic_tls-t.h
@@ -26,7 +26,7 @@
 #include <import/ebtree.h>
 
 #include <haproxy/buf-t.h>
-#include <haproxy/ncbuf-t.h>
+#include <haproxy/ncbmbuf-t.h>
 #include <haproxy/quic_ack-t.h>
 
 /* Use EVP_CIPHER or EVP_AEAD API depending on the library */
@@ -255,7 +255,7 @@ struct quic_crypto_buf {
 struct quic_cstream {
        struct {
                uint64_t offset;       /* absolute current base offset of ncbuf */
-               struct ncbuf ncbuf;    /* receive buffer - can handle out-of-order offset frames */
+               struct ncbmbuf ncbuf;  /* receive buffer - can handle out-of-order offset frames */
        } rx;
        struct {
                uint64_t offset;      /* last offset of data ready to be sent */

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 07a0481e7403dd84be55cdfb5663e6df4d23a911..5cb13cc0c0251677ecdda11a5ea4f124166f29b6 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -16,7 +16,7 @@
 
 #include <haproxy/h3.h>
 #include <haproxy/list.h>
-#include <haproxy/ncbuf.h>
+#include <haproxy/ncbmbuf.h>
 #include <haproxy/proto_quic.h>
 #include <haproxy/quic_ack.h>
 #include <haproxy/quic_cc_drs.h>
@@ -666,7 +666,7 @@ static enum quic_rx_ret_frm qc_handle_crypto_frm(struct quic_conn *qc,
                .len = crypto_frm->len,
        };
        struct quic_cstream *cstream = qel->cstream;
-       struct ncbuf *ncbuf = &qel->cstream->rx.ncbuf;
+       struct ncbmbuf *ncbuf = &qel->cstream->rx.ncbuf;
        uint64_t off_rel;
 
        TRACE_ENTER(QUIC_EV_CONN_PRSHPKT, qc);
@@ -691,7 +691,7 @@ static enum quic_rx_ret_frm qc_handle_crypto_frm(struct quic_conn *qc,
                crypto_frm->offset_node.key = cstream->rx.offset;
        }
 
-       if (!quic_get_ncbuf(ncbuf) || ncb_is_null(ncbuf)) {
+       if (!quic_get_ncbuf(ncbuf) || ncbmb_is_null(ncbuf)) {
                TRACE_ERROR("CRYPTO ncbuf allocation failed", QUIC_EV_CONN_PRSHPKT, qc);
                goto err;
        }
@@ -707,31 +707,18 @@ static enum quic_rx_ret_frm qc_handle_crypto_frm(struct quic_conn *qc,
         * handshake. If an endpoint does not expand its buffer, it MUST close
         * the connection with a CRYPTO_BUFFER_EXCEEDED error code.
         */
-       if (off_rel + crypto_frm->len > ncb_size(ncbuf)) {
+       if (off_rel + crypto_frm->len > ncbmb_size(ncbuf)) {
                TRACE_ERROR("CRYPTO frame too large", QUIC_EV_CONN_PRSHPKT, qc);
                quic_set_connection_close(qc, quic_err_transport(QC_ERR_CRYPTO_BUFFER_EXCEEDED));
                goto err;
        }
 
-       ncb_ret = ncb_add(ncbuf, off_rel, (const char *)crypto_frm->data,
-                         crypto_frm->len, NCB_ADD_COMPARE);
-       if (ncb_ret != NCB_RET_OK) {
-               if (ncb_ret == NCB_RET_DATA_REJ) {
-                       TRACE_ERROR("overlapping data rejected", QUIC_EV_CONN_PRSHPKT, qc);
-                       quic_set_connection_close(qc, quic_err_transport(QC_ERR_PROTOCOL_VIOLATION));
-                       qc_notify_err(qc);
-                       goto err;
-               }
-               else if (ncb_ret == NCB_RET_GAP_SIZE) {
-                       TRACE_DATA("cannot bufferize frame due to gap size limit",
-                                  QUIC_EV_CONN_PRSHPKT, qc);
-                       ret = QUIC_RX_RET_FRM_AGAIN;
-                       goto done;
-               }
-       }
+       ncb_ret = ncbmb_add(ncbuf, off_rel, (const char *)crypto_frm->data,
+                           crypto_frm->len, NCB_ADD_OVERWRT);
+       BUG_ON(ncb_ret != NCB_RET_OK);
 
        /* Reschedule with TASK_HEAVY if CRYPTO data ready for decoding. */
-       if (ncb_data(ncbuf, 0)) {
+       if (ncbmb_data(ncbuf, 0)) {
                HA_ATOMIC_OR(&qc->wait_event.tasklet->state, TASK_HEAVY);
                tasklet_wakeup(qc->wait_event.tasklet);
        }

diff --git a/src/quic_ssl.c b/src/quic_ssl.c
index 6e315ca489b95cdef051bd00e414fc8373779d49..4ef22843789255c89e329b540d2727cfadc2ccda 100644 (file)
--- a/src/quic_ssl.c
+++ b/src/quic_ssl.c
@@ -1,5 +1,5 @@
 #include <haproxy/errors.h>
-#include <haproxy/ncbuf.h>
+#include <haproxy/ncbmbuf.h>
 #include <haproxy/proxy.h>
 #include <haproxy/quic_conn.h>
 #include <haproxy/quic_sock.h>
@@ -439,7 +439,7 @@ static int ha_quic_ossl_crypto_recv_rcd(SSL *ssl,
 {
        struct quic_conn *qc = SSL_get_ex_data(ssl, ssl_qc_app_data_index);
        struct quic_enc_level *qel;
-       struct ncbuf *ncbuf = NULL;
+       struct ncbmbuf *ncbuf = NULL;
        struct quic_cstream *cstream = NULL;
        ncb_sz_t data = 0;
 
@@ -451,10 +451,10 @@ static int ha_quic_ossl_crypto_recv_rcd(SSL *ssl,
                        continue;
 
                ncbuf = &cstream->rx.ncbuf;
-               if (ncb_is_null(ncbuf))
+               if (ncbmb_is_null(ncbuf))
                        continue;
 
-               data = ncb_data(ncbuf, 0);
+               data = ncbmb_data(ncbuf, 0);
                if (data)
                        break;
        }
@@ -462,9 +462,9 @@ static int ha_quic_ossl_crypto_recv_rcd(SSL *ssl,
        if (data) {
                const unsigned char *cdata;
 
-               BUG_ON(ncb_is_null(ncbuf) || !cstream);
+               BUG_ON(ncbmb_is_null(ncbuf) || !cstream);
                /* <ncbuf> must not be released at this time. */
-               cdata = (const unsigned char *)ncb_head(ncbuf);
+               cdata = (const unsigned char *)ncbmb_head(ncbuf);
                cstream->rx.offset += data;
                TRACE_DEVEL("buffered crypto data were provided to TLS stack",
                                        QUIC_EV_CONN_PHPKTS, qc, qel);
@@ -496,24 +496,24 @@ static int ha_quic_ossl_crypto_release_rcd(SSL *ssl,
 
        list_for_each_entry(qel, &qc->qel_list, list) {
                struct quic_cstream *cstream = qel->cstream;
-               struct ncbuf *ncbuf;
+               struct ncbmbuf *ncbuf;
                ncb_sz_t data;
 
                if (!cstream)
                        continue;
 
                ncbuf = &cstream->rx.ncbuf;
-               if (ncb_is_null(ncbuf))
+               if (ncbmb_is_null(ncbuf))
                        continue;
 
-               data = ncb_data(ncbuf, 0);
+               data = ncbmb_data(ncbuf, 0);
                if (!data)
                        continue;
 
                data = data > bytes_read ? bytes_read : data;
-               ncb_advance(ncbuf, data);
+               ncbmb_advance(ncbuf, data);
                bytes_read -= data;
-               if (ncb_is_empty(ncbuf)) {
+               if (ncbmb_is_empty(ncbuf)) {
                        TRACE_DEVEL("freeing crypto buf", QUIC_EV_CONN_PHPKTS, qc, qel);
                        quic_free_ncbuf(ncbuf);
                }
@@ -1069,7 +1069,7 @@ int qc_ssl_do_hanshake(struct quic_conn *qc, struct ssl_sock_ctx *ctx)
  * Remaining parameter are there for debugging purposes.
  * Return 1 if succeeded, 0 if not.
  */
-static int qc_ssl_provide_quic_data(struct ncbuf *ncbuf,
+static int qc_ssl_provide_quic_data(struct ncbmbuf *ncbuf,
                                     enum ssl_encryption_level_t level,
                                     struct ssl_sock_ctx *ctx,
                                     const unsigned char *data, size_t len)
@@ -1099,17 +1099,12 @@ static int qc_ssl_provide_quic_data(struct ncbuf *ncbuf,
        /* The CRYPTO data are consumed even in case of an error to release
         * the memory asap.
         */
-       if (!ncb_is_null(ncbuf)) {
+       if (!ncbmb_is_null(ncbuf)) {
 #ifdef DEBUG_STRICT
-               ncb_ret = ncb_advance(ncbuf, len);
-               /* ncb_advance() must always succeed. This is guaranteed as
-                * this is only done inside a data block. If false, this will
-                * lead to handshake failure with quic_enc_level offset shifted
-                * from buffer data.
-                */
+               ncb_ret = ncbmb_advance(ncbuf, len);
                BUG_ON(ncb_ret != NCB_RET_OK);
 #else
-               ncb_advance(ncbuf, len);
+               ncbmb_advance(ncbuf, len);
 #endif
        }
 
@@ -1124,7 +1119,7 @@ int qc_ssl_provide_all_quic_data(struct quic_conn *qc, struct ssl_sock_ctx *ctx)
 {
        int ret = 0;
        struct quic_enc_level *qel;
-       struct ncbuf *ncbuf;
+       struct ncbmbuf *ncbuf;
        ncb_sz_t data;
 
        TRACE_ENTER(QUIC_EV_CONN_PHPKTS, qc);
@@ -1135,12 +1130,12 @@ int qc_ssl_provide_all_quic_data(struct quic_conn *qc, struct ssl_sock_ctx *ctx)
                        continue;
 
                ncbuf = &cstream->rx.ncbuf;
-               if (ncb_is_null(ncbuf))
+               if (ncbmb_is_null(ncbuf))
                        continue;
 
                /* TODO not working if buffer is wrapping */
-               while ((data = ncb_data(ncbuf, 0))) {
-                       const unsigned char *cdata = (const unsigned char *)ncb_head(ncbuf);
+               while ((data = ncbmb_data(ncbuf, 0))) {
+                       const unsigned char *cdata = (const unsigned char *)ncbmb_head(ncbuf);
 
                        if (!qc_ssl_provide_quic_data(&qel->cstream->rx.ncbuf, qel->level,
                                                      ctx, cdata, data))
@@ -1151,7 +1146,7 @@ int qc_ssl_provide_all_quic_data(struct quic_conn *qc, struct ssl_sock_ctx *ctx)
                                    QUIC_EV_CONN_PHPKTS, qc, qel);
                }
 
-               if (!ncb_is_null(ncbuf) && ncb_is_empty(ncbuf)) {
+               if (!ncbmb_is_null(ncbuf) && ncbmb_is_empty(ncbuf)) {
                        TRACE_DEVEL("freeing crypto buf", QUIC_EV_CONN_PHPKTS, qc, qel);
                        quic_free_ncbuf(ncbuf);
                }

diff --git a/src/quic_tls.c b/src/quic_tls.c
index 40213a1bde89a65a608b7b6505f865719bb3a933..55d36e662428f36ad8327f416b196315d2454d2e 100644 (file)
--- a/src/quic_tls.c
+++ b/src/quic_tls.c
@@ -143,7 +143,7 @@ struct quic_cstream *quic_cstream_new(struct quic_conn *qc)
        }
 
        cs->rx.offset = 0;
-       cs->rx.ncbuf = NCBUF_NULL;
+       cs->rx.ncbuf = NCBMBUF_NULL;
        cs->rx.offset = 0;
 
        cs->tx.offset = 0;