fix(quic): address self-audit findings on #2861 fixes (PR #2873)

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
2026-05-13 19:19:05 +00:00
parent fbacef1f2a
commit 09b28b8d78
6 changed files with 121 additions and 21 deletions
@@ -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(
@@ -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<ByteArray> = 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
@@ -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
        }
@@ -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,
                )
            }
        }
@@ -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.
@@ -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.)