quinn-proto — Unbounded growth of pending.retire_cids via already-retired NEW_CONNECTION_ID frames (remote memory-exhaustion DoS)

Component: quinn-proto (the sans-IO QUIC state machine used by quinn)
Affected: current main at commit fec2f8960df489767bfccef5b08b72013f1b992a (and all prior releases carrying the same handler — the code is long-standing). Verified present at this pin.
Class: CWE-770 Allocation of Resources Without Limits or Throttling (memory-exhaustion DoS)
Severity (self-assessed): Medium. Remote, application-unauthenticated, post-handshake. No favorable amplification and self-limiting (see Impact), but it defeats a deliberate, documented anti-DoS bound and accumulates on long-lived connections and across many connections.
Reporter tooling: found and verified with rust-in-peace (https://github.com/scadastrangelove/rust-in-peace), a Rust-security fork of Anthropic's defending-code review pipeline.

Summary

When handling a peer's NEW_CONNECTION_ID frame, quinn-proto queues a RETIRE_CONNECTION_ID in pending.retire_cids. There are two code paths that push into this queue, and only one of them is bounded:

• The normal "retire a range" path (Ok(Some(..))) enforces MAX_PENDING_RETIRED_CIDS = CidQueue::LEN * 10 = 50 and closes the connection with CONNECTION_ID_LIMIT_ERROR if exceeded.
• The "this CID was already retired" path (Err(InsertError::Retired)) pushes frame.sequence with no cap and no de-duplication.

A peer that has completed the QUIC handshake can drive the second path arbitrarily many times, growing pending.retire_cids without bound. Because the queue drains only via congestion-controlled outbound frames, an attacker who withholds ACKs stalls the drain while continuing to flood, so the queue grows monotonically → the victim's memory is exhausted.

This is the exact scenario the code's own comment claims is impossible.

The defective code (at pin fec2f89)

1. The two push paths — quinn-proto/src/connection/mod.rs:2956

match self.rem_cids.insert(frame) {
    Ok(None) => {}
    Ok(Some((retired, reset_token))) => {
        let pending_retired = &mut self.spaces[SpaceId::Data].pending.retire_cids;
        /// Ensure `pending_retired` cannot grow without bound. Limit is
        /// somewhat arbitrary but very permissive.
        const MAX_PENDING_RETIRED_CIDS: u64 = CidQueue::LEN as u64 * 10;   // = 50
        if (pending_retired.len() as u64)
            .saturating_add(retired.end.saturating_sub(retired.start))
            > MAX_PENDING_RETIRED_CIDS
        {
            return Err(TransportError::CONNECTION_ID_LIMIT_ERROR(
                "queued too many retired CIDs",
            ));                                    // <-- BOUNDED path
        }
        pending_retired.extend(retired);
        self.set_reset_token(reset_token);
    }
    Err(InsertError::ExceedsLimit) => {
        return Err(TransportError::CONNECTION_ID_LIMIT_ERROR(""));
    }
    Err(InsertError::Retired) => {
        trace!("discarding already-retired");
        // RETIRE_CONNECTION_ID might not have been previously sent if e.g. a
        // range of connection IDs larger than the active connection ID limit
        // was retired all at once via retire_prior_to.
        self.spaces[SpaceId::Data]
            .pending
            .retire_cids
            .push(frame.sequence);             // <-- UNBOUNDED path (no cap, no dedup)
        continue;
    }
};

The asymmetry is the bug: Ok(Some) checks MAX_PENDING_RETIRED_CIDS; Err(Retired) does not.

2. When does insert return Err(Retired)? — quinn-proto/src/cid_queue.rs:43

let Some(index) = cid.sequence.checked_sub(self.offset) else {
    return Err(InsertError::Retired);          // sequence < offset  =>  Retired
};

Any NEW_CONNECTION_ID whose sequence is below the current window offset returns Retired → uncapped push.

3. Why one frame can jump offset far ahead without buffering — cid_queue.rs:74

let orig_offset = self.offset;
self.offset = cid.retire_prior_to + i as u64;   // offset jumps to retire_prior_to
// We don't immediately retire CIDs in the range (orig_offset +
// Self::LEN)..self.offset. These are CIDs that we haven't yet received ...
// If we do receive a such a frame in the future, e.g. due to reordering, we'll
// retire it then. This ensures we can't be made to buffer an arbitrarily large
// number of RETIRE_CONNECTION_ID frames.
Ok(Some((
    orig_offset..self.offset.min(orig_offset + Self::LEN as u64),   // returned range LEN-capped
    token.expect("non-initial CID missing reset token"),
)))

A single NEW_CONNECTION_ID{sequence=N, retire_prior_to=N} with large N moves offset to N, but the returned retired range is clamped to LEN (5), so the Ok(Some) cap check trivially passes. Net effect: the window offset is now N, yet almost nothing was buffered.

The comment at lines 79–81 is the vulnerability's own disproof. It argues we can't be made to buffer unboundedly because the gap CIDs are retired lazily — "we'll retire it then." But "then" is the Err(Retired) arm above, and that arm is the one without the cap. The lazy path the comment relies on for safety is precisely the unbounded path.

4. The drain is congestion-controlled — mod.rs:3375

let Some(seq) = space.pending.retire_cids.pop() else { ... };
...
sent.retransmits.get_or_create().retire_cids.push(seq);   // one RETIRE_CONNECTION_ID per pop

pending.retire_cids is drained one entry per outbound RETIRE_CONNECTION_ID frame. RETIRE_CONNECTION_ID is ack-eliciting, so the packets carrying it count toward bytes_in_flight and are congestion-controlled (RFC 9002). An attacker that stops acknowledging the victim's ack-eliciting packets fills the victim's congestion window, which blocks the victim from emitting further RETIRE_CONNECTION_ID — the drain stalls. The attacker's own NEW_CONNECTION_ID flood is not gated by this: per RFC 9002, ACK-only packets are not congestion-controlled, so nothing the attacker sends needs to be acknowledged to keep flowing. This is a one-sided stall of the victim's retirement responses, not a full bidirectional loss of ACKs.

Attack (2 steps, post-handshake)

Any peer that has completed the handshake (1-RTT established — no application auth required):

1. Advance the window once. Send one NEW_CONNECTION_ID{ sequence = 1_000_000, retire_prior_to = 1_000_000, ... }. This takes the Ok(Some) path: offset → 1_000_000, only ~5 entries buffered, cap check passes.
2. Flood the already-retired path. Send NEW_CONNECTION_ID{ sequence = k, retire_prior_to = k, ... } for any k < 1_000_000 (e.g. k = 0), repeatedly. Each frame: sequence < offset → Err(InsertError::Retired) → uncapped push(k). No de-duplication — the same k = 0 can be pushed indefinitely.

Both steps use retire_prior_to == sequence, which passes the arm's frame.retire_prior_to > frame.sequence PROTOCOL_VIOLATION guard (it rejects only strictly-greater), and both are sent post-handshake in 1-RTT (NEW_CONNECTION_ID in 0-RTT is already rejected upstream at mod.rs:2809).

Simultaneously withholding ACKs stalls the drain (step 4 above). pending.retire_cids (a Vec<u64>) then grows monotonically with attacker frame count.

This is a protocol-conformance gap, not merely a missing optimization. RFC 9000 §19.15 requires an endpoint to send a RETIRE_CONNECTION_ID for such a connection ID "unless it has already done so for that connection ID" — the protocol explicitly anticipates duplicate/re-retired sequence numbers and mandates suppression. quinn-proto tracks no "already retired" set for this path, so it neither suppresses the duplicate nor bounds the queue: a spec-conformant implementation would do both.

Reachability (source-level)

The vulnerable push is reached from a decoded wire frame through the production handler, and every guard between decode and the push is satisfied by the attack's frame values:

UDP datagram → packet decrypt (1-RTT / Data space)
  → process_payload()                         mod.rs:2771
    → frame decode → Frame::NewConnectionId(frame)
      → [guard] is_0rtt reject-set            mod.rs:2809/2826  — passed: sent in 1-RTT, not 0-RTT
      → arm                                    mod.rs:2938
        → [guard] rem_cids.active().is_empty() → PROTOCOL_VIOLATION
                                                — passed: normal connection uses non-zero-length CIDs
        → [guard] retire_prior_to > sequence   → PROTOCOL_VIOLATION
                                                — passed: attack uses retire_prior_to == sequence
        → self.rem_cids.insert(frame)          mod.rs:2956
          → Err(InsertError::Retired)          cid_queue.rs:43  (sequence < offset)
          → self.spaces[Data].pending.retire_cids.push(frame.sequence)   mod.rs:2988  ← UNCAPPED

No state beyond a normally-established connection is required; the two guards on the arm and the upstream 0-RTT filter are all passed by legitimately-shaped frames. The only "special" precondition — that offset has been advanced past the flooded sequence — is itself established by a single legal frame (step 1 of the attack).

White-box regression test

poc/ contains an in-crate quinn-proto test that reproduces the production match logic using the real CidQueue and real Connection state, on a connection established with Pair::connect(). It is a white-box regression test, not an end-to-end packet-path test — it invokes a #[cfg(test)] helper whose body mirrors the match self.rem_cids.insert(frame) { … } arms (real CidQueue::insert, real capped Ok(Some) / uncapped Err(Retired) branches) rather than forging encrypted 1-RTT packets. The source-level reachability argument above is what connects it to the on-wire path.

• test_recv_new_cid(1_000_000, 1_000_000) once (advances offset via the capped Ok(Some) path),
• then test_recv_new_cid(0, 0) 500× (each hits Err(Retired) → uncapped push).

Result on main @ fec2f89:

[PoC] pending.retire_cids grew to 505 from 500 frames (legit cap = 50)
test result: ok. 1 passed; 0 failed

pending.retire_cids reached 505 — 10× the MAX_PENDING_RETIRED_CIDS = 50 bound that the sibling path enforces — and growth is linear in attacker frame count (no dedup), i.e. unbounded. See poc/APPLY-AND-RUN.md to reproduce.

Stronger form available on request: extract the Frame::NewConnectionId arm's body into a private Connection method called from both process_payload and the test, so the regression test exercises the same code as production instead of a mirror of it. This removes the "you copied the code under test" objection entirely; happy to include it in the fix PR.

Impact (honest scope)

• Reachability: remote, application-unauthenticated, post-handshake (1-RTT). Not a pre-handshake blind packet.
• Directionality:
  • Server victim: the attacker is any client that completes the QUIC handshake — no account or application credential needed. This is the higher-impact direction (a public QUIC/HTTP-3 server accepts arbitrary clients).
  • Client victim: requires a malicious or compromised QUIC server that the client itself connected to. Lower reach, but valid (e.g. a client library pointed at a hostile endpoint).
• No favorable amplification: each NEW_CONNECTION_ID is ~25–30 wire bytes to store 8 bytes; the attacker must sustain a flood. This is resource-exhaustion by volume, not by amplification.
• Self-limiting: memory is reclaimed if the attacker resumes ACKing and the queue drains. It is gradual exhaustion under a sustained active flood, not a single-packet OOM.
• But: it bypasses a bound the developers deliberately added against exactly this ("Ensure pending_retired cannot grow without bound"). A low-rate trickle on a long-lived connection accumulates, and the effect multiplies across many concurrent connections against a server, making unbounded per-connection growth a practical memory-exhaustion vector. quinn has prior RUSTSEC DoS advisories, so remote DoS is in-scope for the project's threat model.

Rated Medium as a defensible upper bound (defeats an explicit anti-DoS invariant, remotely reachable by any peer, and is a protocol-conformance gap per RFC 9000 §19.15). A Low rating is also defensible given the negative amplification, the self-limiting behavior, and that no OOM/process-abort was demonstrated (only unbounded Vec growth past the deliberate 50-entry bound). We defer to the maintainers' severity call and would not contest a downgrade to a hardening fix.

Suggested fix

Apply the sibling path's bound to the Err(Retired) arm — the queue must be capped no matter which path fills it:

Err(InsertError::Retired) => {
    trace!("discarding already-retired");
    let pending_retired = &mut self.spaces[SpaceId::Data].pending.retire_cids;
    const MAX_PENDING_RETIRED_CIDS: u64 = CidQueue::LEN as u64 * 10;
    if pending_retired.len() as u64 >= MAX_PENDING_RETIRED_CIDS {
        return Err(TransportError::CONNECTION_ID_LIMIT_ERROR(
            "queued too many retired CIDs",
        ));
    }
    pending_retired.push(frame.sequence);
    continue;
}

The single named constant should govern both push sites; consider hoisting it out of the match so the two arms cannot drift again.

Additionally, RFC 9000 §19.15 calls for not re-queuing a retirement already performed for a given sequence. Tracking an "already retired" watermark/set (the queue only needs the highest retired sequence, since retirement is monotonic) would both suppress the duplicate and make the cap effectively unreachable under this attack — a spec-conformant fix rather than a bare bound.

For test durability, extracting the arm body into a private Connection method (called from both process_payload and the regression test) lets the fix ship with a regression test that exercises the real code path, not a mirror of it.

Disclosure

Reported privately via GitHub Security Advisory to quinn-rs/quinn. Found and verified with rust-in-peace (https://github.com/scadastrangelove/rust-in-peace). Happy to submit the fix as a PR once a coordinated timeline is agreed.