From 09b28b8d789f70ca7b67f0904e2debd47c611bbe Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 19:19:05 +0000 Subject: [PATCH] fix(quic): address self-audit findings on #2861 fixes (PR #2873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three issues found in the self-audit of commit fbacef1f: S2 clock bug (CRITICAL): the previous fix compared TlsResumptionState. issuedAtMillis (wallclock — stored via System.currentTimeMillis() in TlsClient) against QuicConnection.nowMillis() (monotonic — anchored at construction). On any new connection the monotonic clock starts near zero, so `(nowMillis() - issuedAtMillis)` produced a large negative value, the coerceAtLeast(0L) clamped it to 0, and the expiry check never fired. Add a dedicated `epochMillis: () -> Long` parameter on QuicConnection (default System.currentTimeMillis()) and use that for ticket-age comparisons — matches the source TlsClient stamps the ticket with. S3 null-ALPN filter: the effectiveResumption filter rejected ANY ticket whose cached negotiatedAlpn was null. That breaks resumption for servers that don't negotiate ALPN at all (legitimate cold-handshake case), as well as for any persisted ticket predating the negotiatedAlpn cache field. Treat absent cached ALPN as "no binding to honour" and allow resumption — the RFC 9001 §4.6.1 restriction is about ALPN MISMATCH, not absence. The post-EE rejected0Rtt check mirrors the same null-tolerant shape. P2 scratch threading: the previous commit added aeadNonceInto but didn't thread a persistent scratch through the call sites, so the hot path still allocated a fresh 12-byte nonce per packet. Add PacketProtection.nonceScratch (sized to iv.size, single-direction so single-threaded), thread it through Short/LongHeaderPacket build + parseAndDecrypt as an optional `nonceScratch: ByteArray? = null` parameter, and pass `proto.nonceScratch` from the four production call sites in QuicConnectionWriter / QuicConnectionParser. Tests that construct packets directly are unchanged (default null = allocate fresh). All quic JVM unit tests pass; spotless applied. https://claude.ai/code/session_01EBHtGLy5o7FUR5qfcpHUUx --- .../quic/connection/EncryptionLevel.kt | 19 ++++- .../quic/connection/QuicConnection.kt | 72 ++++++++++++++----- .../quic/connection/QuicConnectionParser.kt | 4 ++ .../quic/connection/QuicConnectionWriter.kt | 5 ++ .../quic/packet/LongHeaderPacket.kt | 21 +++++- .../quic/packet/ShortHeaderPacket.kt | 21 +++++- 6 files changed, 121 insertions(+), 21 deletions(-) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/EncryptionLevel.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/EncryptionLevel.kt index 051d8f539..628a51b32 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/EncryptionLevel.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/EncryptionLevel.kt @@ -29,7 +29,24 @@ class PacketProtection( val iv: ByteArray, val hp: com.vitorpamplona.quic.crypto.HeaderProtection, val hpKey: ByteArray, -) +) { + /** + * Per-direction nonce scratch buffer, sized to match [iv] (12 bytes for + * QUIC's AEADs). Reused by hot-path callers via + * [com.vitorpamplona.quic.crypto.aeadNonceInto] so the AEAD nonce no + * longer allocates a fresh ByteArray on every encrypt/decrypt (round-5 + * #P2). + * + * Thread-safety: a `PacketProtection` instance only ever lives in ONE + * direction (send or receive) at one encryption level. The writer and + * parser both operate under `streamsLock`, so the scratch is touched + * by at most one coroutine at a time. Callers that need to keep a + * nonce around past a single seal/open call must copy it; the buffer + * is overwritten on the next [com.vitorpamplona.quic.crypto.aeadNonceInto] + * invocation. + */ + val nonceScratch: ByteArray = ByteArray(iv.size) +} /** All four encryption levels we ever see in a QUIC client connection. */ enum class EncryptionLevel( diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt index 55f72a93b..f57101547 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -83,6 +83,22 @@ class QuicConnection( * timestamps in qlog) and understand the wallclock pitfalls. */ val nowMillis: () -> Long = defaultMonotonicNowMillis(), + /** + * Wallclock (epoch) clock used ONLY for cross-connection age comparisons — + * specifically, validating that a cached TLS session ticket hasn't aged + * past its server-advertised lifetime (RFC 8446 §4.6.1). Distinct from + * [nowMillis] because the monotonic clock resets on every connection + * construction; comparing a ticket's wallclock issuedAt against a fresh + * monotonic clock would yield meaningless deltas (typically large + * negative, coerced to 0 → ticket never expires). + * + * Must produce values comparable to [com.vitorpamplona.quic.tls.TlsResumptionState.issuedAtMillis], + * which [com.vitorpamplona.quic.tls.TlsClient] populates via its own + * `nowMillisSource` (wallclock by default). + * + * Tests can inject a fixed wallclock to drive expiry deterministically. + */ + val epochMillis: () -> Long = { defaultEpochMillis() }, val alpnList: List = listOf(TlsConstants.ALPN_H3), /** * Optional second listener invoked after the connection's own @@ -1001,12 +1017,14 @@ class QuicConnection( // server that picks a different ALPN from what we // remembered. val resumed = effectiveResumption - val alpnMatch = - resumed?.negotiatedAlpn?.contentEquals(tls.negotiatedAlpn ?: ByteArray(0)) ?: true + val alpnMismatch = + resumed != null && + resumed.negotiatedAlpn != null && + !resumed.negotiatedAlpn.contentEquals(tls.negotiatedAlpn) val rejected0Rtt = resumed != null && resumed.maxEarlyDataSize > 0 && - (!tls.earlyDataAccepted || !alpnMatch) + (!tls.earlyDataAccepted || alpnMismatch) if (rejected0Rtt) { requeueAllInflightStreamData() application.cryptoSend.requeueAllInflight() @@ -1102,8 +1120,8 @@ class QuicConnection( /** * Resumption state actually passed to [TlsClient] — `null` (cold * handshake) if the cached ticket is past its server-advertised - * lifetime, or if the resumed session's negotiated ALPN isn't in our - * current [alpnList]. + * lifetime, or if the resumed session's negotiated ALPN doesn't + * overlap our current [alpnList]. * * - RFC 8446 §4.6.1 — tickets MUST NOT be used past `ticket_lifetime` * seconds after issue (clipped at 7 days). An expired ticket @@ -1116,21 +1134,35 @@ class QuicConnection( * under a session whose ALPN binding no longer holds. The post-EE * `rejected0Rtt` check below covers the same lane for servers that * pick a different ALPN from what we cached. + * + * Cross-clock note: ticket-age comparison uses [epochMillis] (wallclock) + * to match what [com.vitorpamplona.quic.tls.TlsClient] stamps into + * [com.vitorpamplona.quic.tls.TlsResumptionState.issuedAtMillis]. The + * connection-local [nowMillis] (monotonic, anchored at construction) + * would yield meaningless deltas and silently bypass the check. */ private val effectiveResumption: com.vitorpamplona.quic.tls.TlsResumptionState? = resumption?.takeIf { r -> - // 7-day clip per RFC 8446 §4.6.1 (any larger advertised - // lifetime is the server bypassing the spec; we honour the - // cap regardless). + // RFC 8446 §4.6.1: 7-day cap on usable ticket lifetime. The + // server's advertised lifetime is clipped here regardless of + // what it offered. Note also that `>` (not `>=`) admits a + // ticket on its exact-lifetime boundary — RFC wording is + // "MUST NOT be used after"; treat the boundary as still + // usable since the server-side window is typically several + // seconds wider than the advertised number anyway. val effectiveLifetimeSec = r.ticketLifetimeSec.coerceAtMost(7L * 24L * 60L * 60L) - val ageSec = ((nowMillis() - r.issuedAtMillis).coerceAtLeast(0L)) / 1000L - if (ageSec >= effectiveLifetimeSec) return@takeIf false - // ALPN continuity: drop resumption when our offered ALPN - // list doesn't include the resumed session's ALPN. Use - // null-cached ALPNs (pre-2026-05 tickets) conservatively - // — without the binding we can't prove continuity, so - // skip resumption entirely. - val cachedAlpn = r.negotiatedAlpn ?: return@takeIf false + val ageSec = ((epochMillis() - r.issuedAtMillis).coerceAtLeast(0L)) / 1000L + if (ageSec > effectiveLifetimeSec) return@takeIf false + // ALPN continuity: skip resumption if the cached session's + // ALPN isn't in our offered list. When the cached ALPN is + // null (server didn't negotiate one on the prior connection, + // or the field is missing on a serialized-from-older-build + // ticket) we treat the absence as "no ALPN binding to + // honour" and allow resumption — the RFC 9001 §4.6.1 + // restriction is about ALPN MISMATCH, not absence. Match + // the same null-tolerant shape the existing + // `rejected0Rtt` post-EE branch uses. + val cachedAlpn = r.negotiatedAlpn ?: return@takeIf true alpnList.any { it.contentEquals(cachedAlpn) } } @@ -3317,6 +3349,14 @@ private fun defaultMonotonicNowMillis(): () -> Long { return { anchor.elapsedNow().inWholeMilliseconds } } +/** + * Default wallclock (epoch) supplier for [QuicConnection.epochMillis]. + * Java's `System.currentTimeMillis()` is the same source [com.vitorpamplona.quic.tls.TlsClient] + * uses by default to stamp `TlsResumptionState.issuedAtMillis`, so deltas + * across the pair are meaningful even across process restarts. + */ +private fun defaultEpochMillis(): Long = System.currentTimeMillis() + /** * One source CID we've issued to the peer (RFC 9000 §5.1.1 + * §19.15). Sequence 0 is the initial CID, implicitly issued via the 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 e4547e4fb..97b9cd59e 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -320,6 +320,7 @@ private fun feedLongHeaderPacket( hp = proto.hp, hpKey = proto.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, + nonceScratch = proto.nonceScratch, ) if (parsed == null) { conn.qlogObserver.onPacketDropped( @@ -442,6 +443,7 @@ private fun feedShortHeaderPacket( hp = live.hp, hpKey = live.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, + nonceScratch = live.nonceScratch, ) rotateOnSuccess = null } else { @@ -460,6 +462,7 @@ private fun feedShortHeaderPacket( hp = prev.hp, hpKey = prev.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, + nonceScratch = prev.nonceScratch, ) } if (priorTry != null) { @@ -489,6 +492,7 @@ private fun feedShortHeaderPacket( hp = nextPhase.hp, hpKey = nextPhase.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, + nonceScratch = nextPhase.nonceScratch, ) rotateOnSuccess = nextPhase } 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 9c62c40a8..aa6031233 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -359,6 +359,7 @@ private fun buildBestLevelPacket( proto.hp, proto.hpKey, largestAckedInSpace = -1L, + nonceScratch = proto.nonceScratch, ) emitQlogSent(conn, EncryptionLevel.APPLICATION, pn, built.size, frames) return built @@ -405,6 +406,7 @@ private fun buildLongHeaderPacket( proto.hp, proto.hpKey, largestAckedInSpace = -1L, + nonceScratch = proto.nonceScratch, ) emitQlogSent(conn, level, pn, built.size, frames) return built @@ -559,6 +561,7 @@ private fun buildLongHeaderFromFrames( proto.hp, proto.hpKey, largestAckedInSpace = -1L, + nonceScratch = proto.nonceScratch, ) // Step E: retain the packet for RFC 9002 retransmit. Initial / // Handshake packets carry CRYPTO frames; loss detection runs at @@ -912,6 +915,7 @@ private fun buildApplicationPacket( proto.hp, proto.hpKey, largestAckedInSpace = -1L, + nonceScratch = proto.nonceScratch, ) } else { // 0-RTT — long header type=0x01. Same Application packet @@ -933,6 +937,7 @@ private fun buildApplicationPacket( proto.hp, proto.hpKey, largestAckedInSpace = -1L, + nonceScratch = proto.nonceScratch, ) } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt index f4930ffb9..692999705 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt @@ -27,6 +27,7 @@ import com.vitorpamplona.quic.connection.ConnectionId import com.vitorpamplona.quic.crypto.Aead import com.vitorpamplona.quic.crypto.HeaderProtection import com.vitorpamplona.quic.crypto.aeadNonce +import com.vitorpamplona.quic.crypto.aeadNonceInto import com.vitorpamplona.quic.crypto.applyHeaderProtectionMask /** @@ -72,6 +73,9 @@ object LongHeaderPacket { hp: HeaderProtection, hpKey: ByteArray, largestAckedInSpace: Long, + // Round-5 #P2: optional 12-byte scratch the writer can reuse across + // packets. Default = allocate fresh (preserves test ergonomics). + nonceScratch: ByteArray? = null, ): ByteArray { val pnLen = com.vitorpamplona.quic.connection.PacketNumberSpaceState.encodeLength( @@ -116,7 +120,12 @@ object LongHeaderPacket { // four ByteArrays per outbound packet. The single-buffer + // [Aead.sealInto] form below collapses the seal output and // concat into the same allocation. - val nonce = aeadNonce(iv, plain.packetNumber) + val nonce = + if (nonceScratch != null) { + aeadNonceInto(iv, plain.packetNumber, nonceScratch) + } else { + aeadNonce(iv, plain.packetNumber) + } val packet = ByteArray(headerBytes.size + paddedPlaintext.size + aead.tagLength) headerBytes.copyInto(packet, 0) aead.sealInto( @@ -160,6 +169,9 @@ object LongHeaderPacket { hp: HeaderProtection, hpKey: ByteArray, largestReceivedInSpace: Long, + // Round-5 #P2: optional 12-byte scratch the parser reuses across + // inbound packets. Default = allocate fresh (preserves tests). + nonceScratch: ByteArray? = null, ): ParseResult? { val packetStart = offset val r = QuicReader(bytes, offset) @@ -244,7 +256,12 @@ object LongHeaderPacket { ) val aadEnd = localPnOffset + pnLen - val nonce = aeadNonce(iv, fullPn) + val nonce = + if (nonceScratch != null) { + aeadNonceInto(iv, fullPn, nonceScratch) + } else { + aeadNonce(iv, fullPn) + } // Range-based open avoids two ByteArray slice allocations per // inbound packet — see [ShortHeaderPacket.parseAndDecrypt] for // rationale. diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt index 941deed5c..655b0e789 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt @@ -26,6 +26,7 @@ import com.vitorpamplona.quic.connection.PacketNumberSpaceState import com.vitorpamplona.quic.crypto.Aead import com.vitorpamplona.quic.crypto.HeaderProtection import com.vitorpamplona.quic.crypto.aeadNonce +import com.vitorpamplona.quic.crypto.aeadNonceInto import com.vitorpamplona.quic.crypto.applyHeaderProtectionMask /** @@ -53,6 +54,9 @@ object ShortHeaderPacket { hp: HeaderProtection, hpKey: ByteArray, largestAckedInSpace: Long, + // Round-5 #P2: optional 12-byte scratch the writer can reuse across + // packets. Default = allocate fresh (preserves test ergonomics). + nonceScratch: ByteArray? = null, ): ByteArray { val pnLen = PacketNumberSpaceState.encodeLength(plain.packetNumber, largestAckedInSpace) require(pnLen in 1..4) @@ -80,7 +84,12 @@ object ShortHeaderPacket { // ciphertext+tag directly into it instead of allocating a fresh // `seal()` return + a concat buffer. Saves 2 ByteArrays per // outbound short-header packet. - val nonce = aeadNonce(iv, plain.packetNumber) + val nonce = + if (nonceScratch != null) { + aeadNonceInto(iv, plain.packetNumber, nonceScratch) + } else { + aeadNonce(iv, plain.packetNumber) + } val packet = ByteArray(headerBytes.size + paddedPlaintext.size + aead.tagLength) headerBytes.copyInto(packet, 0) aead.sealInto( @@ -163,6 +172,9 @@ object ShortHeaderPacket { hp: HeaderProtection, hpKey: ByteArray, largestReceivedInSpace: Long, + // Round-5 #P2: optional 12-byte scratch the parser reuses across + // inbound packets. Default = allocate fresh (preserves tests). + nonceScratch: ByteArray? = null, ): ParseResult? { if (offset >= bytes.size) return null val first = bytes[offset].toInt() and 0xFF @@ -203,7 +215,12 @@ object ShortHeaderPacket { } val fullPn = PacketNumberSpaceState.decodePacketNumber(largestReceivedInSpace, truncatedPn, pnLen) val aadEnd = localPnOffset + pnLen - val nonce = aeadNonce(iv, fullPn) + val nonce = + if (nonceScratch != null) { + aeadNonceInto(iv, fullPn, nonceScratch) + } else { + aeadNonce(iv, fullPn) + } // Range-based open: aad = packet[0..aadEnd), ciphertext = packet[aadEnd..size). // Saves the two ByteArray slice allocations that the // whole-array form (`aad = copyOfRange(0, aadEnd)` etc.)