From 2053f50f35ba0f9d26e01afe01950886f9737b2e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 5 May 2026 01:41:28 +0000 Subject: [PATCH] =?UTF-8?q?fix(quic):=20discard=20Initial/Handshake=20keys?= =?UTF-8?q?=20per=20RFC=209001=20=C2=A74.9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix `:quic` held Initial AND Handshake encryption-level state indefinitely once derived. AEAD cipher state, per-level CRYPTO buffers, and the per-level sent-packet map all stayed alive for the lifetime of the connection — a real memory leak for long sessions (audio rooms run for hours). LevelState.discardKeys() (idempotent): - Nulls sendProtection / receiveProtection (frees AEAD state). - Replaces cryptoSend / cryptoReceive with empty instances. - Replaces ackTracker with an empty instance. - Clears sentPackets and resets largestAckedPn / largestAckedSentTimeMs. - Latches keysDiscarded = true. Hook locations: - Initial discard (RFC 9001 §4.9.1, client side): in QuicConnectionWriter.drainOutbound, after a Handshake-level packet is built into the outbound datagram. The next drainOutbound MUST NOT touch the Initial level; any retransmitted Initial from the peer is silently dropped (receiveProtection == null), which is correct per the same RFC since the server has also moved up encryption levels by then. - Handshake discard (RFC 9001 §4.9.2 + §4.1.2, client side): in QuicConnectionParser, on receipt of a HANDSHAKE_DONE frame. Once a level's protection is null, parser-side decrypt at that level returns null silently (existing receiveProtection == null check) and writer-side build skips it (existing sendProtection == null check), so no further code paths needed updating. New test: KeyDiscardTest (4 cases — Initial keys discarded after first Handshake packet, Handshake keys still live until HANDSHAKE_DONE, Handshake keys discarded on HANDSHAKE_DONE, discardKeys is idempotent). Listed in the audit-summary deferred-work as item 3 (`No Initial / Handshake key discard`). https://claude.ai/code/session_01PYYez8a6sjiakyjAxsfCEQ --- quic/plans/2026-04-26-quic-stack-status.md | 19 ++- .../quic/connection/LevelState.kt | 62 ++++++++- .../quic/connection/QuicConnectionParser.kt | 6 + .../quic/connection/QuicConnectionWriter.kt | 58 ++++++--- .../quic/connection/KeyDiscardTest.kt | 123 ++++++++++++++++++ 5 files changed, 237 insertions(+), 31 deletions(-) create mode 100644 quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyDiscardTest.kt diff --git a/quic/plans/2026-04-26-quic-stack-status.md b/quic/plans/2026-04-26-quic-stack-status.md index 6e1b4fd8e..07f878f81 100644 --- a/quic/plans/2026-04-26-quic-stack-status.md +++ b/quic/plans/2026-04-26-quic-stack-status.md @@ -188,16 +188,21 @@ rooms don't exercise heavily: and RESET_STREAM / STOP_SENDING / NEW_CONNECTION_ID retransmit. See [`2026-05-04-control-frame-retransmit.md`](2026-05-04-control-frame-retransmit.md). 2. ~~**`SendBuffer` doesn't retain bytes until ACK.**~~ Resolved with #1. -3. **No Initial / Handshake key discard.** RFC 9000 §17.2.2 / RFC 9001 §4.9 - require dropping these after handshake completes; we hold them - indefinitely. Memory leak per long session. +3. ~~**No Initial / Handshake key discard.**~~ **Resolved 2026-05-05** — + `LevelState.discardKeys()` nulls the AEAD protection, replaces the + per-level CRYPTO buffers and ack-tracker with empty instances, and + clears the sent-packet map. Initial keys discarded by the writer + after the first Handshake packet is built (RFC 9001 §4.9.1); + Handshake keys discarded by the parser on receipt of HANDSHAKE_DONE + (RFC 9001 §4.9.2 + §4.1.2 client-side handshake confirmation). 4. **No path validation for `NEW_CONNECTION_ID`.** We don't migrate. 5. **Stateless reset detection.** Stateless-reset packets look like corruption to us. -6. **`AckTracker.purgeBelow` threshold semantics.** Pre-existing bug: - purges based on peer's largestAcknowledged of OUR outbound PNs, but - purges OUR inbound PN tracker. Causes range-list bloat, not correctness - failure. +6. ~~**`AckTracker.purgeBelow` threshold semantics.**~~ **Resolved + 2026-05-05** — `RecoveryToken.Ack` now carries `(level, largestAcked)`; + the writer captures these from the AckFrame at emit time, and + `QuicConnection.onTokensAcked` purges the matching per-level inbound + tracker on ACK-of-ACK. The wrong purge in the parser is gone. 7. **Driver direct unit tests** require turning `UdpSocket` from `expect class` into an interface so the test side can stub. The driver is covered indirectly by every pipe-based test plus the live interop diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/LevelState.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/LevelState.kt index 919c049a1..b3ba89e64 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/LevelState.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/LevelState.kt @@ -27,11 +27,18 @@ import com.vitorpamplona.quic.stream.SendBuffer /** Per-encryption-level state owned by [QuicConnection]. */ class LevelState { val pnSpace = PacketNumberSpaceState() - val ackTracker = + + var ackTracker = com.vitorpamplona.quic.recovery .AckTracker() - val cryptoSend = SendBuffer() - val cryptoReceive = ReceiveBuffer() + private set + + var cryptoSend = SendBuffer() + private set + + var cryptoReceive = ReceiveBuffer() + private set + var sendProtection: PacketProtection? = null var receiveProtection: PacketProtection? = null @@ -66,4 +73,53 @@ class LevelState { * Null until an ACK arrives. */ var largestAckedSentTimeMs: Long? = null + + /** + * RFC 9001 §4.9: latches true once [discardKeys] runs. Used by + * the writer / parser to short-circuit operations on a discarded + * level (parser already drops packets via the + * [receiveProtection] null check; this flag is for the symmetry + * tests + future code that wants to assert "level is dead"). + */ + var keysDiscarded: Boolean = false + private set + + /** + * RFC 9001 §4.9: drop all key material + buffered state for this + * encryption level once the level is no longer needed. Idempotent. + * + * - §4.9.1: a client MUST discard Initial keys when it first + * sends a Handshake packet. Trigger lives in + * [com.vitorpamplona.quic.connection.QuicConnectionWriter] — + * after we build a Handshake-level packet, we discard Initial. + * - §4.9.2: an endpoint MUST discard Handshake keys when the + * handshake is confirmed. Per §4.1.2 a client confirms the + * handshake on receipt of a HANDSHAKE_DONE frame; the trigger + * lives in [com.vitorpamplona.quic.connection.QuicConnectionParser]. + * + * Frees the AEAD cipher state, drops any unsent / unacked CRYPTO + * bytes (no longer reachable since they were handshake-only), and + * resets the loss-detection accounting. Future inbound packets at + * this encryption level are dropped silently because + * [receiveProtection] is null. Future outbound builds skip the + * level for the same reason on [sendProtection]. + * + * The class-level docstring on [LevelState] still describes the + * fields as if they're permanent; after [discardKeys] those + * descriptions only apply while [keysDiscarded] is false. + */ + fun discardKeys() { + if (keysDiscarded) return + sendProtection = null + receiveProtection = null + cryptoSend = SendBuffer() + cryptoReceive = ReceiveBuffer() + ackTracker = + com.vitorpamplona.quic.recovery + .AckTracker() + sentPackets.clear() + largestAckedPn = null + largestAckedSentTimeMs = null + keysDiscarded = true + } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt index 03e825754..284b05f4f 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -419,6 +419,12 @@ private fun dispatchFrames( if (conn.status == QuicConnection.Status.HANDSHAKING) { conn.status = QuicConnection.Status.CONNECTED } + // RFC 9001 §4.9.2 + §4.1.2: a client confirms the handshake + // on receipt of HANDSHAKE_DONE; once confirmed, MUST discard + // Handshake keys. Frees the AEAD cipher state and any + // residual handshake CRYPTO bookkeeping that's no longer + // needed for the lifetime of the connection. + conn.handshake.discardKeys() } is PingFrame -> { diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt index eaf711ff3..96bd15f8f 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -113,29 +113,45 @@ fun drainOutbound( // RFC 9000 §14.1: client datagrams containing Initial MUST pad to ≥ 1200, // via PADDING frames inside the Initial's encryption envelope. - if (initialNatural != null) { - var natural = 0 - for (p in firstPass) natural += p.size - if (natural < 1200) { - val deficit = 1200 - natural - // Rewind the Initial PN — we'll reissue with the same PN and the - // same captured frames plus padding. The SentPacket entry recorded - // by the natural-size build will be overwritten by the rebuild - // below since both use the same PN. - initialState.pnSpace.rewindOutboundForRebuild() - val paddedInitial = - buildLongHeaderFromFrames( - conn = conn, - level = EncryptionLevel.INITIAL, - frames = initialContents!!.frames, // null-safe: gated by `initialNatural != null` above - tokens = initialContents.tokens, - nowMillis = nowMillis, - padBytes = deficit, - ) - return concat(listOfNotNull(paddedInitial, handshakeNatural, applicationPkt)) + val datagram = + if (initialNatural != null) { + var natural = 0 + for (p in firstPass) natural += p.size + if (natural < 1200) { + val deficit = 1200 - natural + // Rewind the Initial PN — we'll reissue with the same PN and the + // same captured frames plus padding. The SentPacket entry recorded + // by the natural-size build will be overwritten by the rebuild + // below since both use the same PN. + initialState.pnSpace.rewindOutboundForRebuild() + val paddedInitial = + buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.INITIAL, + frames = initialContents!!.frames, // null-safe: gated by `initialNatural != null` above + tokens = initialContents.tokens, + nowMillis = nowMillis, + padBytes = deficit, + ) + concat(listOfNotNull(paddedInitial, handshakeNatural, applicationPkt)) + } else { + concat(firstPass) + } + } else { + concat(firstPass) } + + // RFC 9001 §4.9.1: a client MUST discard Initial keys when it first + // sends a Handshake packet. We just built one (handshakeNatural != + // null), so the next drainOutbound MUST NOT touch the Initial level. + // After this point any retransmitted Initial from the peer is silently + // dropped (initial.receiveProtection == null), which is correct per + // the same RFC: by the time the client sends a Handshake packet, the + // server has already moved up encryption levels too. + if (handshakeNatural != null && !conn.initial.keysDiscarded) { + conn.initial.discardKeys() } - return concat(firstPass) + return datagram } private fun concat(parts: List): ByteArray { diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyDiscardTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyDiscardTest.kt new file mode 100644 index 000000000..571c6777d --- /dev/null +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyDiscardTest.kt @@ -0,0 +1,123 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quic.connection + +import com.vitorpamplona.quic.frame.HandshakeDoneFrame +import com.vitorpamplona.quic.frame.PingFrame +import com.vitorpamplona.quic.tls.PermissiveCertificateValidator +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * RFC 9001 §4.9: Initial / Handshake keys MUST be discarded once the + * level is no longer needed. Pre-fix `:quic` held both indefinitely, + * causing a per-session memory leak (AEAD cipher state + handshake + * CRYPTO buffers) for the lifetime of long connections. + * + * - §4.9.1 client side: discard Initial keys when the client first + * sends a Handshake packet. Hooked from + * [QuicConnectionWriter.drainOutbound] after a Handshake packet is + * built into the outbound datagram. + * - §4.9.2 + §4.1.2 client side: handshake confirmation = receipt of + * HANDSHAKE_DONE; discard Handshake keys at that point. Hooked from + * [QuicConnectionParser]'s HandshakeDoneFrame branch. + */ +class KeyDiscardTest { + private fun newConnectedClient(): Pair { + val client = + QuicConnection( + serverName = "example.test", + config = QuicConnectionConfig(), + tlsCertificateValidator = PermissiveCertificateValidator(), + ) + val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes) + client.start() + pipe.drive(maxRounds = 16) + check(client.status == QuicConnection.Status.CONNECTED) { + "handshake must succeed; status=${client.status}" + } + return client to pipe + } + + @Test + fun initialKeys_discardedAfterFirstHandshakePacket() { + val (client, _) = newConnectedClient() + // After the handshake, drive one more drainOutbound to flush + // the client's Finished message — pipe.drive returns as soon as + // status flips to CONNECTED, which happens after feedDatagram + // processes the server's Finished but BEFORE the client emits + // its own Finished. + drainOutbound(client, nowMillis = 0L) + assertTrue(client.initial.keysDiscarded, "Initial keys must be discarded after first Handshake packet") + assertNull(client.initial.sendProtection, "Initial sendProtection nulled out") + assertNull(client.initial.receiveProtection, "Initial receiveProtection nulled out") + assertTrue( + client.initial.sentPackets.isEmpty(), + "Initial sentPackets cleared (no more loss-detection at this level)", + ) + } + + @Test + fun handshakeKeys_survivePastHandshakeUntilHandshakeDone() { + val (client, _) = newConnectedClient() + // Even though TLS finished, the client hasn't seen HANDSHAKE_DONE + // yet. Per §4.1.2 the handshake isn't "confirmed" until then; the + // Handshake keys must still be available for ACK exchange. + assertFalse(client.handshake.keysDiscarded, "Handshake keys still live before HANDSHAKE_DONE") + assertNotNull(client.handshake.sendProtection) + assertNotNull(client.handshake.receiveProtection) + } + + @Test + fun handshakeKeys_discardedOnHandshakeDone() { + val (client, pipe) = newConnectedClient() + // Inject a HANDSHAKE_DONE at APPLICATION level. Pad with PINGs so + // the encrypted payload is long enough for header-protection's + // 16-byte sample window. + val pings = List(40) { PingFrame } + val packet = pipe.buildServerApplicationDatagram(listOf(HandshakeDoneFrame()) + pings)!! + feedDatagram(client, packet, nowMillis = 0L) + + assertTrue(client.handshake.keysDiscarded, "HANDSHAKE_DONE must trigger Handshake key discard") + assertNull(client.handshake.sendProtection) + assertNull(client.handshake.receiveProtection) + assertTrue(client.handshake.sentPackets.isEmpty()) + // Sanity: Application keys live on; the connection still works. + assertNotNull(client.application.sendProtection, "Application keys untouched") + } + + @Test + fun discardKeys_isIdempotent() { + val (client, _) = newConnectedClient() + // Initial keys already discarded by the handshake exchange. A + // second call must not throw and must not mutate any other state. + client.initial.discardKeys() + client.initial.discardKeys() + assertTrue(client.initial.keysDiscarded) + // Application level was never discarded; calling on a still-live + // level is also valid (used in real life on connection close + // for cleanup). + assertFalse(client.application.keysDiscarded) + } +}