From: Frédéric Lécaille Date: Wed, 8 Mar 2023 10:01:58 +0000 (+0100) Subject: BUG/MINOR: quic: Wrong RETIRE_CONNECTION_ID sequence number check X-Git-Tag: v2.8-dev5~13 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cc101cd2aa44af25f4fb0b895f7ef530b993f45a;p=thirdparty%2Fhaproxy.git 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 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 d85dc6fd39..e2e6d0b143 100644 --- a/include/haproxy/quic_conn-t.h +++ b/include/haproxy/quic_conn-t.h @@ -660,7 +660,6 @@ struct quic_conn { 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 66da15829d..6d671911f7 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -2941,30 +2941,6 @@ static int qc_handle_crypto_frm(struct quic_conn *qc, return ret; } - -/* Remove the connection ID from with 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 connection and build a NEW_CONNECTION_ID * frame to be sent. * Return 1 if succeeded, 0 if not. @@ -2995,32 +2971,60 @@ static int qc_build_new_connection_id_frm(struct quic_conn *qc, /* Handle RETIRE_CONNECTION_ID frame from frame. - * Return 1 if succeeded, 0 if not. + * Return 1 if succeeded, 0 if not. If succeeded, also set + * 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 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 quic-conn from its ha_thread_ctx list. If is true, @@ -3199,14 +3203,19 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, 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 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, goto err; } - /* The sequence number of the current CID is the same as for . */ - 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);