fix(quic): discard Initial/Handshake keys per RFC 9001 §4.9
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
This commit is contained in:
Claude
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 -> {
|
||||
|
||||
+37
-21
@@ -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>): ByteArray {
|
||||
|
||||
@@ -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<QuicConnection, InMemoryQuicPipe> {
|
||||
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)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user