BUG/MAJOR: quic: don't reference a retired CID from a NEW_CONNECTION_ID frame

A NEW_CONNECTION_ID frame built by HAProxy only stores pointers to the
CID and stateless reset token of its quic_connection_id. When a
RETIRE_CONNECTION_ID frame is received, the CID is freed at once, while
a NEW_CONNECTION_ID frame for it may still be queued, in flight, or be
duplicated later for a retransmission. A client can trigger it by
retiring sequence numbers 1 and 2 in the same packet: the second frame
frees the CID allocated by the first one, whose frame is not sent yet.
The frame is then built from freed memory. In a default build, the pool
cache gives the object back to the next CID of the same connection, so
two frames carry the same CID. Without the pool cache, freed heap memory
is sent to the peer, and ASan reports a heap-use-after-free.

struct qf_new_connection_id now stores the CID and the token instead of
pointers, and quic_connection_id_to_frm_cpy() and the frame duplication
copy them. The parser copies them as well and rejects a CID length above
20 (RFC 9000 section 19.15). quic_frame does not grow, since its union
already holds a larger member.

This should be backported to 2.8 and later.

diff --git a/include/haproxy/quic_cid.h b/include/haproxy/quic_cid.h
index 05f3e5b80d014d0dc70c0b9f403456648cc5d5e8..3aa7c481b0e36d527d5bf26a347ee592fd632e0f 100644 (file)
--- a/include/haproxy/quic_cid.h
+++ b/include/haproxy/quic_cid.h
@@ -111,8 +111,9 @@ static inline void quic_connection_id_to_frm_cpy(struct quic_frame *dst,
        ncid_frm->seq_num = src->seq_num.key;
        ncid_frm->retire_prior_to = src->retire_prior_to;
        ncid_frm->cid.len = src->cid.len;
-       ncid_frm->cid.data = src->cid.data;
-       ncid_frm->stateless_reset_token = src->stateless_reset_token;
+       memcpy(ncid_frm->cid.data, src->cid.data, src->cid.len);
+       memcpy(ncid_frm->stateless_reset_token, src->stateless_reset_token,
+              QUIC_STATELESS_RESET_TOKEN_LEN);
 }
 
 #endif /* USE_QUIC */

diff --git a/include/haproxy/quic_frame-t.h b/include/haproxy/quic_frame-t.h
index c94aff0f5cba08a087895c0a2413276603a92c04..7db31c9c2b19e4ec921465533e2c889f88a30283 100644 (file)
--- a/include/haproxy/quic_frame-t.h
+++ b/include/haproxy/quic_frame-t.h
@@ -32,6 +32,8 @@
 #include <import/ebtree-t.h>
 #include <haproxy/buf-t.h>
 #include <haproxy/list.h>
+#include <haproxy/quic_cid-t.h>
+#include <haproxy/quic_tp-t.h>
 #include <haproxy/quic_token.h>
 
 extern struct pool_head *pool_head_quic_frame;
@@ -217,14 +219,18 @@ struct qf_streams_blocked {
        uint64_t limit;
 };
 
+/* The CID and the token are stored in the frame, because a frame which is
+ * waiting to be sent or retransmitted must not depend on the lifetime of the
+ * quic_connection_id it was built from: the peer may retire it at any time.
+ */
 struct qf_new_connection_id {
        uint64_t seq_num;
        uint64_t retire_prior_to;
        struct {
                unsigned char len;
-               const unsigned char *data;
+               unsigned char data[QUIC_CID_MAXLEN];
        } cid;
-       const unsigned char *stateless_reset_token;
+       unsigned char stateless_reset_token[QUIC_STATELESS_RESET_TOKEN_LEN];
 };
 
 struct qf_retire_connection_id {

diff --git a/src/quic_frame.c b/src/quic_frame.c
index 1e813ee7fe02cc65f2e41ba57a7748a998611b32..0233869bd69d5bd1a0bfe1de5ee6e6ad32c1a582 100644 (file)
--- a/src/quic_frame.c
+++ b/src/quic_frame.c
@@ -807,14 +807,19 @@ static int quic_parse_new_connection_id_frame(struct quic_frame *frm, struct qui
                return 0;
 
        ncid_frm->cid.len = *(*pos)++;
-       if (end - *pos < ncid_frm->cid.len + QUIC_STATELESS_RESET_TOKEN_LEN)
+       /* RFC 9000 19.15. NEW_CONNECTION_ID Frames:
+        * Values less than 1 and greater than 20 are invalid and MUST be
+        * treated as a connection error of type FRAME_ENCODING_ERROR.
+        */
+       if (ncid_frm->cid.len > QUIC_CID_MAXLEN ||
+           end - *pos < ncid_frm->cid.len + QUIC_STATELESS_RESET_TOKEN_LEN)
                return 0;
 
        if (ncid_frm->cid.len) {
-               ncid_frm->cid.data = *pos;
+               memcpy(ncid_frm->cid.data, *pos, ncid_frm->cid.len);
                *pos += ncid_frm->cid.len;
        }
-       ncid_frm->stateless_reset_token = *pos;
+       memcpy(ncid_frm->stateless_reset_token, *pos, QUIC_STATELESS_RESET_TOKEN_LEN);
        *pos += QUIC_STATELESS_RESET_TOKEN_LEN;
 
        return 1;