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 d85dc6fd39665fa9c1e26436a8a8a84d6676958b..e2e6d0b143a0796c4959d8f8156c8134ad8d5e78 100644 (file)
--- 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 66da15829dd6a2c1b1173a9b26e8c155dac6e259..6d671911f7833c366b4af672f4f3c9856b2e513e 100644 (file)
--- 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 <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 @@ static int qc_build_new_connection_id_frm(struct quic_conn *qc,
 
 
 /* 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 @@ 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 <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);