From dfda884633c202d7c2df281b19b7a12ed738aee2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= Date: Fri, 10 Nov 2023 16:57:32 +0100 Subject: [PATCH] BUG/MINOR: quic: Useless use of non-contiguous buffer for in order CRYPTO data This issue could be reproduced with a TLS client certificate verificatio to generate enough CRYPTO data between the client and haproxy and with dev/udp/udp-perturb as network perturbator. Haproxy could crash thanks to a BUG_ON() call as soon as in disorder data were bufferized into a non-contiguous buffer. There is no need to pass a non NULL non-contiguous to qc_ssl_provide_quic_data() from qc_ssl_provide_all_quic_data() which handles in order CRYPTO data which have not been bufferized. If not, the first call to qc_ssl_provide_quic_data() to process the first block of in order data leads the non-contiguous buffer head to be advanced to a wrong offset, by bytes which is the length of the in order CRYPTO frame. This is detected by a BUG_ON() as follows: FATAL: bug condition "ncb_ret != NCB_RET_OK" matched at src/quic_ssl.c:620 call trace(11): | 0x5631cc41f3cc [0f 0b 8b 05 d4 df 48 00]: qc_ssl_provide_quic_data+0xca7/0xd78 | 0x5631cc41f6b2 [89 45 bc 48 8b 45 b0 48]: qc_ssl_provide_all_quic_data+0x215/0x576 | 0x5631cc3ce862 [48 8b 45 b0 8b 40 04 25]: quic_conn_io_cb+0x19a/0x8c2 | 0x5631cc67f092 [e9 1b 02 00 00 83 45 e4]: run_tasks_from_lists+0x498/0x741 | 0x5631cc67fb51 [89 c2 8b 45 e0 29 d0 89]: process_runnable_tasks+0x816/0x879 | 0x5631cc625305 [8b 05 bd 0c 2d 00 83 f8]: run_poll_loop+0x8b/0x4bc | 0x5631cc6259c0 [48 8b 05 b9 ac 29 00 48]: main-0x2c6 | 0x7fa6c34a2ea7 [64 48 89 04 25 30 06 00]: libpthread:+0x7ea7 | 0x7fa6c33c2a2f [48 89 c7 b8 3c 00 00 00]: libc:clone+0x3f/0x5a Thank you to @Tristan971 for having reported this issue in GH #2095. No need to backport. --- src/quic_ssl.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/quic_ssl.c b/src/quic_ssl.c index 88943e8a29223..814b1b1c51105 100644 --- a/src/quic_ssl.c +++ b/src/quic_ssl.c @@ -634,24 +634,16 @@ 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 = NCBUF_NULL; TRACE_ENTER(QUIC_EV_CONN_PHPKTS, qc); list_for_each_entry(qel, &qc->qel_list, list) { int ssl_ret; - struct quic_cstream *cstream = qel->cstream; - struct ncbuf *ncbuf; struct qf_crypto *qf_crypto, *qf_back; - if (!qel->cstream) { - TRACE_DEVEL("no cstream", QUIC_EV_CONN_PHPKTS, qc, qel); - continue; - } - ssl_ret = 1; - ncbuf = &cstream->rx.ncbuf; list_for_each_entry_safe(qf_crypto, qf_back, &qel->rx.crypto_frms, list) { - - ssl_ret = qc_ssl_provide_quic_data(ncbuf, qel->level, ctx, + ssl_ret = qc_ssl_provide_quic_data(&ncbuf, qel->level, ctx, qf_crypto->data, qf_crypto->len); /* Free this frame asap */ LIST_DELETE(&qf_crypto->list);