From fbacef1f2ae2eecae5a4789b33dbd28d3d28df58 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 18:55:52 +0000 Subject: [PATCH] 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 93b3f9f68..55f72a93b 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 1b960159d..e4547e4fb 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 4ed82810a..9c62c40a8 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 2a4feec53..a07bf62ac 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 179676813..6b7470170 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 63b4ec5a0..f4930ffb9 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 21b8a6f42..941deed5c 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 2926bccbe..71c76fb2c 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 8d148d1ea..c3019bd0a 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 dfc12b886..332d65d5c 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 1ed38e968..5fe19adaf 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()) } }