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

Author: a-denoyelle

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 */
 };
 

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 <iter>. */
+	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;
 }