Skip to content

quic: do not register locally issued reset tokens - #67000

Open
woolfi182 wants to merge 1 commit into
nodejs:mainfrom
woolfi182:quic-local-reset-tokens
Open

woolfi182 wants to merge 1 commit into
nodejs:mainfrom
woolfi182:quic-local-reset-tokens

Conversation

@woolfi182

Copy link
Copy Markdown

The stateless reset tokens that a QUIC session generates for its own connection IDs (Session::GenerateNewConnectionId() for NEW_CONNECTION_ID frames, and TransportParams::GeneratePreferredAddressToken()) were registered in SessionManager::token_map_. That map is what Endpoint::Receive() consults to recognize a stateless reset sent by the peer; per the comments in endpoint.cc and session_manager.h, it should only contain tokens previously given by the remote peer, and sessions clean up their entries on teardown.

Teardown (Session::Impl::~Impl()) only removes the peer's tokens (ngtcp2_conn_get_active_dcid2()), so the locally issued entries were never removed and kept a raw Session* after the session was destroyed. A short header packet for an unknown connection ID ending in one of those tokens was then:

  • dispatched to the destroyed session and silently dropped, instead of being answered with a stateless reset, and
  • once the Session had been garbage collected, a use-after-free in maybeStatelessReset().

This change stops registering locally issued tokens. They are still generated and sent to the peer; nothing on the receive side needs them, since the peer's tokens are registered through the dcid_status callback.

The new test reads the server's NEW_CONNECTION_ID tokens from its qlog, destroys the session, and sends the server endpoint a short header packet that ends in one of those tokens, followed by a control packet. Each reply is matched to its packet by the reset token derived from the packet's destination connection ID. Without the fix, the server answers only the control packet, and the process often crashes with SIGSEGV (an ASAN build reports a heap-use-after-free in Session::is_destroyed()); with it, both packets get a stateless reset.

QUIC is at Stability 1.0 behind --experimental-quic, so per SECURITY.md this is handled as a regular bug.

AI disclosure: this change was developed with assistance from Claude Code (analysis, fix and test). I reviewed the change and verified locally that the new test fails without the fix and passes with it, and that parallel/test-quic-* and the QUIC cctests pass.

The stateless reset tokens that a session generates for its own
connection IDs (in NEW_CONNECTION_ID frames and for the preferred
address) were added to the token map that the endpoint consults for
incoming stateless resets. That map is meant to hold only the tokens
issued by the peer, and session teardown only removes those, so the
locally issued entries outlived the session as dangling pointers.

A short header packet for an unknown connection ID that ended in one
of these tokens was then dispatched to the destroyed session instead
of being answered with a stateless reset, and once the session had
been garbage collected the lookup read freed memory.

Assisted-by: Claude Code
Signed-off-by: woolfi182 <[email protected]>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation. labels Oct 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants