From fbacef1f2ae2eecae5a4789b33dbd28d3d28df58 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 18:55:52 +0000 Subject: [PATCH 1/3] fix(quic): address #2861 code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the 10 actionable items from the QUIC code review in issue #2861; the lone P4 item (encrypt-under-streamsLock) is already tracked as deferred phase 2 work in quic/plans/2026-05-08-lock-split-design.md and is unchanged. Android-only correctness (would silently break on API 26–32): - A1 JdkCertificateValidator: wrap Signature.getInstance("Ed25519") in try/catch and surface NoSuchAlgorithmException as a clean QuicCodecException so an Ed25519 leaf cert no longer crashes the TLS read loop on pre-API-33 Android. - A2 TlsRunningSha256 (jvmAndroid): keep a parallel byte accumulator and fall back to one-shot SHA-256 on the rare API-26–28 Conscrypt builds whose OpenSSLMessageDigestJDK throws CloneNotSupportedException from MessageDigest.clone(). Latches the fallback after first failure so we don't pay the JCA throw per snapshot. Hot-path performance: - P1 HeaderProtection: add maskAt(hpKey, src, srcOffset) + extend AesOneBlockEncrypt with encryptInto(key, src, srcOffset, dst, dstOffset) so the per-packet HP path no longer allocates a 16-byte sample slice AND no longer allocates a 16-byte JCA Cipher output (the jvmAndroid impl uses Cipher.doFinal's range overload). Updated 5 call sites in ShortHeaderPacket and LongHeaderPacket. - P2 aeadNonce: add aeadNonceInto(staticIv, packetNumber, dst) so call sites with a persistent 12-byte scratch can build the nonce without per-packet allocation; aeadNonce keeps its existing shape via the new helper. Threading the scratch through Short/LongHeaderPacket and the writer is deferred (similar shape to the documented P4 phase 2 work). - P3 QuicConnectionParser: decode the frame list once per inbound packet, feed both qlog (frameNamesFor) and dispatch from the single decode. Pre-fix every qlog-attached packet ran decodeFrames twice. - P5 QuicConnectionWriter: iterate pendingMaxStreamData / pendingNewConnectionId directly instead of allocating entries.toList() per drain. Protocol / security: - S1 QuicConnectionParser: cap MAX_STREAMS at 2^60 (RFC 9000 §19.11); a peer sending a larger value now triggers STREAM_LIMIT_ERROR close rather than overflowing the local nextLocalBidi/UniIndex counters. - S2 QuicConnection.effectiveResumption: drop the cached session ticket when (now - issuedAt) ≥ min(ticketLifetimeSec, 7 days) per RFC 8446 §4.6.1; expired tickets would otherwise silently fail server-side and lose any 0-RTT bytes. - S3 QuicConnection: refuse to offer 0-RTT when our current alpnList doesn't include the resumed session's negotiated ALPN, and treat 0-RTT as rejected on EE if the new ALPN differs from the cached one (RFC 9001 §4.6.1). - S4 TlsExtension.encodeSignatureAlgorithms: drop rsa_pkcs1_sha256. The validator already rejects it in CertificateVerify per RFC 8446 §4.2.3 — advertising it lied about what we accept. All quic JVM unit tests pass; spotless applied. https://claude.ai/code/session_01EBHtGLy5o7FUR5qfcpHUUx --- .../quic/connection/QuicConnection.kt | 63 +++++++++++-- .../quic/connection/QuicConnectionParser.kt | 89 +++++++++++++------ .../quic/connection/QuicConnectionWriter.kt | 13 +-- .../com/vitorpamplona/quic/crypto/Aead.kt | 30 ++++++- .../quic/crypto/HeaderProtection.kt | 84 +++++++++++++++-- .../quic/packet/LongHeaderPacket.kt | 8 +- .../quic/packet/ShortHeaderPacket.kt | 11 ++- .../vitorpamplona/quic/tls/TlsExtension.kt | 8 +- .../quic/crypto/PlatformCrypto.kt | 34 +++++-- .../quic/tls/JdkCertificateValidator.kt | 40 +++++++-- .../quic/tls/TlsRunningSha256.kt | 40 +++++++-- 11 files changed, 338 insertions(+), 82 deletions(-) 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 93b3f9f682..55f72a93b1 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -987,10 +987,26 @@ class QuicConnection( // listener (handleServerFinished → onApplicationKeysReady) // runs inside streamsLock.withLock { feedDatagram(...) } // in the read loop. + // RFC 9001 §4.6.1: treat 0-RTT as REJECTED if either + // - the server did not echo the early_data extension + // in EncryptedExtensions (`!tls.earlyDataAccepted`), or + // - the negotiated ALPN differs from the resumed + // session's ALPN. Even when `earlyDataAccepted` is + // true the spec forbids using 0-RTT under a new + // ALPN binding, so we must replay any already-sent + // 0-RTT bytes under 1-RTT. + // The `effectiveResumption` filter above prevents us + // from OFFERING 0-RTT with an incompatible ALPN — this + // post-EE check is the safety net for a non-conforming + // server that picks a different ALPN from what we + // remembered. + val resumed = effectiveResumption + val alpnMatch = + resumed?.negotiatedAlpn?.contentEquals(tls.negotiatedAlpn ?: ByteArray(0)) ?: true val rejected0Rtt = - resumption != null && - resumption.maxEarlyDataSize > 0 && - !tls.earlyDataAccepted + resumed != null && + resumed.maxEarlyDataSize > 0 && + (!tls.earlyDataAccepted || !alpnMatch) if (rejected0Rtt) { requeueAllInflightStreamData() application.cryptoSend.requeueAllInflight() @@ -1083,6 +1099,41 @@ class QuicConnection( if (!handshakeConfirmedSignal.isCompleted) handshakeConfirmedSignal.completeExceptionally(cause) } + /** + * 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]. + * + * - RFC 8446 §4.6.1 — tickets MUST NOT be used past `ticket_lifetime` + * seconds after issue (clipped at 7 days). An expired ticket + * silently fails to resume server-side and any 0-RTT bytes are + * discarded; filtering at the call site keeps us from emitting the + * PSK extension + 0-RTT data on a doomed ticket. + * - RFC 9001 §4.6.1 — 0-RTT is forbidden when the new ALPN differs + * from the resumed session's ALPN. Defense-in-depth: a server that + * doesn't reject the offer would still see 0-RTT bytes encrypted + * 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. + */ + 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). + 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 + alpnList.any { it.contentEquals(cachedAlpn) } + } + val tls: TlsClient = TlsClient( serverName = serverName, @@ -1091,7 +1142,7 @@ class QuicConnection( certificateValidator = tlsCertificateValidator, offeredAlpns = alpnList, cipherSuites = cipherSuites, - resumption = resumption, + resumption = effectiveResumption, ) init { @@ -1117,9 +1168,9 @@ class QuicConnection( // in EncryptedExtensions and the existing // [applyPeerTransportParameters] hook then overwrites these // pre-loaded values. - if (resumption?.peerTransportParameters != null) { + if (effectiveResumption?.peerTransportParameters != null) { try { - val tp = TransportParameters.decode(resumption.peerTransportParameters) + val tp = TransportParameters.decode(effectiveResumption.peerTransportParameters) sendConnectionFlowCredit = tp.initialMaxData ?: 0L peerMaxStreamsBidi = tp.initialMaxStreamsBidi ?: 0L peerMaxStreamsUni = tp.initialMaxStreamsUni ?: 0L 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 1b960159d9..e4547e4fb3 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -52,6 +52,14 @@ import com.vitorpamplona.quic.tls.TlsClient /** RFC 9000 §16: maximum varint value, also the per-stream offset ceiling. */ private const val MAX_QUIC_OFFSET: Long = (1L shl 62) - 1L +/** + * RFC 9000 §19.11: MAX_STREAMS values strictly above 2^60 are illegal. + * Anything larger (e.g. a hostile 2^62-1) would let local stream-ID + * minting overflow Long, so we treat the receipt as STREAM_LIMIT_ERROR + * and close. + */ +private const val MAX_STREAMS_LIMIT: Long = 1L shl 60 + /** * RFC 9000 §22 CRYPTO_BUFFER_EXCEEDED cap. Largest amount of CRYPTO * data we'll buffer per encryption level past the contiguous read @@ -331,15 +339,19 @@ private fun feedLongHeaderPacket( conn.destinationConnectionId = parsed.packet.scid } + // Round-5 #P3: decode the frame list ONCE — qlog (when attached) and + // dispatch both need the same decoded view. Pre-fix the parser walked + // the payload twice per inbound packet. + val decodedFrames = decodeFramesOrClose(conn, parsed.packet.payload) ?: return parsed.consumed if (conn.qlogObserver !== com.vitorpamplona.quic.observability.QlogObserver.NoOp) { conn.qlogObserver.onPacketReceived( level = level, packetNumber = parsed.packet.packetNumber, sizeBytes = parsed.consumed, - frames = peekFrameNames(parsed.packet.payload), + frames = frameNamesFor(decodedFrames), ) } - dispatchFrames(conn, level, parsed.packet.payload, parsed.packet.packetNumber, nowMillis) + dispatchFrames(conn, level, decodedFrames, parsed.packet.packetNumber, nowMillis) return parsed.consumed } @@ -543,52 +555,60 @@ private fun feedShortHeaderPacket( // RFC 9000 §10.1.1: any successfully processed inbound packet // resets the idle timer. conn.lastActivityMs = nowMillis + // Round-5 #P3: single decode of the application-level payload feeds + // both qlog and dispatch. + val decodedFrames = decodeFramesOrClose(conn, parsed.packet.payload) ?: return if (conn.qlogObserver !== com.vitorpamplona.quic.observability.QlogObserver.NoOp) { conn.qlogObserver.onPacketReceived( level = EncryptionLevel.APPLICATION, packetNumber = parsed.packet.packetNumber, sizeBytes = datagram.size - offset, - frames = peekFrameNames(parsed.packet.payload), + frames = frameNamesFor(decodedFrames), ) } - dispatchFrames(conn, EncryptionLevel.APPLICATION, parsed.packet.payload, parsed.packet.packetNumber, nowMillis) + dispatchFrames(conn, EncryptionLevel.APPLICATION, decodedFrames, parsed.packet.packetNumber, nowMillis) } /** - * Decode the payload's frames just to surface their qlog names. Reuses - * the same [com.vitorpamplona.quic.frame.decodeFrames] path as - * [dispatchFrames]; if it throws (malformed peer payload), we return - * an empty list — the dispatch path will catch the same exception - * and surface the close via `markClosedExternally`. + * Map a decoded frame list to qlog frame-type names. Reused by callers + * that have already decoded the payload (qlog path) so we don't pay the + * varint scan twice. */ -private fun peekFrameNames(payload: ByteArray): List = +private fun frameNamesFor(frames: List): List { + val out = ArrayList(frames.size) + for (f in frames) out += qlogFrameName(f::class.simpleName ?: "frame") + return out +} + +/** + * Decode a packet payload into a frame list, gracefully closing the + * connection on a malformed (post-AEAD) payload. Returns null on close — + * the caller MUST stop processing the packet. + * + * Audit-4 #1: malformed frames in an otherwise-AEAD-validated payload (or + * unknown frame types from a future-extension peer) used to throw straight + * through the read loop's `finally` block, dropping the connection without + * ever sending CONNECTION_CLOSE. Catch decode exceptions and turn them into + * a graceful close so the peer learns why we tore down. + */ +private fun decodeFramesOrClose( + conn: QuicConnection, + payload: ByteArray, +): List? = try { - com.vitorpamplona.quic.frame - .decodeFrames(payload) - .map { qlogFrameName(it::class.simpleName ?: "frame") } - } catch (_: QuicCodecException) { - emptyList() + decodeFrames(payload) + } catch (e: QuicCodecException) { + conn.markClosedExternally("frame decode failed: ${e.message}") + null } private fun dispatchFrames( conn: QuicConnection, level: EncryptionLevel, - payload: ByteArray, + frames: List, packetNumber: Long, nowMillis: Long, ) { - // Audit-4 #1: malformed frames in an otherwise-AEAD-validated payload (or - // unknown frame types from a future-extension peer) used to throw straight - // through the read loop's `finally` block, dropping the connection without - // ever sending CONNECTION_CLOSE. Catch decode exceptions and turn them - // into a graceful close so the peer learns why we tore down. - val frames = - try { - decodeFrames(payload) - } catch (e: QuicCodecException) { - conn.markClosedExternally("frame decode failed: ${e.message}") - return - } val state = conn.levelState(level) var ackEliciting = false for (frame in frames) { @@ -908,6 +928,19 @@ private fun dispatchFrames( // RFC 9000 §19.11: MAX_STREAMS only ever raises the cap. // Frames with values smaller than the current cap are ignored. // Bidi vs uni is signaled via the frame's `bidi` flag. + // + // §19.11 also caps a valid MAX_STREAMS value at 2^60 — a peer + // that sends a larger value commits a STREAM_LIMIT_ERROR. + // Without this gate a hostile peer could push the cap up to + // 2^62-1 (varint max), and our per-connection + // `nextLocalBidiIndex`/`nextLocalUniIndex` (Long) would then + // overflow as we minted stream IDs. Treat as a fatal close. + if (frame.maxStreams > MAX_STREAMS_LIMIT) { + conn.markClosedExternally( + "STREAM_LIMIT_ERROR: peer MAX_STREAMS=${frame.maxStreams} exceeds RFC 9000 §19.11 cap of 2^60", + ) + return + } if (frame.bidi) { if (frame.maxStreams > conn.peerMaxStreamsBidi) { conn.peerMaxStreamsBidi = frame.maxStreams 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 4ed82810a7..9c62c40a88 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -1039,9 +1039,10 @@ private fun appendFlowControlUpdates( conn.pendingMaxData = null } if (conn.pendingMaxStreamData.isNotEmpty()) { - // Iterate over a snapshot so we can mutate the map safely. - val pendingStreamEntries = conn.pendingMaxStreamData.entries.toList() - for ((streamId, maxData) in pendingStreamEntries) { + // Direct map iteration — safe because we don't mutate the map + // inside the loop, only `clear()` after the walk completes. Avoids + // the per-drain `entries.toList()` allocation (round-5 #P5). + for ((streamId, maxData) in conn.pendingMaxStreamData) { frames += MaxStreamDataFrame(streamId, maxData) tokens += RecoveryToken.MaxStreamData(streamId = streamId, maxData = maxData) } @@ -1140,8 +1141,10 @@ private fun appendFlowControlUpdates( // carrier packet was declared lost. Same wire shape as a fresh // issuance; we just preserve the original token. if (conn.pendingNewConnectionId.isNotEmpty()) { - val pendingNewCidEntries = conn.pendingNewConnectionId.entries.toList() - for ((_, token) in pendingNewCidEntries) { + // Direct iteration over map values — we only mutate via `clear()` + // after the walk completes. Drops the per-drain `entries.toList()` + // (round-5 #P5). + for (token in conn.pendingNewConnectionId.values) { frames += NewConnectionIdFrame( sequenceNumber = token.sequenceNumber, diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt index 2a4feec53b..a07bf62ac3 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt @@ -247,15 +247,37 @@ object ChaCha20Poly1305Aead : Aead() { * Build a QUIC AEAD nonce from a static IV and a packet number. * * RFC 9001 §5.3: nonce = static_iv XOR (packet_number padded to nonce length, big-endian). + * + * Allocates a fresh nonce buffer on every call — see [aeadNonceInto] for the + * caller-owned-scratch variant used when the call site is on a per-packet + * hot path and can maintain a persistent 12-byte buffer (round-5 #P2). */ fun aeadNonce( staticIv: ByteArray, packetNumber: Long, +): ByteArray = aeadNonceInto(staticIv, packetNumber, ByteArray(staticIv.size)) + +/** + * Build a QUIC AEAD nonce into a caller-owned [dst] buffer. [dst] must + * have the same size as [staticIv] (12 bytes for AES-128-GCM, AES-256-GCM + * and ChaCha20-Poly1305 in QUIC). + * + * Returns [dst] for fluent use. This is the allocation-free shape that lets + * a long-lived call site (a per-direction packet-protection slot, a + * writer hot loop, …) reuse the same nonce buffer across thousands of + * packets. RFC 9001 §5.3: nonce = static_iv XOR (packet_number padded to + * nonce length, big-endian). + */ +fun aeadNonceInto( + staticIv: ByteArray, + packetNumber: Long, + dst: ByteArray, ): ByteArray { - val nonce = staticIv.copyOf() - val len = nonce.size + require(dst.size == staticIv.size) { "nonce scratch must match static IV size" } + staticIv.copyInto(dst) + val len = dst.size for (i in 0 until 8) { - nonce[len - 1 - i] = (nonce[len - 1 - i].toInt() xor ((packetNumber ushr (i * 8)).toInt() and 0xFF)).toByte() + dst[len - 1 - i] = (dst[len - 1 - i].toInt() xor ((packetNumber ushr (i * 8)).toInt() and 0xFF)).toByte() } - return nonce + return dst } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt index 1796768134..6b74701703 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt @@ -29,10 +29,28 @@ package com.vitorpamplona.quic.crypto * are the nonce; ChaCha20-encrypt 5 zero bytes; that's the mask. */ sealed class HeaderProtection { + /** + * Compute the HP mask from a 16-byte standalone sample buffer. Retained + * for callers (mostly tests) that already have a heap-allocated sample + * blob — the production hot path uses [maskAt] to avoid the slice. + */ abstract fun mask( hpKey: ByteArray, sample: ByteArray, ): ByteArray + + /** + * Compute the HP mask from a 16-byte sample window inside [src] starting + * at [srcOffset]. Avoids the `copyOfRange` of the sample on every + * outbound and inbound packet (round-5 #P1). Returns a freshly allocated + * 5-byte mask — the per-packet allocation budget drops from + * `16 (sample) + 16 (cipher output) + 5 (mask)` to `16 + 5` for AES-ECB. + */ + abstract fun maskAt( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, + ): ByteArray } /** AES-128-ECB header protection. Implemented via the platform AES helper. */ @@ -48,6 +66,21 @@ class AesEcbHeaderProtection( val out = aesEncryptOneBlock.encrypt(hpKey, sample) return out.copyOfRange(0, 5) } + + override fun maskAt( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, + ): ByteArray { + require(srcOffset >= 0 && srcOffset + 16 <= src.size) { "HP sample window out of range" } + require(hpKey.size in setOf(16, 24, 32)) { "AES-ECB key must be 16/24/32 bytes" } + val scratch = ByteArray(16) + aesEncryptOneBlock.encryptInto(hpKey, src, srcOffset, scratch, 0) + // Mask is the first 5 bytes per RFC 9001 §5.4.3. + val mask = ByteArray(5) + scratch.copyInto(mask, 0, 0, 5) + return mask + } } /** ChaCha20-based header protection per RFC 9001 §5.4.4. */ @@ -59,23 +92,60 @@ class ChaCha20HeaderProtection( sample: ByteArray, ): ByteArray { require(sample.size == 16) { "ChaCha20 HP sample must be 16 bytes" } + return maskAt(hpKey, sample, 0) + } + + override fun maskAt( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, + ): ByteArray { + require(srcOffset >= 0 && srcOffset + 16 <= src.size) { "ChaCha20 HP sample window out of range" } require(hpKey.size == 32) { "ChaCha20 HP key must be 32 bytes" } val counter = - ((sample[0].toInt() and 0xFF)) or - ((sample[1].toInt() and 0xFF) shl 8) or - ((sample[2].toInt() and 0xFF) shl 16) or - ((sample[3].toInt() and 0xFF) shl 24) - val nonce = sample.copyOfRange(4, 16) + ((src[srcOffset].toInt() and 0xFF)) or + ((src[srcOffset + 1].toInt() and 0xFF) shl 8) or + ((src[srcOffset + 2].toInt() and 0xFF) shl 16) or + ((src[srcOffset + 3].toInt() and 0xFF) shl 24) + // The nonce slice is unavoidable as long as we delegate to Quartz's + // `ChaCha20Core.chaCha20Xor(plaintext, key, nonce, counter)` which + // takes a standalone nonce ByteArray. A future pass could thread + // src + offset all the way down. + val nonce = src.copyOfRange(srcOffset + 4, srcOffset + 16) return chacha20Encrypt.encrypt(hpKey, nonce, counter, ByteArray(5)) } } -/** SPI for one-block AES encryption (provided by jvmAndroid via JCA). */ -fun interface AesOneBlockEncrypt { +/** + * SPI for one-block AES-ECB encryption (provided by jvmAndroid via JCA). + * Two shapes: + * + * - [encrypt] — returns a freshly allocated 16-byte ciphertext. Retained + * for callers that don't have a destination buffer at hand. + * - [encryptInto] — fills caller-owned [dst] starting at [dstOffset] with + * the AES-ECB encryption of `src[srcOffset..srcOffset+16)`. The hot QUIC + * header-protection path uses this overload so the per-packet allocation + * of both the sample slice AND the cipher output goes away (round-5 #P1). + */ +interface AesOneBlockEncrypt { fun encrypt( key: ByteArray, block: ByteArray, ): ByteArray + + fun encryptInto( + key: ByteArray, + src: ByteArray, + srcOffset: Int, + dst: ByteArray, + dstOffset: Int, + ) { + // Default impl: copy the 16-byte sample out and call the existing + // allocation-shaped overload. Concrete platform impls override + // this with a zero-allocation Cipher.doFinal range overload. + val ct = encrypt(key, src.copyOfRange(srcOffset, srcOffset + 16)) + ct.copyInto(dst, dstOffset, 0, 16) + } } /** SPI for ChaCha20 keystream encryption with explicit counter. */ 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 63b4ec5a09..f4930ffb91 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt @@ -133,10 +133,11 @@ object LongHeaderPacket { ) // Apply header protection. Sample is 16 bytes starting 4 bytes after pnOffset. + // Round-5 #P1: read the sample window directly from `packet` instead + // of allocating a 16-byte copyOfRange per outbound long-header packet. val sampleStart = pnOffset + 4 require(sampleStart + 16 <= packet.size) { "packet too short for HP sample" } - val sample = packet.copyOfRange(sampleStart, sampleStart + 16) - val mask = hp.mask(hpKey, sample) + val mask = hp.maskAt(hpKey, packet, sampleStart) applyHeaderProtectionMask(packet, firstByteOffset, pnOffset, pnLen, mask) return packet @@ -196,8 +197,7 @@ object LongHeaderPacket { // Sample for HP starts at pnOffset + 4. val sampleStart = pnOffset + 4 if (sampleStart + 16 > bytes.size) return null - val sample = bytes.copyOfRange(sampleStart, sampleStart + 16) - val mask = hp.mask(hpKey, sample) + val mask = hp.maskAt(hpKey, bytes, sampleStart) // Make a private copy of the packet so we can mutate the header in place. val packetEnd = pnOffset + length 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 21b8a6f428..941deed5ce 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt @@ -98,8 +98,9 @@ object ShortHeaderPacket { val sampleStart = pnOffset + 4 require(sampleStart + 16 <= packet.size) { "packet too short for HP sample" } - val sample = packet.copyOfRange(sampleStart, sampleStart + 16) - val mask = hp.mask(hpKey, sample) + // Round-5 #P1: read the sample window directly from `packet` — pre- + // fix this allocated a fresh 16-byte ByteArray on every outbound. + val mask = hp.maskAt(hpKey, packet, sampleStart) applyHeaderProtectionMask(packet, firstByteOffset, pnOffset, pnLen, mask) return packet } @@ -138,8 +139,7 @@ object ShortHeaderPacket { val pnOffset = offset + 1 + dcidLen val sampleStart = pnOffset + 4 if (sampleStart + 16 > bytes.size) return null - val sample = bytes.copyOfRange(sampleStart, sampleStart + 16) - val mask = hp.mask(hpKey, sample) + val mask = hp.maskAt(hpKey, bytes, sampleStart) val unprotectedFirst = first xor (mask[0].toInt() and 0x1F) return Peek( keyPhase = (unprotectedFirst and 0x04) != 0, @@ -173,8 +173,7 @@ object ShortHeaderPacket { val pnOffset = offset + 1 + dcidLen val sampleStart = pnOffset + 4 if (sampleStart + 16 > bytes.size) return null - val sample = bytes.copyOfRange(sampleStart, sampleStart + 16) - val mask = hp.mask(hpKey, sample) + val mask = hp.maskAt(hpKey, bytes, sampleStart) val packetEnd = bytes.size val packet = bytes.copyOfRange(offset, packetEnd) val localPnOffset = pnOffset - offset diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsExtension.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsExtension.kt index 2926bccbe5..71c76fb2cd 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsExtension.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsExtension.kt @@ -103,16 +103,20 @@ fun encodeSupportedGroupsX25519(): ByteArray { return w.toByteArray() } -/** Build the `signature_algorithms` extension covering ECDSA-P256, RSA-PSS, Ed25519. */ +/** Build the `signature_algorithms` extension covering ECDSA-P256/P384, RSA-PSS, Ed25519. */ fun encodeSignatureAlgorithms(): ByteArray { val w = QuicWriter() w.withUint16Length { + // RFC 8446 §4.2.3 forbids rsa_pkcs1_* in CertificateVerify (only + // permitted as a server-side cert chain hint). The JdkCertificateValidator + // already rejects it, so advertising rsa_pkcs1_sha256 here lied to the + // peer about what we accept and risked a 0x0401 selection that we'd + // then reject with an alert. Stick to RSA-PSS / ECDSA / Ed25519. writeUint16(TlsConstants.SIG_ECDSA_SECP256R1_SHA256) writeUint16(TlsConstants.SIG_RSA_PSS_RSAE_SHA256) writeUint16(TlsConstants.SIG_RSA_PSS_RSAE_SHA384) writeUint16(TlsConstants.SIG_RSA_PSS_RSAE_SHA512) writeUint16(TlsConstants.SIG_ED25519) - writeUint16(TlsConstants.SIG_RSA_PKCS1_SHA256) writeUint16(TlsConstants.SIG_ECDSA_SECP384R1_SHA384) } return w.toByteArray() diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt index 8d148d1ea2..c3019bd0ab 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt @@ -39,12 +39,34 @@ private val aesEcbCipher: ThreadLocal = ThreadLocal.withInitial { Cipher.getInstance("AES/ECB/NoPadding") } actual val PlatformAesOneBlock: AesOneBlockEncrypt = - AesOneBlockEncrypt { key, block -> - // .get() is non-null because withInitial supplies a Cipher, but - // Kotlin sees the Java return type as platform-nullable. - val cipher = aesEcbCipher.get()!! - cipher.init(Cipher.ENCRYPT_MODE, SecretKeySpec(key, "AES")) - cipher.doFinal(block) + object : AesOneBlockEncrypt { + override fun encrypt( + key: ByteArray, + block: ByteArray, + ): ByteArray { + // .get() is non-null because withInitial supplies a Cipher, but + // Kotlin sees the Java return type as platform-nullable. + val cipher = aesEcbCipher.get()!! + cipher.init(Cipher.ENCRYPT_MODE, SecretKeySpec(key, "AES")) + return cipher.doFinal(block) + } + + override fun encryptInto( + key: ByteArray, + src: ByteArray, + srcOffset: Int, + dst: ByteArray, + dstOffset: Int, + ) { + // JCA's range-overload writes directly into [dst] starting at + // [dstOffset] — skips both the [block] copyOfRange the caller + // would have done AND the freshly-allocated 16-byte ciphertext + // the no-offset doFinal returns. Per-packet HP cost on the hot + // path drops from two 16-byte ByteArrays to zero. + val cipher = aesEcbCipher.get()!! + cipher.init(Cipher.ENCRYPT_MODE, SecretKeySpec(key, "AES")) + cipher.doFinal(src, srcOffset, 16, dst, dstOffset) + } } /** diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt index dfc12b8866..332d65d5c1 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt @@ -26,6 +26,7 @@ import java.lang.reflect.InvocationTargetException import java.net.IDN import java.net.InetAddress import java.security.KeyStore +import java.security.NoSuchAlgorithmException import java.security.Signature import java.security.cert.CertificateFactory import java.security.cert.X509Certificate @@ -136,23 +137,48 @@ class JdkCertificateValidator( private fun jcaSignatureFor(algorithm: Int): Signature = when (algorithm) { - TlsConstants.SIG_ECDSA_SECP256R1_SHA256 -> Signature.getInstance("SHA256withECDSA") + TlsConstants.SIG_ECDSA_SECP256R1_SHA256 -> { + Signature.getInstance("SHA256withECDSA") + } - TlsConstants.SIG_ECDSA_SECP384R1_SHA384 -> Signature.getInstance("SHA384withECDSA") + TlsConstants.SIG_ECDSA_SECP384R1_SHA384 -> { + Signature.getInstance("SHA384withECDSA") + } - TlsConstants.SIG_RSA_PSS_RSAE_SHA256 -> rsaPss("SHA-256", 32) + TlsConstants.SIG_RSA_PSS_RSAE_SHA256 -> { + rsaPss("SHA-256", 32) + } - TlsConstants.SIG_RSA_PSS_RSAE_SHA384 -> rsaPss("SHA-384", 48) + TlsConstants.SIG_RSA_PSS_RSAE_SHA384 -> { + rsaPss("SHA-384", 48) + } - TlsConstants.SIG_RSA_PSS_RSAE_SHA512 -> rsaPss("SHA-512", 64) + TlsConstants.SIG_RSA_PSS_RSAE_SHA512 -> { + rsaPss("SHA-512", 64) + } - TlsConstants.SIG_ED25519 -> Signature.getInstance("Ed25519") + TlsConstants.SIG_ED25519 -> { + try { + // JCA "Ed25519" was added to Android Conscrypt in API 33. + // On API 26–32 (our minSdk floor) this throws — surface + // it as a clean QuicCodecException so the read loop maps + // to CONNECTION_CLOSE rather than crashing the parser. + Signature.getInstance("Ed25519") + } catch (_: NoSuchAlgorithmException) { + throw QuicCodecException( + "Ed25519 not supported on this platform " + + "(requires Android API 33+ or a JDK with the EdDSA provider)", + ) + } + } // Audit-4 #2: rsa_pkcs1_* schemes are forbidden in CertificateVerify // by RFC 8446 §4.2.3 (only allowed in CertificateRequest for // legacy compat). Accepting them allowed a server to sign with // weaker PKCS#1 v1.5 instead of RSA-PSS. - else -> throw QuicCodecException("unsupported signature algorithm 0x${algorithm.toString(16)}") + else -> { + throw QuicCodecException("unsupported signature algorithm 0x${algorithm.toString(16)}") + } } private fun rsaPss( diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt index 1ed38e9684..5fe19adaf0 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt @@ -20,12 +20,17 @@ */ package com.vitorpamplona.quic.tls +import java.io.ByteArrayOutputStream import java.security.MessageDigest /** * JCA-backed incremental SHA-256. `MessageDigest.clone()` is supported by all - * stock JDK SHA-256 providers and produces an independent digest object — we - * use that to take a snapshot without disturbing the running state. + * stock JDK SHA-256 providers AND by Android's Conscrypt + * `OpenSSLMessageDigestJDK` on shipping releases — but a handful of API + * 26–28 builds have been reported to throw `CloneNotSupportedException` + * from the native digest. Detect that on first snapshot and fall back to + * one-shot SHA-256 over an accumulated byte buffer; we keep accumulating + * regardless so the fallback always has the complete transcript to hash. * * Single-thread per instance: the TlsClient state machine is the sole caller, * driven by the QUIC connection's lock, so no synchronization is needed. @@ -33,15 +38,36 @@ import java.security.MessageDigest actual class TlsRunningSha256 actual constructor() { private val digest: MessageDigest = MessageDigest.getInstance("SHA-256") + // Parallel byte accumulator — small (a few KB for a TLS transcript), + // and only consulted on the cloneable-digest fallback path. Keeping it + // populated unconditionally costs one `ByteArrayOutputStream.write` per + // [update] but avoids a "first snapshot fails, we have no history" + // failure mode on the broken-clone Android builds. + private val accumulator = ByteArrayOutputStream(512) + private var cloneable = true + actual fun update(bytes: ByteArray) { digest.update(bytes) + accumulator.write(bytes) } actual fun snapshot(): ByteArray { - // Cloning the digest is the only way to read the current hash without - // ending the running state — `digest.digest()` finalizes and resets, - // which would silently corrupt subsequent updates. - val clone = digest.clone() as MessageDigest - return clone.digest() + if (cloneable) { + try { + // Cloning the digest is the cheap path — independent digest + // object with the current internal state, no consume. + val clone = digest.clone() as MessageDigest + return clone.digest() + } catch (_: CloneNotSupportedException) { + // Latch the fallback so we don't pay the JCA throw on every + // subsequent snapshot. + cloneable = false + } + } + // Fallback: one-shot SHA-256 over the accumulated transcript bytes. + // `MessageDigest.getInstance("SHA-256")` is mandated on every JCA + // provider — only the `.clone()` capability varies — so this is + // guaranteed to work on the same device that rejected the clone. + return MessageDigest.getInstance("SHA-256").digest(accumulator.toByteArray()) } } From 09b28b8d789f70ca7b67f0904e2debd47c611bbe Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 19:19:05 +0000 Subject: [PATCH 2/3] 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 051d8f5398..628a51b320 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 55f72a93b1..f57101547a 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 e4547e4fb3..97b9cd59ee 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 9c62c40a88..aa60312335 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 f4930ffb91..6929997057 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 941deed5ce..655b0e7898 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.) From 8a3bd52631e0f9aa83496315b776e6030df5abdc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 19:27:47 +0000 Subject: [PATCH 3/3] fix(quic): finish P1 + A2 audit follow-ups (PR #2873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 (header protection — eliminate remaining HP allocations): Add HeaderProtection.maskInto(hpKey, src, srcOffset, scratch16, dstMask) that writes the 5-byte mask into a caller-owned buffer using a caller-owned 16-byte AES scratch. Add `hpScratch` (16 bytes) and `hpMask` (5 bytes) to PacketProtection — same per-direction single-threaded analysis as nonceScratch from the prior commit. Thread both buffers through Short/LongHeaderPacket build / parseAndDecrypt / peekKeyPhase as optional parameters paired with nonceScratch; production call sites in QuicConnectionWriter / QuicConnectionParser pass `proto.hpScratch` + `proto.hpMask`. With both this commit and 09b28b8d the AES-128-GCM hot path now allocates ZERO bytes for HP + nonce per packet (down from 16-byte sample slice + 16-byte AES output + 5-byte mask + 12-byte nonce = 49 bytes/pkt). ChaCha20HeaderProtection.maskInto routes through the same shape but still allocates a fresh 5-byte ciphertext + 12-byte nonce slice internally — Quartz's `ChaCha20Core.chaCha20Xor` SPI takes a standalone nonce ByteArray. Documented as a future cleanup; AES is the dominant path in production. A2 (TlsRunningSha256 — lazy fallback accumulator): Probe `digest.clone()` ONCE at construction. Conscrypt's clone support is a build-time property (native bridge presence), not state- dependent, so a single probe is a reliable signal. Devices where the probe succeeds skip the byte accumulator entirely (zero overhead); devices where it fails get the byte-accumulator path from the very first update so the first snapshot already has the complete transcript. Replaces the previous "always accumulate" shape that wasted a few KB per handshake on every device, including the overwhelming majority that support clone. All quic JVM unit tests pass; spotless applied. https://claude.ai/code/session_01EBHtGLy5o7FUR5qfcpHUUx --- .../quic/connection/EncryptionLevel.kt | 17 ++++ .../quic/connection/QuicConnectionParser.kt | 10 +++ .../quic/connection/QuicConnectionWriter.kt | 10 +++ .../quic/crypto/HeaderProtection.kt | 78 +++++++++++++++---- .../quic/packet/LongHeaderPacket.kt | 33 ++++++-- .../quic/packet/ShortHeaderPacket.kt | 48 +++++++++--- .../quic/tls/TlsRunningSha256.kt | 60 ++++++++------ 7 files changed, 203 insertions(+), 53 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 628a51b320..af087ef015 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/EncryptionLevel.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/EncryptionLevel.kt @@ -46,6 +46,23 @@ class PacketProtection( * invocation. */ val nonceScratch: ByteArray = ByteArray(iv.size) + + /** + * Per-direction header-protection scratch — 16 bytes of AES-ECB output + * for [hp.maskInto][com.vitorpamplona.quic.crypto.HeaderProtection.maskInto]. + * Reused per packet so the AES output no longer allocates (round-5 #P1). + * Same single-direction thread-safety rationale as [nonceScratch]. + */ + val hpScratch: ByteArray = ByteArray(16) + + /** + * Per-direction header-protection mask buffer — 5 bytes per RFC 9001 + * §5.4.3. Filled by [hp.maskInto][com.vitorpamplona.quic.crypto.HeaderProtection.maskInto] + * and immediately consumed by [com.vitorpamplona.quic.crypto.applyHeaderProtectionMask] + * (write path) or by inline first-byte / packet-number unmasking + * (parse path). Consume before the next mask call on this instance. + */ + val hpMask: ByteArray = ByteArray(5) } /** All four encryption levels we ever see in a QUIC client connection. */ 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 97b9cd59ee..e424c274d5 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -321,6 +321,8 @@ private fun feedLongHeaderPacket( hpKey = proto.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, nonceScratch = proto.nonceScratch, + hpScratch = proto.hpScratch, + hpMask = proto.hpMask, ) if (parsed == null) { conn.qlogObserver.onPacketDropped( @@ -393,6 +395,8 @@ private fun feedShortHeaderPacket( dcidLen = conn.sourceConnectionId.length, hp = live.hp, hpKey = live.hpKey, + hpScratch = live.hpScratch, + hpMask = live.hpMask, ) if (peek == null) { conn.qlogObserver.onPacketDropped( @@ -444,6 +448,8 @@ private fun feedShortHeaderPacket( hpKey = live.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, nonceScratch = live.nonceScratch, + hpScratch = live.hpScratch, + hpMask = live.hpMask, ) rotateOnSuccess = null } else { @@ -463,6 +469,8 @@ private fun feedShortHeaderPacket( hpKey = prev.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, nonceScratch = prev.nonceScratch, + hpScratch = prev.hpScratch, + hpMask = prev.hpMask, ) } if (priorTry != null) { @@ -493,6 +501,8 @@ private fun feedShortHeaderPacket( hpKey = nextPhase.hpKey, largestReceivedInSpace = state.pnSpace.largestReceived, nonceScratch = nextPhase.nonceScratch, + hpScratch = nextPhase.hpScratch, + hpMask = nextPhase.hpMask, ) 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 aa60312335..35631dff45 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -360,6 +360,8 @@ private fun buildBestLevelPacket( proto.hpKey, largestAckedInSpace = -1L, nonceScratch = proto.nonceScratch, + hpScratch = proto.hpScratch, + hpMask = proto.hpMask, ) emitQlogSent(conn, EncryptionLevel.APPLICATION, pn, built.size, frames) return built @@ -407,6 +409,8 @@ private fun buildLongHeaderPacket( proto.hpKey, largestAckedInSpace = -1L, nonceScratch = proto.nonceScratch, + hpScratch = proto.hpScratch, + hpMask = proto.hpMask, ) emitQlogSent(conn, level, pn, built.size, frames) return built @@ -562,6 +566,8 @@ private fun buildLongHeaderFromFrames( proto.hpKey, largestAckedInSpace = -1L, nonceScratch = proto.nonceScratch, + hpScratch = proto.hpScratch, + hpMask = proto.hpMask, ) // Step E: retain the packet for RFC 9002 retransmit. Initial / // Handshake packets carry CRYPTO frames; loss detection runs at @@ -916,6 +922,8 @@ private fun buildApplicationPacket( proto.hpKey, largestAckedInSpace = -1L, nonceScratch = proto.nonceScratch, + hpScratch = proto.hpScratch, + hpMask = proto.hpMask, ) } else { // 0-RTT — long header type=0x01. Same Application packet @@ -938,6 +946,8 @@ private fun buildApplicationPacket( proto.hpKey, largestAckedInSpace = -1L, nonceScratch = proto.nonceScratch, + hpScratch = proto.hpScratch, + hpMask = proto.hpMask, ) } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt index 6b74701703..f038bb7640 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/HeaderProtection.kt @@ -32,7 +32,8 @@ sealed class HeaderProtection { /** * Compute the HP mask from a 16-byte standalone sample buffer. Retained * for callers (mostly tests) that already have a heap-allocated sample - * blob — the production hot path uses [maskAt] to avoid the slice. + * blob — the production hot path uses [maskInto] to avoid every per- + * packet allocation. */ abstract fun mask( hpKey: ByteArray, @@ -42,15 +43,36 @@ sealed class HeaderProtection { /** * Compute the HP mask from a 16-byte sample window inside [src] starting * at [srcOffset]. Avoids the `copyOfRange` of the sample on every - * outbound and inbound packet (round-5 #P1). Returns a freshly allocated - * 5-byte mask — the per-packet allocation budget drops from - * `16 (sample) + 16 (cipher output) + 5 (mask)` to `16 + 5` for AES-ECB. + * outbound and inbound packet (round-5 #P1) but still allocates a + * fresh 5-byte mask + an internal 16-byte AES scratch on each call. + * Use [maskInto] when caller-owned scratch + mask buffers are + * available (the production writer/parser path). */ abstract fun maskAt( hpKey: ByteArray, src: ByteArray, srcOffset: Int, ): ByteArray + + /** + * Allocation-free HP mask: write the 5 mask bytes into [dstMask], using + * [scratch16] (caller-owned 16-byte buffer) for the AES-ECB output + * intermediate. Returns [dstMask] for fluent use. + * + * Both buffers are MUTATED — callers must consume the mask before the + * next [maskInto] call on the same buffers. The hot QUIC writer/parser + * path in `Short/LongHeaderPacket.build` + `parseAndDecrypt` keeps a + * per-PacketProtection scratch + mask pair and consumes the mask + * immediately during [com.vitorpamplona.quic.crypto.applyHeaderProtectionMask], + * so the lifetime is trivially safe. + */ + abstract fun maskInto( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, + scratch16: ByteArray, + dstMask: ByteArray, + ): ByteArray } /** AES-128-ECB header protection. Implemented via the platform AES helper. */ @@ -71,15 +93,23 @@ class AesEcbHeaderProtection( hpKey: ByteArray, src: ByteArray, srcOffset: Int, + ): ByteArray = maskInto(hpKey, src, srcOffset, ByteArray(16), ByteArray(5)) + + override fun maskInto( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, + scratch16: ByteArray, + dstMask: ByteArray, ): ByteArray { require(srcOffset >= 0 && srcOffset + 16 <= src.size) { "HP sample window out of range" } require(hpKey.size in setOf(16, 24, 32)) { "AES-ECB key must be 16/24/32 bytes" } - val scratch = ByteArray(16) - aesEncryptOneBlock.encryptInto(hpKey, src, srcOffset, scratch, 0) + require(scratch16.size == 16) { "AES scratch must be 16 bytes" } + require(dstMask.size >= 5) { "HP mask buffer must be at least 5 bytes" } + aesEncryptOneBlock.encryptInto(hpKey, src, srcOffset, scratch16, 0) // Mask is the first 5 bytes per RFC 9001 §5.4.3. - val mask = ByteArray(5) - scratch.copyInto(mask, 0, 0, 5) - return mask + scratch16.copyInto(dstMask, 0, 0, 5) + return dstMask } } @@ -99,6 +129,32 @@ class ChaCha20HeaderProtection( hpKey: ByteArray, src: ByteArray, srcOffset: Int, + ): ByteArray = chacha20Mask(hpKey, src, srcOffset) + + override fun maskInto( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, + scratch16: ByteArray, + dstMask: ByteArray, + ): ByteArray { + require(dstMask.size >= 5) { "HP mask buffer must be at least 5 bytes" } + // ChaCha20 HP unavoidably allocates a fresh 5-byte ciphertext via the + // [ChaCha20BlockEncrypt] SPI, plus a 12-byte nonce slice (Quartz's + // `ChaCha20Core.chaCha20Xor` takes a standalone nonce). Copy the + // result into the caller's [dstMask] so call-site shape matches the + // AES-ECB path; a future pass could push src+offset through the + // ChaCha20 SPI to fully eliminate the slice. [scratch16] is unused + // here. + val mask = chacha20Mask(hpKey, src, srcOffset) + mask.copyInto(dstMask, 0, 0, 5) + return dstMask + } + + private fun chacha20Mask( + hpKey: ByteArray, + src: ByteArray, + srcOffset: Int, ): ByteArray { require(srcOffset >= 0 && srcOffset + 16 <= src.size) { "ChaCha20 HP sample window out of range" } require(hpKey.size == 32) { "ChaCha20 HP key must be 32 bytes" } @@ -107,10 +163,6 @@ class ChaCha20HeaderProtection( ((src[srcOffset + 1].toInt() and 0xFF) shl 8) or ((src[srcOffset + 2].toInt() and 0xFF) shl 16) or ((src[srcOffset + 3].toInt() and 0xFF) shl 24) - // The nonce slice is unavoidable as long as we delegate to Quartz's - // `ChaCha20Core.chaCha20Xor(plaintext, key, nonce, counter)` which - // takes a standalone nonce ByteArray. A future pass could thread - // src + offset all the way down. val nonce = src.copyOfRange(srcOffset + 4, srcOffset + 16) return chacha20Encrypt.encrypt(hpKey, nonce, counter, ByteArray(5)) } 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 6929997057..a055cb33c8 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt @@ -73,9 +73,12 @@ 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). + // Round-5 #P2 + #P1: optional caller-owned scratch buffers. When + // provided the build path allocates nothing for nonce / HP work. + // Default = allocate fresh on each call (preserves tests). nonceScratch: ByteArray? = null, + hpScratch: ByteArray? = null, + hpMask: ByteArray? = null, ): ByteArray { val pnLen = com.vitorpamplona.quic.connection.PacketNumberSpaceState.encodeLength( @@ -142,11 +145,17 @@ object LongHeaderPacket { ) // Apply header protection. Sample is 16 bytes starting 4 bytes after pnOffset. - // Round-5 #P1: read the sample window directly from `packet` instead - // of allocating a 16-byte copyOfRange per outbound long-header packet. + // Round-5 #P1: read the sample window directly from `packet`. When + // caller-owned hp scratch + mask are provided the HP path allocates + // zero bytes per packet; otherwise allocate fresh (test path). val sampleStart = pnOffset + 4 require(sampleStart + 16 <= packet.size) { "packet too short for HP sample" } - val mask = hp.maskAt(hpKey, packet, sampleStart) + val mask = + if (hpScratch != null && hpMask != null) { + hp.maskInto(hpKey, packet, sampleStart, hpScratch, hpMask) + } else { + hp.maskAt(hpKey, packet, sampleStart) + } applyHeaderProtectionMask(packet, firstByteOffset, pnOffset, pnLen, mask) return packet @@ -169,9 +178,12 @@ 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). + // Round-5 #P2 + #P1: optional caller-owned scratch buffers. When + // provided the parse path allocates nothing for nonce / HP work. + // Default = allocate fresh on each call (preserves tests). nonceScratch: ByteArray? = null, + hpScratch: ByteArray? = null, + hpMask: ByteArray? = null, ): ParseResult? { val packetStart = offset val r = QuicReader(bytes, offset) @@ -209,7 +221,12 @@ object LongHeaderPacket { // Sample for HP starts at pnOffset + 4. val sampleStart = pnOffset + 4 if (sampleStart + 16 > bytes.size) return null - val mask = hp.maskAt(hpKey, bytes, sampleStart) + val mask = + if (hpScratch != null && hpMask != null) { + hp.maskInto(hpKey, bytes, sampleStart, hpScratch, hpMask) + } else { + hp.maskAt(hpKey, bytes, sampleStart) + } // Make a private copy of the packet so we can mutate the header in place. val packetEnd = pnOffset + length 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 655b0e7898..250b80f093 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt @@ -54,9 +54,16 @@ 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). + // Round-5 #P2: optional 12-byte AEAD-nonce scratch the writer can + // reuse across packets. Default = allocate fresh (preserves tests). nonceScratch: ByteArray? = null, + // Round-5 #P1: optional 16-byte AES-ECB scratch + 5-byte mask + // buffer for header protection. When BOTH are provided the call + // site reuses them across packets and the HP path allocates + // nothing per packet. Either-null (or both-null) falls back to + // the maskAt path which allocates fresh (preserves tests). + hpScratch: ByteArray? = null, + hpMask: ByteArray? = null, ): ByteArray { val pnLen = PacketNumberSpaceState.encodeLength(plain.packetNumber, largestAckedInSpace) require(pnLen in 1..4) @@ -107,9 +114,15 @@ object ShortHeaderPacket { val sampleStart = pnOffset + 4 require(sampleStart + 16 <= packet.size) { "packet too short for HP sample" } - // Round-5 #P1: read the sample window directly from `packet` — pre- - // fix this allocated a fresh 16-byte ByteArray on every outbound. - val mask = hp.maskAt(hpKey, packet, sampleStart) + // Round-5 #P1: when caller-owned HP scratch + mask buffers are + // provided, write the mask in-place — zero per-packet allocation. + // Otherwise allocate fresh (test path). + val mask = + if (hpScratch != null && hpMask != null) { + hp.maskInto(hpKey, packet, sampleStart, hpScratch, hpMask) + } else { + hp.maskAt(hpKey, packet, sampleStart) + } applyHeaderProtectionMask(packet, firstByteOffset, pnOffset, pnLen, mask) return packet } @@ -136,6 +149,10 @@ object ShortHeaderPacket { dcidLen: Int, hp: HeaderProtection, hpKey: ByteArray, + // Round-5 #P1: optional caller-owned HP scratch + mask. Both null + // = allocate fresh (test path); both non-null = zero-alloc. + hpScratch: ByteArray? = null, + hpMask: ByteArray? = null, ): Peek? { if (offset >= bytes.size) return null val first = bytes[offset].toInt() and 0xFF @@ -148,7 +165,12 @@ object ShortHeaderPacket { val pnOffset = offset + 1 + dcidLen val sampleStart = pnOffset + 4 if (sampleStart + 16 > bytes.size) return null - val mask = hp.maskAt(hpKey, bytes, sampleStart) + val mask = + if (hpScratch != null && hpMask != null) { + hp.maskInto(hpKey, bytes, sampleStart, hpScratch, hpMask) + } else { + hp.maskAt(hpKey, bytes, sampleStart) + } val unprotectedFirst = first xor (mask[0].toInt() and 0x1F) return Peek( keyPhase = (unprotectedFirst and 0x04) != 0, @@ -172,9 +194,12 @@ 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). + // Round-5 #P2 + #P1: optional caller-owned scratch buffers. When + // provided the parse path allocates nothing for nonce / HP work. + // Default = allocate fresh on each call (preserves tests). nonceScratch: ByteArray? = null, + hpScratch: ByteArray? = null, + hpMask: ByteArray? = null, ): ParseResult? { if (offset >= bytes.size) return null val first = bytes[offset].toInt() and 0xFF @@ -185,7 +210,12 @@ object ShortHeaderPacket { val pnOffset = offset + 1 + dcidLen val sampleStart = pnOffset + 4 if (sampleStart + 16 > bytes.size) return null - val mask = hp.maskAt(hpKey, bytes, sampleStart) + val mask = + if (hpScratch != null && hpMask != null) { + hp.maskInto(hpKey, bytes, sampleStart, hpScratch, hpMask) + } else { + hp.maskAt(hpKey, bytes, sampleStart) + } val packetEnd = bytes.size val packet = bytes.copyOfRange(offset, packetEnd) val localPnOffset = pnOffset - offset diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt index 5fe19adaf0..e9820aea15 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/TlsRunningSha256.kt @@ -28,9 +28,14 @@ import java.security.MessageDigest * stock JDK SHA-256 providers AND by Android's Conscrypt * `OpenSSLMessageDigestJDK` on shipping releases — but a handful of API * 26–28 builds have been reported to throw `CloneNotSupportedException` - * from the native digest. Detect that on first snapshot and fall back to - * one-shot SHA-256 over an accumulated byte buffer; we keep accumulating - * regardless so the fallback always has the complete transcript to hash. + * from the native digest. + * + * We probe `clone()` ONCE at construction: if it works the JCA-clone path + * runs for the lifetime of the instance with zero side-buffer overhead; + * if it doesn't we switch to byte-accumulator-then-one-shot-SHA-256 mode + * from the start, so the very first snapshot already has the complete + * transcript. Working devices (the overwhelming majority) pay nothing + * for the fallback. * * Single-thread per instance: the TlsClient state machine is the sole caller, * driven by the QUIC connection's lock, so no synchronization is needed. @@ -38,36 +43,45 @@ import java.security.MessageDigest actual class TlsRunningSha256 actual constructor() { private val digest: MessageDigest = MessageDigest.getInstance("SHA-256") - // Parallel byte accumulator — small (a few KB for a TLS transcript), - // and only consulted on the cloneable-digest fallback path. Keeping it - // populated unconditionally costs one `ByteArrayOutputStream.write` per - // [update] but avoids a "first snapshot fails, we have no history" - // failure mode on the broken-clone Android builds. - private val accumulator = ByteArrayOutputStream(512) - private var cloneable = true + /** + * Result of the one-time `digest.clone()` probe. Conscrypt's clone + * support is a property of the build (native bridge presence), not of + * the digest's internal state, so a single probe is a reliable signal + * — repeated clones on the same instance behave identically. + */ + private val cloneable: Boolean = + try { + digest.clone() + true + } catch (_: CloneNotSupportedException) { + false + } + + /** + * Byte accumulator allocated only on the broken-clone fallback path. + * Holds every byte fed to [update] so [snapshot] can one-shot + * SHA-256 the full transcript. On clone-capable devices this stays + * `null` and [update] never touches it. + */ + private val accumulator: ByteArrayOutputStream? = + if (cloneable) null else ByteArrayOutputStream(512) actual fun update(bytes: ByteArray) { digest.update(bytes) - accumulator.write(bytes) + accumulator?.write(bytes) } actual fun snapshot(): ByteArray { if (cloneable) { - try { - // Cloning the digest is the cheap path — independent digest - // object with the current internal state, no consume. - val clone = digest.clone() as MessageDigest - return clone.digest() - } catch (_: CloneNotSupportedException) { - // Latch the fallback so we don't pay the JCA throw on every - // subsequent snapshot. - cloneable = false - } + // Cheap path — independent digest object with current internal + // state, no consume. + val clone = digest.clone() as MessageDigest + return clone.digest() } // Fallback: one-shot SHA-256 over the accumulated transcript bytes. // `MessageDigest.getInstance("SHA-256")` is mandated on every JCA - // provider — only the `.clone()` capability varies — so this is + // provider — only `.clone()` capability varies — so this is // guaranteed to work on the same device that rejected the clone. - return MessageDigest.getInstance("SHA-256").digest(accumulator.toByteArray()) + return MessageDigest.getInstance("SHA-256").digest(accumulator!!.toByteArray()) } }