From: Amaury Denoyelle Date: Thu, 6 Nov 2025 15:33:31 +0000 (+0100) Subject: BUG/MEDIUM: quic: handle collision on CID generation X-Git-Tag: v3.3-dev13~54 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2623e0a0b7a1426386bcbf1e4419ded9d673cf27;p=thirdparty%2Fhaproxy.git BUG/MEDIUM: quic: handle collision on CID generation CIDs are provided by haproxy so that the peer can use them as DCID of its packets. Their value is set via a random generator. It happens on several occasions during connection lifetime: * via ODCID derivation if haproxy is the server * on quic_conn init if haproxy is the client * during post-handshake if haproxy is the server * on RETIRE_CONNECTION_ID frame parsing CIDs are stored in a global tree. On ODCID derivation, a check is performed to ensure the CID is not a duplicate value. This is mandatory to properly handle multiple INITIAL packets from the same client on different thread. However, for the other cases, no check is performed for CID collision. As _quic_cid_insert() is silent, the issue is not detected at all. This results in a CID advertized to the peer but not stored in the global one. In the end, this may cause two issues. The first one is that packets from the client which use the new CID will be rejected by haproxy, most probably with a STATELESS_RESET. The second issue is that it can cause a crash during quic_conn release. Indeed, the CID is stored in the quic_conn local tree and thus eb_delete() for the global tree will be performed. As member is uninit, this results in a segfault. Note that this issue is pretty rare. It can only be observed if running with a high number of concurrent connections in parallel, so that the random generator will provide duplicate values. Patch is still labelled as MEDIUM as this modifies code paths used frequently. To fix this, _quic_cid_insert() unsafe function is completely removed. Instead, quic_cid_insert() can be used, which reports an error code if a collision happens. CID are then stored in the quic_conn tree only after global tree insert success. Here is the solution for each steps if a collision occurs : * on init as client: the connection is completely released * post-handshake: the CID is immediately released. The connection is kept, but it will miss an extra CID. * on RETIRE_CONNECTION_ID parsing: a loop is implemented to retry random generation. It it fails several times, the connection is closed in error. A small convenience change is made to quic_cid_insert(). Output parameter can now be NULL, which is useful as most of the times caller do not care about it. This must be backported up to 2.6. --- diff --git a/include/haproxy/quic_cid.h b/include/haproxy/quic_cid.h index 2ee368f95..db6d4e00f 100644 --- a/include/haproxy/quic_cid.h +++ b/include/haproxy/quic_cid.h @@ -80,20 +80,6 @@ static inline uchar quic_cid_tree_idx(const struct quic_cid *cid) return _quic_cid_tree_idx(cid->data); } -/* Insert into global CID tree. Do not check if value is already - * present in the tree. As such, it should not be used for the first DCID of a - * connection instance. - */ -static inline void _quic_cid_insert(struct quic_connection_id *conn_id) -{ - const uchar idx = quic_cid_tree_idx(&conn_id->cid); - struct quic_cid_tree *tree = &quic_cid_trees[idx]; - - HA_RWLOCK_WRLOCK(QC_CID_LOCK, &tree->lock); - ebmb_insert(&tree->root, &conn_id->node, conn_id->cid.len); - HA_RWLOCK_WRUNLOCK(QC_CID_LOCK, &tree->lock); -} - /* Remove from global CID tree as a thread-safe operation. */ static inline void quic_cid_delete(struct quic_connection_id *conn_id) { diff --git a/src/quic_cid.c b/src/quic_cid.c index 6f83bfced..403b3ffe2 100644 --- a/src/quic_cid.c +++ b/src/quic_cid.c @@ -238,9 +238,12 @@ void quic_cid_register_seq_num(struct quic_connection_id *conn_id) TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); } -/* Insert in global CID tree. It may fail if an identical value is - * already stored. In this case, will be filled with the thread ID of - * the already stored CID. +/* Insert in global CID tree. It may fail if a collision happens due + * to an identical CID already stored. + * + * If is non null, it will be used as an output parameter, + * initialized to -1 by default. In case of a CID collision, it will be set to + * the thread ID of the already stored CID. * * Returns 0 on insert success else non-zero. */ @@ -250,15 +253,17 @@ int quic_cid_insert(struct quic_connection_id *conn_id, int *new_tid) struct quic_cid_tree *tree; int ret; - *new_tid = -1; + if (new_tid) + *new_tid = -1; tree = &quic_cid_trees[quic_cid_tree_idx(&conn_id->cid)]; HA_RWLOCK_WRLOCK(QC_CID_LOCK, &tree->lock); node = ebmb_insert(&tree->root, &conn_id->node, conn_id->cid.len); if (node != &conn_id->node) { - /* Node already inserted, may happen on thread contention. */ - conn_id = ebmb_entry(node, struct quic_connection_id, node); - *new_tid = HA_ATOMIC_LOAD(&conn_id->tid); + if (new_tid) { + conn_id = ebmb_entry(node, struct quic_connection_id, node); + *new_tid = HA_ATOMIC_LOAD(&conn_id->tid); + } ret = -1; } else { diff --git a/src/quic_conn.c b/src/quic_conn.c index 81d57ea81..3273c332d 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -539,10 +539,12 @@ int quic_build_post_handshake_frames(struct quic_conn *qc) goto err; } - /* TODO To prevent CID tree locking, all CIDs created here - * could be allocated at the same time as the first one. - */ - _quic_cid_insert(conn_id); + if (quic_cid_insert(conn_id, NULL)) { + TRACE_STATE("collision during CID generation on post-handshake", QUIC_EV_CONN_IO_CB, qc); + pool_free(pool_head_quic_connection_id, conn_id); + qc_frm_free(qc, &frm); + continue; + } quic_cid_register_seq_num(conn_id); quic_connection_id_to_frm_cpy(frm, conn_id); @@ -1245,7 +1247,11 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, goto err; } - _quic_cid_insert(conn_cid); + if (quic_cid_insert(conn_cid, NULL)) { + pool_free(pool_head_quic_connection_id, conn_cid); + goto err; + } + quic_cid_register_seq_num(conn_cid); dcid = &qc->dcid; conn_id = conn_cid; diff --git a/src/quic_rx.c b/src/quic_rx.c index 6b8f61ad5..c9ae4a5d4 100644 --- a/src/quic_rx.c +++ b/src/quic_rx.c @@ -1005,6 +1005,7 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, case QUIC_FT_RETIRE_CONNECTION_ID: { struct quic_connection_id *conn_id = NULL; + int retry_rand_cid = 3; /* Number of random retries on CID collision. */ if (!qc_handle_retire_connection_id_frm(qc, frm, &pkt->dcid, &conn_id)) goto err; @@ -1025,14 +1026,30 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt, goto err; } - if (quic_cid_generate(conn_id)) { - TRACE_ERROR("error on CID generation", QUIC_EV_CONN_PSTRM, qc); + while (retry_rand_cid--) { + if (quic_cid_generate(conn_id)) { + TRACE_ERROR("error on CID generation", QUIC_EV_CONN_PSTRM, qc); + quic_set_connection_close(qc, quic_err_transport(QC_ERR_INTERNAL_ERROR)); + pool_free(pool_head_quic_connection_id, conn_id); + qc_notify_err(qc); + goto err; + } + + if (quic_cid_insert(conn_id, NULL) == 0) + break; + } + + if (retry_rand_cid < 0) { + /* Multiple collisions during CID random gen. + * Prefer to close the connection in error + * immediately. + */ + TRACE_ERROR("CID pool exhausted", QUIC_EV_CONN_IO_CB, qc); quic_set_connection_close(qc, quic_err_transport(QC_ERR_INTERNAL_ERROR)); pool_free(pool_head_quic_connection_id, conn_id); qc_notify_err(qc); goto err; } - _quic_cid_insert(conn_id); quic_cid_register_seq_num(conn_id);