From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Tue, 16 Aug 2022 09:29:08 +0000 (+0200)
Subject: BUG/MEDIUM: mux-quic: reject uni stream ID exceeding flow control
X-Git-Tag: v2.7-dev4~28
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=bf3c208760861ee36590fc4a5a579b8808818bd9;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: mux-quic: reject uni stream ID exceeding flow control

Emit STREAM_LIMIT_ERROR if a client tries to open an unidirectional
stream with an ID greater than the value specified by our flow-control
limit. The code is similar to the bidirectional stream opening.

MAX_STREAMS_UNI emission is not implement for the moment and is left as
a TODO. This should not be too urgent for the moment : in HTTP/3, a
client has only a limited use for unidirectional streams (H3 control
stream + 2 QPACK streams). This is covered by the value provided by
haproxy in transport parameters.

This patch has been tagged with BUG as it should have prevented last
crash reported on github issue #1808 when opening a new unidirectional
streams with an invalid ID. However, it is probably not the main cause
of the bug contrary to the patch
  commit 11a6f4007b908b49ecd3abd5cd10fba177f07c11
  BUG/MINOR: quic: Wrong status returned by qc_pkt_decrypt()

This must be backported up to 2.6.
---

diff --git a/include/haproxy/mux_quic-t.h b/include/haproxy/mux_quic-t.h
index c555ace3c1..aa29c00b75 100644
--- a/include/haproxy/mux_quic-t.h
+++ b/include/haproxy/mux_quic-t.h
@@ -52,10 +52,13 @@ struct qcc {
 	/* flow-control fields set by us enforced on our side. */
 	struct {
 		struct list frms; /* prepared frames related to flow-control  */
+
 		uint64_t ms_bidi_init; /* max initial sub-ID of bidi stream allowed for the peer */
 		uint64_t ms_bidi; /* max sub-ID of bidi stream allowed for the peer */
 		uint64_t cl_bidi_r; /* total count of closed remote bidi stream since last MAX_STREAMS emission */
 
+		uint64_t ms_uni; /* max sub-ID of uni stream allowed for the peer */
+
 		uint64_t msd_bidi_l; /* initial max-stream-data on local streams */
 		uint64_t msd_bidi_r; /* initial max-stream-data on remote streams */
 
diff --git a/src/mux_quic.c b/src/mux_quic.c
index 57d7fdb2e3..7b71ddb2c1 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -582,7 +582,7 @@ static struct qcs *qcc_init_stream_remote(struct qcc *qcc, uint64_t id)
 {
 	struct qcs *qcs = NULL;
 	enum qcs_type type;
-	uint64_t *largest;
+	uint64_t *largest, max_id;
 
 	TRACE_ENTER(QMUX_EV_QCS_NEW, qcc->conn);
 
@@ -597,20 +597,18 @@ static struct qcs *qcc_init_stream_remote(struct qcc *qcc, uint64_t id)
 		type = conn_is_back(qcc->conn) ? QCS_SRV_UNI : QCS_CLT_UNI;
 	}
 
-	/* TODO also checks max-streams for uni streams */
-	if (quic_stream_is_bidi(id)) {
-		if (id >= qcc->lfctl.ms_bidi * 4) {
-			/* RFC 9000 4.6. Controlling Concurrency
-			 *
-			 * An endpoint that receives a frame with a
-			 * stream ID exceeding the limit it has sent
-			 * MUST treat this as a connection error of
-			 * type STREAM_LIMIT_ERROR
-			 */
-			TRACE_ERROR("flow control error", QMUX_EV_QCS_NEW|QMUX_EV_PROTO_ERR, qcc->conn);
-			qcc_emit_cc(qcc, QC_ERR_STREAM_LIMIT_ERROR);
-			goto err;
-		}
+	/* RFC 9000 4.6. Controlling Concurrency
+	 *
+	 * An endpoint that receives a frame with a stream ID exceeding the
+	 * limit it has sent MUST treat this as a connection error of type
+	 * STREAM_LIMIT_ERROR
+	 */
+	max_id = quic_stream_is_bidi(id) ? qcc->lfctl.ms_bidi * 4 :
+	                                   qcc->lfctl.ms_uni * 4;
+	if (id >= max_id) {
+		TRACE_ERROR("flow control error", QMUX_EV_QCS_NEW|QMUX_EV_PROTO_ERR, qcc->conn);
+		qcc_emit_cc(qcc, QC_ERR_STREAM_LIMIT_ERROR);
+		goto err;
 	}
 
 	/* Only stream ID not already opened can be used. */
@@ -1204,7 +1202,12 @@ static int qcc_release_remote_stream(struct qcc *qcc, uint64_t id)
 		}
 	}
 	else {
-		/* TODO */
+		/* TODO in HTTP/3 unidirectional streams cannot be closed or a
+		 * H3_CLOSED_CRITICAL_STREAM will be triggered before
+		 * entering here. If a new application protocol is supported it
+		 * might be necessary to implement MAX_STREAMS_UNI emission.
+		 */
+		ABORT_NOW();
 	}
 
 	TRACE_LEAVE(QMUX_EV_QCS_END, qcc->conn);
@@ -1980,6 +1983,7 @@ static int qc_init(struct connection *conn, struct proxy *prx,
 
 	LIST_INIT(&qcc->lfctl.frms);
 	qcc->lfctl.ms_bidi = qcc->lfctl.ms_bidi_init = lparams->initial_max_streams_bidi;
+	qcc->lfctl.ms_uni = lparams->initial_max_streams_uni;
 	qcc->lfctl.msd_bidi_l = lparams->initial_max_stream_data_bidi_local;
 	qcc->lfctl.msd_bidi_r = lparams->initial_max_stream_data_bidi_remote;
 	qcc->lfctl.cl_bidi_r = 0;