From 933cce6fbb1d43c00959e6eb9627454293d10a50 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 4 Nov 2024 17:28:02 +0100 Subject: [PATCH] BUG/MINOR: quic: repeat packet parsing to deal with fragmented CRYPTO A ClientHello may be splitted accross several different CRYPTO frames, then mixed in a single QUIC packet. This is used notably by clients such as chrome to render the first Initial packet opaque to middleboxes. Each packet frame is handled sequentially. Out-of-order CRYPTO frames are buffered in a ncbuf, until gaps are filled and data is transferred to the SSL stack. If CRYPTO frames are heavily splitted with small fragments, buffering may fail as ncbuf does not support small gaps. This causes the whole packet to be rejected and unacknowledged. It could be solved if the client reemits its ClientHello after remixing its CRYPTO frames. This patch is written to improve CRYPTO frame parsing. Each CRYPTO frames which cannot be buffered due to ncbuf limitation are now stored in a temporary list. Packet parsing is completed until all frames have been handled. If temporary list is not empty, reparsing is done on the stored frames. With the newly buffered CRYPTO frames, ncbuf insert operation may this time succeeds if the frame now covers a whole gap. Reparsing will loop until either no progress can be made or it has been done at least 3 times, to prevent CPU utilization. This patch should fix github issue #2776. This should be backported up to 2.6, after a period of observation. Note that it relies on the following refactor patches : MINOR: quic: extend return value of CRYPTO parsing MINOR: quic: use dynamically allocated frame on parsing MINOR: quic: simplify qc_parse_pkt_frms() return path --- include/haproxy/quic_rx-t.h | 1 + src/quic_rx.c | 78 ++++++++++++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/include/haproxy/quic_rx-t.h b/include/haproxy/quic_rx-t.h index 6639364df..2bfe6d5e8 100644 --- a/include/haproxy/quic_rx-t.h +++ b/include/haproxy/quic_rx-t.h @@ -61,6 +61,7 @@ struct quic_rx_packet { enum quic_rx_ret_frm { QUIC_RX_RET_FRM_DONE = 0, /* frame handled correctly */ QUIC_RX_RET_FRM_DUP, /* frame ignored as already handled previously */ + QUIC_RX_RET_FRM_AGAIN, /* frame cannot be handled temporarily, caller may retry during another parsing round */ QUIC_RX_RET_FRM_FATAL, /* error during frame handling, packet must not be acknowledged */ }; diff --git a/src/quic_rx.c b/src/quic_rx.c index 911ce0e87..51ab3da46 100644 --- a/src/quic_rx.c +++ b/src/quic_rx.c @@ -675,12 +675,14 @@ static enum quic_rx_ret_frm qc_handle_crypto_frm(struct quic_conn *qc, 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_ERROR("cannot bufferize frame due to gap size limit", - QUIC_EV_CONN_PRSHPKT, qc); + TRACE_DATA("cannot bufferize frame due to gap size limit", + QUIC_EV_CONN_PRSHPKT, qc); + ret = QUIC_RX_RET_FRM_AGAIN; + goto done; } - goto err; } /* Reschedule with TASK_HEAVY if CRYPTO data ready for decoding. */ @@ -773,10 +775,13 @@ static inline unsigned int quic_ack_delay_ms(struct qf_ack *ack_frm, static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, struct quic_enc_level *qel) { - struct quic_frame *frm = NULL; + struct list retry_frms = LIST_HEAD_INIT(retry_frms); + struct quic_frame *frm = NULL, *frm_tmp; const unsigned char *pos, *end; enum quic_rx_ret_frm ret; int fast_retrans = 0; + /* parsing may be rerun multiple times, but no more than . */ + int iter = 3, parsing_stage = 0; TRACE_ENTER(QUIC_EV_CONN_PRSHPKT, qc); /* Skip the AAD */ @@ -858,16 +863,32 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, switch (ret) { case QUIC_RX_RET_FRM_FATAL: goto err; + + case QUIC_RX_RET_FRM_AGAIN: + if (parsing_stage == 0) { + TRACE_STATE("parsing stage set to 1 (AGAIN encountered)", QUIC_EV_CONN_PRSHPKT, qc); + ++parsing_stage; + } + /* Save frame in temp list to reparse it later. A new instance must be used for next packet frames. */ + LIST_APPEND(&retry_frms, &frm->list); + frm = NULL; + break; + case QUIC_RX_RET_FRM_DUP: if (qc_is_listener(qc) && qel == qc->iel && !(qc->flags & QUIC_FL_CONN_HANDSHAKE_SPEED_UP)) { fast_retrans = 1; } break; + case QUIC_RX_RET_FRM_DONE: - /* nothing to do here */ + if (parsing_stage == 1) { + TRACE_STATE("parsing stage set to 2 (DONE after AGAIN)", QUIC_EV_CONN_PRSHPKT, qc); + ++parsing_stage; + } break; } + break; case QUIC_FT_NEW_TOKEN: /* TODO */ @@ -1015,6 +1036,49 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, } } + while (!LIST_ISEMPTY(&retry_frms)) { + if (--iter <= 0) { + TRACE_ERROR("interrupt parsing due to max iteration reached", + QUIC_EV_CONN_PRSHPKT, qc); + goto err; + } + else if (parsing_stage == 1) { + TRACE_ERROR("interrupt parsing due to buffering blocked on gap size limit", + QUIC_EV_CONN_PRSHPKT, qc); + goto err; + } + + parsing_stage = 0; + list_for_each_entry_safe(frm, frm_tmp, &retry_frms, list) { + /* only CRYPTO frames may be reparsed for now */ + BUG_ON(frm->type != QUIC_FT_CRYPTO); + ret = qc_handle_crypto_frm(qc, &frm->crypto, pkt, qel); + switch (ret) { + case QUIC_RX_RET_FRM_FATAL: + goto err; + + case QUIC_RX_RET_FRM_AGAIN: + if (parsing_stage == 0) { + TRACE_STATE("parsing stage set to 1 (AGAIN encountered)", QUIC_EV_CONN_PRSHPKT, qc); + ++parsing_stage; + } + break; + + case QUIC_RX_RET_FRM_DONE: + TRACE_PROTO("frame handled after a new parsing iteration", + QUIC_EV_CONN_PRSAFRM, qc, frm); + if (parsing_stage == 1) { + TRACE_STATE("parsing stage set to 2 (DONE after AGAIN)", QUIC_EV_CONN_PRSHPKT, qc); + ++parsing_stage; + } + __fallthrough; + case QUIC_RX_RET_FRM_DUP: + qc_frm_free(qc, &frm); + break; + } + } + } + if (fast_retrans && qc->iel && qc->hel) { struct quic_enc_level *iqel = qc->iel; struct quic_enc_level *hqel = qc->hel; @@ -1049,6 +1113,10 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, err: if (frm) qc_frm_free(qc, &frm); + list_for_each_entry_safe(frm, frm_tmp, &retry_frms, list) { + qc_frm_free(qc, &frm); + } + TRACE_DEVEL("leaving on error", QUIC_EV_CONN_PRSHPKT, qc); return 0; }