BUG/MINOR: quic: Wrong RETIRE_CONNECTION_ID sequence number check
This bug arrived with this commit:
b5a8020e9 MINOR: quic: RETIRE_CONNECTION_ID frame handling (RX)
and was revealed by h3 interop tests with clients like s2n-quic and quic-go
as noticed by Amaury.
Indeed, one must check that the CID matching the sequence number provided by a received
RETIRE_CONNECTION_ID frame does not match the DCID of the packet.
Remove useless ->curr_cid_seq_num member from quic_conn struct.
The sequence number lookup must be done in qc_handle_retire_connection_id_frm()
to check the validity of the RETIRE_CONNECTION_ID frame, it returns the CID to be
retired into <cid_to_retire> variable passed as parameter to this function if
the frame is valid and if the CID was not already retired
Must be backported to 2.7.
diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h
index d85dc6f..e2e6d0b 100644
--- a/include/haproxy/quic_conn-t.h
+++ b/include/haproxy/quic_conn-t.h
@@ -660,7 +660,6 @@
struct quic_cid scid; /* first SCID of our endpoint - not updated when a new SCID is used */
struct eb_root cids; /* tree of quic_connection_id - used to match a received packet DCID with a connection */
uint64_t next_cid_seq_num;
- uint64_t curr_cid_seq_num;
struct quic_enc_level els[QUIC_TLS_ENC_LEVEL_MAX];
struct quic_pktns pktns[QUIC_TLS_PKTNS_MAX];
diff --git a/src/quic_conn.c b/src/quic_conn.c
index 66da158..6d67191 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -2941,30 +2941,6 @@
return ret;
}
-
-/* Remove the connection ID from with <seq_num> as sequence number.
- * Return 1 if a connection ID was effectively removed, 0 if not.
- */
-static int qc_retire_connection_seq_num(struct quic_conn *qc, uint64_t seq_num)
-{
- struct eb64_node *node;
- struct quic_connection_id *cid;
-
- TRACE_ENTER(QUIC_EV_CONN_PRSHPKT, qc);
- node = eb64_lookup(&qc->cids, seq_num);
- if (!node)
- return 0;
-
- cid = eb64_entry(node, struct quic_connection_id, seq_num);
- ebmb_delete(&cid->node);
- eb64_delete(&cid->seq_num);
- pool_free(pool_head_quic_connection_id, cid);
- TRACE_PROTO("CID retired", QUIC_EV_CONN_PSTRM, qc);
-
- TRACE_LEAVE(QUIC_EV_CONN_PRSHPKT, qc);
- return 1;
-}
-
/* Allocate a new connection ID for <qc> connection and build a NEW_CONNECTION_ID
* frame to be sent.
* Return 1 if succeeded, 0 if not.
@@ -2995,32 +2971,60 @@
/* Handle RETIRE_CONNECTION_ID frame from <frm> frame.
- * Return 1 if succeeded, 0 if not.
+ * Return 1 if succeeded, 0 if not. If succeeded, also set <cid_to_retire>
+ * to the CID to be retired if not already retired.
*/
static int qc_handle_retire_connection_id_frm(struct quic_conn *qc,
- struct quic_frame *frm)
+ struct quic_frame *frm,
+ struct quic_cid *dcid,
+ struct quic_connection_id **cid_to_retire)
{
int ret = 0;
struct quic_retire_connection_id *rcid = &frm->retire_connection_id;
+ struct eb64_node *node;
+ struct quic_connection_id *cid;
TRACE_ENTER(QUIC_EV_CONN_PRSHPKT, qc);
+ /* RFC 9000 19.16. RETIRE_CONNECTION_ID Frames:
+ * Receipt of a RETIRE_CONNECTION_ID frame containing a sequence number greater
+ * than any previously sent to the peer MUST be treated as a connection error
+ * of type PROTOCOL_VIOLATION.
+ */
if (rcid->seq_num >= qc->next_cid_seq_num) {
TRACE_PROTO("CID seq. number too big", QUIC_EV_CONN_PSTRM, qc, frm);
- quic_set_connection_close(qc, quic_err_transport(QC_ERR_PROTOCOL_VIOLATION));
- goto leave;
+ goto protocol_violation;
+ }
+
+ /* RFC 9000 19.16. RETIRE_CONNECTION_ID Frames:
+ * The sequence number specified in a RETIRE_CONNECTION_ID frame MUST NOT refer to
+ * the Destination Connection ID field of the packet in which the frame is contained.
+ * The peer MAY treat this as a connection error of type PROTOCOL_VIOLATION.
+ */
+ node = eb64_lookup(&qc->cids, rcid->seq_num);
+ if (!node) {
+ TRACE_PROTO("CID already retired", QUIC_EV_CONN_PSTRM, qc, frm);
+ goto out;
}
- if (rcid->seq_num == qc->curr_cid_seq_num) {
+ cid = eb64_entry(node, struct quic_connection_id, seq_num);
+ /* Note that the length of <dcid> has already been checked. It must match the
+ * length of the CIDs which have been provided to the peer.
+ */
+ if (!memcmp(dcid->data, cid->cid.data, QUIC_HAP_CID_LEN)) {
TRACE_PROTO("cannot retire the current CID", QUIC_EV_CONN_PSTRM, qc, frm);
- quic_set_connection_close(qc, quic_err_transport(QC_ERR_PROTOCOL_VIOLATION));
- goto leave;
+ goto protocol_violation;
}
+ *cid_to_retire = cid;
+ out:
ret = 1;
leave:
TRACE_LEAVE(QUIC_EV_CONN_PRSHPKT, qc);
return ret;
+ protocol_violation:
+ quic_set_connection_close(qc, quic_err_transport(QC_ERR_PROTOCOL_VIOLATION));
+ goto leave;
}
/* Remove a <qc> quic-conn from its ha_thread_ctx list. If <closing> is true,
@@ -3199,14 +3203,19 @@
break;
case QUIC_FT_RETIRE_CONNECTION_ID:
{
- struct quic_connection_id *cid;
+ struct quic_connection_id *cid = NULL;
- if (!qc_handle_retire_connection_id_frm(qc, &frm))
+ if (!qc_handle_retire_connection_id_frm(qc, &frm, &pkt->dcid, &cid))
goto leave;
- if (!qc_retire_connection_seq_num(qc, frm.retire_connection_id.seq_num))
+ if (!cid)
break;
+ ebmb_delete(&cid->node);
+ eb64_delete(&cid->seq_num);
+ pool_free(pool_head_quic_connection_id, cid);
+ TRACE_PROTO("CID retired", QUIC_EV_CONN_PSTRM, qc);
+
cid = new_quic_cid(&qc->cids, qc);
if (!cid) {
TRACE_ERROR("CID allocation error", QUIC_EV_CONN_IO_CB, qc);
@@ -5346,8 +5355,6 @@
goto err;
}
- /* The sequence number of the current CID is the same as for <icid>. */
- qc->curr_cid_seq_num = icid->seq_num.key;
if ((global.tune.options & GTUNE_QUIC_SOCK_PER_CONN) &&
is_addr(local_addr)) {
TRACE_USER("Allocate a socket for QUIC connection", QUIC_EV_CONN_INIT, qc);