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 <leaf_p> 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 <new_tid> 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 2ee368f956e0d50ab17bf90f9fc4b8f5c9497051..db6d4e00f443a8e59b44bd8f93bfac4642231948 100644 (file)
--- 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 <conn_id> 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 <conn_id> 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 6f83bfced3a8bc9580fbd353289d91c5c7d03af0..403b3ffe2aff8ff34efa37c076be692a193b39e8 100644 (file)
--- 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 <conn_id> in global CID tree. It may fail if an identical value is
- * already stored. In this case, <new_tid> will be filled with the thread ID of
- * the already stored CID.
+/* Insert <conn_id> in global CID tree. It may fail if a collision happens due
+ * to an identical CID already stored.
+ *
+ * If <new_tid> 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 81d57ea81d8f40cd9732f1353916d798fcb78d54..3273c332daae364b53dd0c40aa9bd1ad75e4e6e6 100644 (file)
--- 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 6b8f61ad50423b2961b6469c4babd586ec208c47..c9ae4a5d4647a1eaa7e1d2ec35f4d185415cfaf5 100644 (file)
--- 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);