From 222a4e7d42a386cbfb2992fb6ef2e25b868141df Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Apr 2026 00:31:21 +0000 Subject: [PATCH] fix(quic): round-4 tier-1 + tier-2 audit fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical interop blockers + security/correctness gaps surfaced by the parallel round-4 audit. All fixes have inline comments referencing the audit finding number. Frame layer: * Decode RESET_STREAM (0x04), STOP_SENDING (0x05), NEW_TOKEN (0x07). Pre-fix these fell through to the `unknown frame type` branch and threw QuicCodecException through the read loop, killing the connection. aioquic and picoquic emit RESET_STREAM regularly. * Wrap decodeFrames in try/catch in dispatchFrames; on a decode error, transition to CLOSED gracefully via markClosedExternally instead of letting the exception escape the read loop. Connection layer: * Reject peer-attempted CLIENT_BIDI / CLIENT_UNI stream IDs that don't map to a stream we opened (RFC 9000 §19.8 STREAM_STATE_ERROR). * MaxDataFrame now actually updates sendConnectionFlowCredit (was a no-op pre-fix; sustained sends silently stalled). * Writer enforces sendConnectionFlowCredit and tracks sendConnectionFlowConsumed so cumulative bytes stay under the peer's initial_max_data cap. * SERVER_BIDI peer-opened streams inherit sendCredit from peer.initialMaxStreamDataBidiLocal (was 0L; reply path was wedged until MAX_STREAM_DATA arrived). * applyPeerTransportParameters validates initial_source_connection_id and original_destination_connection_id (RFC 9000 §7.3 MUST checks); mismatch closes with TRANSPORT_PARAMETER_ERROR. * Cap incomingDatagrams queue at 256 (audio rooms ~50/sec; 5-second burst). On overflow, drop oldest — fresh frames matter more for live media. Pre-fix RFC 9221 datagrams were unbounded. Stream layer: * QuicStream.deliverIncoming now returns Boolean; parser closes the connection with INTERNAL_ERROR on saturation rather than silently dropping bytes (peer believes the bytes were delivered, application sees a hole). * ReceiveBuffer tracks finOffset and exposes isFullyRead(); parser only closes the incoming channel after the contiguous read frontier reaches the FIN offset (pre-fix closing on FIN-frame arrival truncated streams that had gaps). TLS hardening: * certificateValidator is non-null. Tests pass an explicit PermissiveCertificateValidator; null was a silent-MITM hazard. * Drop SIG_RSA_PKCS1_SHA256 from accepted CertificateVerify schemes (forbidden by RFC 8446 §4.2.3 in CertificateVerify). * Hard-fail the PSK-Finished path: we never offer a pre_shared_key extension, so a server skipping Certificate/CertificateVerify is either misbehaving or a partial-MITM stripping cert proof. * Validate ALPN: reject any ALPN the server selected that we didn't offer (was previously accepted silently). * Add APPLICATION-level inboundBuffer so post-handshake CRYPTO (NewSessionTicket, KeyUpdate detection) reaches the SENT_CLIENT_FINISHED handler. * State.FAILED is now actually assigned on any handler throw; pushHandshakeBytes refuses further bytes when in FAILED. * IP-literal precheck before InetAddress.getByName so cert validation doesn't trigger DNS A/AAAA lookups for hostnames (audit-4 #4: leaked SNI/hostname over plaintext DNS). WT layer: * GOAWAY id-regression check (RFC 9114 §5.2: MUST NOT increase). A server sending an increasing id raises QuicCodecException. * WT_CLOSE_SESSION decoder rejects bodies < 4 bytes (mandatory error-code field) and reasons > 8192 bytes. * Capsule reader catches Throwable but separately rethrows CancellationException; on parse error, completes peerCloseDeferred exceptionally so awaitPeerClose() doesn't hang forever. HTTP/3 + QPACK: * Http3Settings.decodeBody rejects duplicate ids (RFC 9114 §7.2.4.1 H3_SETTINGS_ERROR). * QpackInteger.decode bounds-checks shift before extending value; defence-in-depth Long-overflow check on accumulated value. * QpackDecoder static-table accesses go through a bounds-checking helper that throws typed QuicCodecException; literal lengths are range-checked before allocation. Test infra: * InMemoryQuicPipe accepts an injectable serverScid and constructs its tlsServer with TPs that include the required CIDs. * InProcessTlsServer emits stub Certificate + CertificateVerify so the real (non-PSK) handshake path is exercised. * Updated all test callers to use PermissiveCertificateValidator. * Updated CapsuleReaderTest with negative-path assertions for the new strictness. https://claude.ai/code/session_01EC1tfXfap8k8GyKvrxkxZx --- .../quic/connection/QuicConnection.kt | 85 +++++++++++-- .../quic/connection/QuicConnectionParser.kt | 112 ++++++++++++++++-- .../quic/connection/QuicConnectionWriter.kt | 20 +++- .../com/vitorpamplona/quic/frame/Frame.kt | 72 +++++++++++ .../vitorpamplona/quic/http3/Http3Settings.kt | 8 ++ .../vitorpamplona/quic/qpack/QpackDecoder.kt | 41 ++++++- .../vitorpamplona/quic/qpack/QpackInteger.kt | 14 +++ .../vitorpamplona/quic/stream/QuicStream.kt | 35 ++++-- .../quic/stream/ReceiveBuffer.kt | 29 ++++- .../com/vitorpamplona/quic/tls/TlsClient.kt | 68 +++++++++-- .../QuicWebTransportSessionState.kt | 16 ++- .../quic/webtransport/WtCapsule.kt | 31 +++-- .../quic/webtransport/WtPeerStreamDemux.kt | 25 +++- .../connection/CoalescedPacketSkipTest.kt | 12 +- .../quic/connection/InMemoryQuicPipe.kt | 32 ++++- .../quic/connection/InMemoryQuicPipeTest.kt | 4 +- .../quic/connection/PeerStreamLimitTest.kt | 41 ++++++- .../connection/ReceiveLimitEnforcementTest.kt | 24 +++- .../quic/tls/HelloRetryRequestTest.kt | 2 +- .../quic/tls/InProcessTlsServer.kt | 45 +++++++ .../quic/tls/TlsRoundTripTest.kt | 27 +++-- .../quic/webtransport/CapsuleReaderTest.kt | 31 +++-- .../quic/tls/JdkCertificateValidator.kt | 37 +++++- 23 files changed, 702 insertions(+), 109 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 e61081b1af..9a807f8004 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -61,13 +61,12 @@ class QuicConnection( val serverName: String, val config: QuicConnectionConfig, /** - * MUST be non-null for any network-facing connection. Pass an explicit - * `null` only when the caller has audited the threat model and accepts - * unauthenticated TLS (e.g. an in-process test loopback). There is no - * silent default — production callers either pass a system-trust-store - * validator or get a misconfiguration that's obvious in code review. + * Certificate validator is REQUIRED (audit-4 #1). For in-process tests + * pass an explicit [com.vitorpamplona.quic.tls.PermissiveCertificateValidator]; + * the type system catches "forgot to validate" misconfigurations instead + * of letting null silently disable MITM protection. */ - val tlsCertificateValidator: com.vitorpamplona.quic.tls.CertificateValidator?, + val tlsCertificateValidator: com.vitorpamplona.quic.tls.CertificateValidator, val nowMillis: () -> Long = { kotlin.time.Clock.System .now() @@ -133,7 +132,18 @@ class QuicConnection( internal var streamRoundRobinStart: Int = 0 private val pendingDatagrams = ArrayDeque() private val incomingDatagrams = ArrayDeque() - private var sendConnectionFlowCredit: Long = 0L + + /** + * Connection-level send credit, refreshed by inbound MAX_DATA frames + * (RFC 9000 §19.9). Internal because the parser updates it directly under + * the connection lock; the writer reads it via [sendConnectionFlowCreditSnapshot] + * to gate stream-frame emission once we've sent past the peer's cap. + */ + internal var sendConnectionFlowCredit: Long = 0L + + /** Total stream bytes we've already sent against [sendConnectionFlowCredit]. */ + internal var sendConnectionFlowConsumed: Long = 0L + private var receiveConnectionFlowLimit: Long = config.initialMaxData /** Streams the peer has opened that we haven't surfaced yet. */ @@ -214,6 +224,7 @@ class QuicConnection( transportParameters = buildLocalTransportParameters().encode(), secretsListener = tlsListener, certificateValidator = tlsCertificateValidator, + offeredAlpns = alpnList, ) init { @@ -256,6 +267,30 @@ class QuicConnection( private fun applyPeerTransportParameters() { val raw = tls.peerTransportParameters ?: return val tp = TransportParameters.decode(raw) + // Audit-4 #7: RFC 9000 §7.3 MUST checks. The peer's + // initial_source_connection_id MUST equal the SCID it put in its + // first Initial (which we adopted as `destinationConnectionId`). + // original_destination_connection_id MUST equal the DCID we put in + // our first Initial (`originalDestinationConnectionId`). + // Skipping these opens a CID-substitution / downgrade window where + // an attacker who can rewrite the first Initial can swap CIDs. + val iscid = tp.initialSourceConnectionId + if (iscid == null || !iscid.contentEquals(destinationConnectionId.bytes)) { + markClosedExternally( + "TRANSPORT_PARAMETER_ERROR: peer initial_source_connection_id mismatch", + ) + return + } + val odcid = tp.originalDestinationConnectionId + // We don't speak Retry yet, so the peer SHOULD echo our original DCID. + // If it's missing (some servers omit it pre-handshake-complete) we + // accept; if present but wrong we close. + if (odcid != null && !odcid.contentEquals(originalDestinationConnectionId.bytes)) { + markClosedExternally( + "TRANSPORT_PARAMETER_ERROR: peer original_destination_connection_id mismatch", + ) + return + } peerTransportParameters = tp sendConnectionFlowCredit = tp.initialMaxData ?: 0L peerMaxStreamsBidi = tp.initialMaxStreamsBidi ?: 0L @@ -424,19 +459,34 @@ class QuicConnection( */ internal fun getOrCreatePeerStreamLocked(id: Long): QuicStream { streams[id]?.let { return it } + val kind = StreamId.kindOf(id) val direction = - when (StreamId.kindOf(id)) { + when (kind) { StreamId.Kind.CLIENT_BIDI, StreamId.Kind.SERVER_BIDI -> QuicStream.Direction.BIDIRECTIONAL StreamId.Kind.SERVER_UNI -> QuicStream.Direction.UNIDIRECTIONAL_REMOTE_TO_LOCAL StreamId.Kind.CLIENT_UNI -> QuicStream.Direction.UNIDIRECTIONAL_LOCAL_TO_REMOTE } val stream = QuicStream(id, direction) - stream.sendCredit = 0L + // Audit-4 #11: SERVER_BIDI peer-opened streams inherit + // peerTransportParameters.initialMaxStreamDataBidiLocal as their + // sendCredit (we are writing back on a stream the peer initiated; + // the local-flow side's value applies). Previously they got 0L, + // which silently blocked any reply until an unsolicited + // MAX_STREAM_DATA arrived. + val tp = peerTransportParameters + stream.sendCredit = + when (kind) { + StreamId.Kind.SERVER_BIDI -> tp?.initialMaxStreamDataBidiLocal ?: 0L + + // Peer-uni and (defensively) peer-attempted CLIENT_* streams + // can't be written from our side, so 0 is correct. + else -> 0L + } // Pick the local receive-limit appropriate for the stream's direction // — peer-bidi → we advertised initialMaxStreamDataBidiRemote; // peer-uni → we advertised initialMaxStreamDataUni. stream.receiveLimit = - when (StreamId.kindOf(id)) { + when (kind) { StreamId.Kind.SERVER_UNI, StreamId.Kind.CLIENT_UNI -> config.initialMaxStreamDataUni StreamId.Kind.SERVER_BIDI -> config.initialMaxStreamDataBidiRemote StreamId.Kind.CLIENT_BIDI -> config.initialMaxStreamDataBidiLocal @@ -476,6 +526,21 @@ class QuicConnection( /** Caller must hold [lock]. */ internal fun streamByIdLocked(id: Long): QuicStream? = streams[id] + + companion object { + /** + * Bound on the inbound datagram queue depth. RFC 9221 datagrams are + * outside connection-level flow control, so without this cap a peer + * can pin arbitrary memory by spamming DATAGRAM frames. 256 entries + * × ~1200 bytes/datagram ≈ 300 KB worst case, which is fine even on + * memory-constrained devices and well above any realistic burst at + * audio-room rates (~50/sec). + * + * On overflow the parser drops the OLDEST queued datagram — for live + * audio/video, fresh frames matter more than stale ones. + */ + const val MAX_INCOMING_DATAGRAM_QUEUE: Int = 256 + } } /** Connection was closed (locally or by peer) before reaching CONNECTED. */ 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 fbe40112f4..af8b5803c5 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.quic.connection +import com.vitorpamplona.quic.QuicCodecException import com.vitorpamplona.quic.frame.AckFrame import com.vitorpamplona.quic.frame.ConnectionCloseFrame import com.vitorpamplona.quic.frame.CryptoFrame @@ -29,12 +30,16 @@ import com.vitorpamplona.quic.frame.MaxDataFrame import com.vitorpamplona.quic.frame.MaxStreamDataFrame import com.vitorpamplona.quic.frame.MaxStreamsFrame import com.vitorpamplona.quic.frame.NewConnectionIdFrame +import com.vitorpamplona.quic.frame.NewTokenFrame import com.vitorpamplona.quic.frame.PingFrame +import com.vitorpamplona.quic.frame.ResetStreamFrame +import com.vitorpamplona.quic.frame.StopSendingFrame import com.vitorpamplona.quic.frame.StreamFrame import com.vitorpamplona.quic.frame.decodeFrames import com.vitorpamplona.quic.packet.LongHeaderPacket import com.vitorpamplona.quic.packet.LongHeaderType import com.vitorpamplona.quic.packet.ShortHeaderPacket +import com.vitorpamplona.quic.stream.StreamId import com.vitorpamplona.quic.tls.TlsClient /** @@ -141,7 +146,18 @@ private fun dispatchFrames( packetNumber: Long, nowMillis: Long, ) { - val frames = decodeFrames(payload) + // 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) { @@ -175,11 +191,23 @@ private fun dispatchFrames( is StreamFrame -> { ackEliciting = true + // Audit-4 #5: reject peer-attempted CLIENT_BIDI / CLIENT_UNI + // stream IDs that don't match a stream we've opened. Per RFC + // 9000 §19.8, only the side that owns the parity may open; + // a server squatting on a CLIENT_* id is a protocol violation + // and could otherwise inject phantom streams into newPeerStreams. + if (StreamId.isClientInitiated(frame.streamId) && + conn.streamByIdLocked(frame.streamId) == null + ) { + conn.markClosedExternally( + "peer opened stream ${frame.streamId} on client-initiated id space (STREAM_STATE_ERROR)", + ) + return + } val stream = conn.getOrCreatePeerStreamLocked(frame.streamId) // RFC 9000 §4.1: peer MUST NOT send beyond the limit we advertised. - // We don't enforce per-stream limit here yet (it'd require closing - // with FLOW_CONTROL_ERROR), but we DO enforce connection-level - // bound to prevent unbounded memory growth from a misbehaving peer. + // The connection-level kill protects against unbounded memory + // growth from a misbehaving peer. val frameEnd = frame.offset + frame.data.size if (frameEnd > stream.receiveLimit) { conn.markClosedExternally( @@ -190,21 +218,53 @@ private fun dispatchFrames( stream.receive.insert(frame.offset, frame.data, frame.fin) val data = stream.receive.readContiguous() if (data.isNotEmpty()) { - stream.deliverIncoming(data) + val delivered = stream.deliverIncoming(data) + if (!delivered) { + // Audit-4 #3: incoming channel saturated. Closing the + // connection beats silently dropping bytes — a stalled + // consumer is better surfaced as an error than as a + // mysterious hole in the application's data. Use + // INTERNAL_ERROR (RFC 9000 §20.1). + conn.markClosedExternally( + "INTERNAL_ERROR: stream ${frame.streamId} consumer overflowed " + + "incoming channel (slow consumer)", + ) + return + } } - if (stream.receive.finReceived) { + // Audit-4 #4: only close the incoming channel once the + // contiguous read frontier has actually reached the FIN + // offset. Closing on FIN-arrival drops any later-arriving + // fill chunks silently because trySend on a closed channel + // returns failure — the application would see a truncated + // stream with no error signal. + if (stream.receive.finReceived && stream.receive.isFullyRead()) { stream.closeIncoming() } } is DatagramFrame -> { ackEliciting = true - conn.incomingDatagramsLocked().addLast(frame.data) + // Audit-4 #8: cap the inbound datagram queue. RFC 9221 + // datagrams are outside connection flow control; a peer can + // otherwise pin arbitrary memory by spamming DATAGRAM frames. + // We drop the OLDEST queued datagram when full — preferable + // for audio rooms (live streams) over rejecting fresh ones. + val queue = conn.incomingDatagramsLocked() + if (queue.size >= QuicConnection.MAX_INCOMING_DATAGRAM_QUEUE) { + queue.removeFirst() + } + queue.addLast(frame.data) conn.signalIncomingDatagram() } is MaxDataFrame -> { - // Updates connection-level send credit; left to the orchestrator. + // Audit-4 #9 + #12: previously a no-op, which silently stalled + // the writer once we sent more than initialMaxData total bytes. + // RFC 9000 §19.9: MAX_DATA only ever raises the cap. + if (frame.maxData > conn.sendConnectionFlowCredit) { + conn.sendConnectionFlowCredit = frame.maxData + } } is MaxStreamDataFrame -> { @@ -228,15 +288,51 @@ private fun dispatchFrames( } } + is ResetStreamFrame -> { + // Audit-4 #2: peer aborted the send side of a stream. We + // accept the frame for survival; the receive buffer keeps + // whatever it had. Closing the local read flow is left to + // the application or a future enhancement that surfaces the + // application error code. + ackEliciting = true + conn.streamByIdLocked(frame.streamId)?.closeIncoming() + } + + is StopSendingFrame -> { + // Audit-4 #2: peer asks us to stop sending on its read side. + // We don't model an outbound abort yet — this is acknowledged + // and dropped. A peer using STOP_SENDING to back-pressure + // would have to fall back to MAX_STREAM_DATA = 0, which we + // already honour. + ackEliciting = true + } + + is NewTokenFrame -> { + // Audit-4 #2: 0-RTT/resumption token. Out-of-scope; drop. + ackEliciting = true + } + is NewConnectionIdFrame -> { // We don't support migration; ignore. } is ConnectionCloseFrame -> { + // Audit-4 #13: any frames following CONNECTION_CLOSE in the + // same payload MUST NOT be dispatched — they could create + // streams or deliver bytes on an already-closed connection. conn.markClosedExternally("peer CONNECTION_CLOSE: ${frame.reason}") + return } is HandshakeDoneFrame -> { + // Audit-4 #14: HANDSHAKE_DONE is permitted ONLY at Application + // level (RFC 9000 §19.20). Anywhere else is PROTOCOL_VIOLATION. + if (level != EncryptionLevel.APPLICATION) { + conn.markClosedExternally( + "HANDSHAKE_DONE at $level (PROTOCOL_VIOLATION; allowed only at APPLICATION)", + ) + return + } conn.status = QuicConnection.Status.CONNECTED } 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 af758d2e9f..4248ffce74 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -259,8 +259,15 @@ private fun buildApplicationPacket( // Drain stream send buffers — round-robin starting from a rotating index // so streams created earlier don't starve streams created later under MTU - // pressure. Honors per-stream send credit (RFC 9000 §4). + // pressure. Honors per-stream send credit (RFC 9000 §4) AND connection- + // level send credit (audit-4 #9: previously the writer ignored it + // entirely; the peer's initial_max_data was decoded then forgotten, + // causing the connection to be torn down with FLOW_CONTROL_ERROR once + // we cumulatively sent past the cap). var packetBudget = 1100 + val connRemaining = + (conn.sendConnectionFlowCredit - conn.sendConnectionFlowConsumed).coerceAtLeast(0L) + var connBudget = connRemaining val streamsList = conn.streamsLocked().entries.toList() if (streamsList.isNotEmpty()) { val start = conn.streamRoundRobinStart % streamsList.size @@ -268,12 +275,19 @@ private fun buildApplicationPacket( if (packetBudget <= 64) break val (id, stream) = streamsList[(start + i) % streamsList.size] val streamRemaining = (stream.sendCredit - stream.send.sentOffset).coerceAtLeast(0L) - if (streamRemaining <= 0L && !stream.send.finPending) continue - val maxBytes = minOf(packetBudget - 32, streamRemaining.coerceAtMost(Int.MAX_VALUE.toLong()).toInt()) + // Skip if both stream and connection have no credit; FIN-only + // (zero-byte) chunks may still go through because they don't + // consume credit. + if (streamRemaining <= 0L && connBudget <= 0L && !stream.send.finPending) continue + val effectiveCap = minOf(streamRemaining, connBudget) + val maxBytes = + minOf(packetBudget - 32, effectiveCap.coerceAtMost(Int.MAX_VALUE.toLong()).toInt()) val chunk = stream.send.takeChunk(maxBytes = maxBytes) ?: continue if (chunk.data.isNotEmpty() || chunk.fin) { frames += StreamFrame(streamId = id, offset = chunk.offset, data = chunk.data, fin = chunk.fin, explicitLength = true) packetBudget -= chunk.data.size + 32 + connBudget -= chunk.data.size + conn.sendConnectionFlowConsumed += chunk.data.size } } conn.streamRoundRobinStart = (start + 1) % streamsList.size diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/frame/Frame.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/frame/Frame.kt index 56ae817c20..e2c47f379e 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/frame/Frame.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/frame/Frame.kt @@ -158,6 +158,60 @@ class ConnectionCloseFrame( } } +/** + * RFC 9000 §19.4 — peer abruptly terminates the send side of a stream. + * + * Audit-4 finding: peers (aioquic, picoquic) routinely emit RESET_STREAM and + * the prior parser dropped the connection on first arrival because the frame + * type wasn't decoded. We accept and surface it; cleanup of the affected + * receive buffer is left to the orchestrator (no existing test exercises a + * post-reset read, but the parser must not crash). + */ +class ResetStreamFrame( + val streamId: Long, + val applicationErrorCode: Long, + val finalSize: Long, +) : Frame() { + override fun encode(out: QuicWriter) { + out.writeByte(FrameType.RESET_STREAM.toInt()) + out.writeVarint(streamId) + out.writeVarint(applicationErrorCode) + out.writeVarint(finalSize) + } +} + +/** + * RFC 9000 §19.5 — peer asks us to stop sending on a stream we own. We don't + * model an outbound abort yet (MoQ-minimal scope), so we accept the frame + * for survival and let the application read [streamId]/[applicationErrorCode] + * if it ever wires a handler. + */ +class StopSendingFrame( + val streamId: Long, + val applicationErrorCode: Long, +) : Frame() { + override fun encode(out: QuicWriter) { + out.writeByte(FrameType.STOP_SENDING.toInt()) + out.writeVarint(streamId) + out.writeVarint(applicationErrorCode) + } +} + +/** + * RFC 9000 §19.7 — server provides a token for use in a future Initial. We + * don't do 0-RTT or stateful resumption, so the token is dropped, but the + * frame MUST decode without killing the connection. + */ +class NewTokenFrame( + val token: ByteArray, +) : Frame() { + override fun encode(out: QuicWriter) { + out.writeByte(FrameType.NEW_TOKEN.toInt()) + out.writeVarint(token.size.toLong()) + out.writeBytes(token) + } +} + class MaxDataFrame( val maxData: Long, ) : Frame() { @@ -268,6 +322,19 @@ fun decodeFrames(data: ByteArray): List { out += AckFrame(largest, delay, firstRange, ranges) } + type == FrameType.RESET_STREAM -> { + val streamId = r.readVarint() + val errorCode = r.readVarint() + val finalSize = r.readVarint() + out += ResetStreamFrame(streamId, errorCode, finalSize) + } + + type == FrameType.STOP_SENDING -> { + val streamId = r.readVarint() + val errorCode = r.readVarint() + out += StopSendingFrame(streamId, errorCode) + } + type == FrameType.CRYPTO -> { val offset = r.readVarint() val len = boundedLength(r.readVarint(), r.remaining, "CRYPTO") @@ -275,6 +342,11 @@ fun decodeFrames(data: ByteArray): List { out += CryptoFrame(offset, data2) } + type == FrameType.NEW_TOKEN -> { + val tokenLen = boundedLength(r.readVarint(), r.remaining, "NEW_TOKEN") + out += NewTokenFrame(r.readBytes(tokenLen)) + } + type in FrameType.STREAM_BASE..(FrameType.STREAM_BASE or 0x07) -> { val flags = (type - FrameType.STREAM_BASE) val hasOff = (flags and FrameType.STREAM_OFF_BIT) != 0L diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/http3/Http3Settings.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/http3/Http3Settings.kt index cd1147d789..212e4dbc37 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/http3/Http3Settings.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/http3/Http3Settings.kt @@ -55,6 +55,14 @@ data class Http3Settings( while (r.hasMore()) { val id = r.readVarint() val value = r.readVarint() + // Audit-4 #18: RFC 9114 §7.2.4.1 — duplicate SETTINGS ids + // MUST cause a connection error of type H3_SETTINGS_ERROR. + // Pre-fix the second value silently overwrote the first. + if (map.containsKey(id)) { + throw com.vitorpamplona.quic.QuicCodecException( + "duplicate HTTP/3 SETTINGS id 0x${id.toString(16)}", + ) + } map[id] = value } return Http3Settings(map) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackDecoder.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackDecoder.kt index ece3357416..9311083f8a 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackDecoder.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackDecoder.kt @@ -60,7 +60,7 @@ class QpackDecoder { if (!isStatic) throw QuicCodecException("QPACK dynamic indexed field line unsupported") val r = QpackInteger.decode(payload, pos, 6) pos += r.bytesConsumed - val entry = QpackStaticTable.entries[r.value.toInt()] + val entry = staticEntryAt(r.value) out += entry } @@ -70,7 +70,7 @@ class QpackDecoder { if (!isStatic) throw QuicCodecException("QPACK dynamic name-ref field line unsupported") val nameRef = QpackInteger.decode(payload, pos, 4) pos += nameRef.bytesConsumed - val name = QpackStaticTable.entries[nameRef.value.toInt()].first + val name = staticEntryAt(nameRef.value).first val (value, valueLen) = readStringLiteral(payload, pos) pos += valueLen out += name to value @@ -81,7 +81,12 @@ class QpackDecoder { val nameH = (first and 0x08) != 0 val nameLenR = QpackInteger.decode(payload, pos, 3) pos += nameLenR.bytesConsumed - val nameBytes = ByteArray(nameLenR.value.toInt()) + // Audit-4 #10: bound the literal length before truncating + // to Int and allocating. A malformed encoder could otherwise + // pass a value > Int.MAX_VALUE that wraps negative or + // OOMs. + val nameLen = boundedQpackLength(nameLenR.value, payload.size - pos, "QPACK literal name") + val nameBytes = ByteArray(nameLen) payload.copyInto(nameBytes, 0, pos, pos + nameBytes.size) pos += nameBytes.size val name = if (nameH) QpackHuffman.decode(nameBytes).decodeToString() else nameBytes.decodeToString() @@ -108,8 +113,36 @@ class QpackDecoder { val huffman = (first and 0x80) != 0 val lenR = QpackInteger.decode(payload, offset, 7) val dataStart = offset + lenR.bytesConsumed - val raw = payload.copyOfRange(dataStart, dataStart + lenR.value.toInt()) + // Audit-4 #10: range-check before allocating. payload.size - dataStart + // is the upper bound on a legitimate literal; a value past it is + // malformed. + val len = boundedQpackLength(lenR.value, payload.size - dataStart, "QPACK literal value") + val raw = payload.copyOfRange(dataStart, dataStart + len) val str = if (huffman) QpackHuffman.decode(raw).decodeToString() else raw.decodeToString() return str to (lenR.bytesConsumed + raw.size) } + + /** + * Static-table index lookup with explicit bounds. Pre-fix a malformed + * encoder (or a `Long → Int` truncation that wrapped negative) produced + * raw IndexOutOfBoundsException; we now throw a typed QuicCodecException + * the caller's `catch (_: Throwable)` paths can distinguish. + */ + private fun staticEntryAt(index: Long): Pair { + if (index < 0L || index >= QpackStaticTable.entries.size.toLong()) { + throw QuicCodecException("QPACK static-table index $index out of range") + } + return QpackStaticTable.entries[index.toInt()] + } + + private fun boundedQpackLength( + value: Long, + remaining: Int, + field: String, + ): Int { + if (value < 0L || value > remaining) { + throw QuicCodecException("$field length $value out of bounds (remaining=$remaining)") + } + return value.toInt() + } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackInteger.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackInteger.kt index da43817032..5bc6826ad2 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackInteger.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/qpack/QpackInteger.kt @@ -72,7 +72,21 @@ object QpackInteger { while (true) { if (pos >= src.size) throw QuicCodecException("truncated QPACK integer continuation") val b = src[pos++].toInt() and 0xFF + // Audit-4 #12: range-check BEFORE shifting. Pre-fix the check ran + // after `shift += 7`, so a continuation byte read with shift == 63 + // could already wrap Long quietly before the next iteration's + // check fired. We also reject values that would overflow at the + // top of the 63-bit range. + if (shift >= 63 && (b and 0x7F) > 0) { + throw QuicCodecException("QPACK integer too large") + } value += ((b and 0x7F).toLong() shl shift) + if (value < 0L) { + // Defence-in-depth: any sign-bit flip indicates overflow that + // slipped past the shift check (shouldn't happen, but the + // cost is one branch per continuation byte). + throw QuicCodecException("QPACK integer overflowed Long") + } if ((b and 0x80) == 0) return DecodeResult(value, pos - offset) shift += 7 if (shift > 63) throw QuicCodecException("QPACK integer too large") diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt index c99868d8f3..487f927083 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt @@ -42,15 +42,25 @@ class QuicStream( /** * Bytes received and confirmed contiguous, exposed as a flow to the consumer. * - * Bounded buffer (64 chunks) — combined with the per-stream receive-limit - * enforced in the parser, this caps unbounded memory growth from a slow - * consumer. Producer (the parser) uses [trySend], dropping bytes if the - * channel is full; the receive-limit enforcement makes "channel full" a - * connection-level error long before it becomes a memory problem. + * Bounded buffer (64 chunks). The producer (parser) uses [trySend] and + * surfaces saturation by setting [overflowed]; the parser checks this flag + * after each delivery and tears the connection down with INTERNAL_ERROR + * rather than silently dropping bytes. Pre-audit-4 the failed `trySend` + * was discarded, leaving a hole in the stream that the application could + * never know about. */ private val incomingChannel = Channel(capacity = 64) val incoming: Flow get() = incomingChannel.consumeAsFlow() + /** + * True once a [deliverIncoming] call failed because the channel was + * saturated (slow consumer). The parser observes this and closes the + * connection rather than letting bytes silently disappear. + */ + @Volatile + var overflowed: Boolean = false + private set + /** Per-stream send credit (peer's MAX_STREAM_DATA value). */ var sendCredit: Long = 0L internal set @@ -63,10 +73,17 @@ class QuicStream( val isClosed: Boolean get() = send.finSent && receive.finReceived - internal fun deliverIncoming(data: ByteArray) { - if (data.isNotEmpty()) { - incomingChannel.trySend(data) - } + /** + * Pushes [data] toward the consumer. Returns false if the bounded channel + * was full; the caller (parser) is expected to escalate to a connection- + * level error in that case (audit-4 #3 — silent data loss is unacceptable + * because the peer believes the bytes were delivered). + */ + internal fun deliverIncoming(data: ByteArray): Boolean { + if (data.isEmpty()) return true + val ok = incomingChannel.trySend(data).isSuccess + if (!ok) overflowed = true + return ok } internal fun closeIncoming() { diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/ReceiveBuffer.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/ReceiveBuffer.kt index 869e0849f1..702807c8bd 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/ReceiveBuffer.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/ReceiveBuffer.kt @@ -41,10 +41,19 @@ class ReceiveBuffer { var readOffset: Long = 0L private set - /** True once all sender-emitted bytes have been fully delivered. */ + /** True once a STREAM frame carrying FIN has been observed. */ var finReceived: Boolean = false private set + /** + * Total stream length, set the moment any frame carrying FIN arrives. + * Equals `offset + data.size` of the FIN-bearing frame; null until then. + * Used by [isFullyRead] to distinguish "FIN seen but holes remain" from + * "FIN seen and contiguous read frontier reached the end". + */ + var finOffset: Long? = null + private set + /** Insert a chunk at [offset] of size [data.size]. Idempotent on overlap. */ fun insert( offset: Long, @@ -52,7 +61,15 @@ class ReceiveBuffer { fin: Boolean = false, ) { if (data.isEmpty() && !fin) return - if (fin) finReceived = true + if (fin) { + finReceived = true + // The FIN flag carries an implicit final offset = offset + data.size. + // RFC 9000 §4.5: once set, this MUST NOT change; ignore subsequent + // FIN frames whose final size disagrees (they should already have + // been rejected at the stream-state level, but be defensive here). + val finalSize = offset + data.size + if (finOffset == null) finOffset = finalSize + } if (data.isEmpty()) return val end = offset + data.size @@ -121,6 +138,14 @@ class ReceiveBuffer { /** Highest contiguous offset received so far. */ fun contiguousEnd(): Long = readOffset + /** + * True once the contiguous read frontier has reached the FIN offset, i.e. + * the application has received every byte the sender ever sent. Closing + * the consumer-facing channel before this point would silently drop any + * later-arriving fill chunks — that's the audit-4 #4 bug. + */ + fun isFullyRead(): Boolean = finReceived && chunks.isEmpty() && finOffset == readOffset + private class Chunk( val offset: Long, val data: ByteArray, diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsClient.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsClient.kt index dccedefadb..691adb8ff4 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsClient.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsClient.kt @@ -51,13 +51,19 @@ class TlsClient( val transportParameters: ByteArray, val secretsListener: TlsSecretsListener, /** - * Certificate validator MUST be supplied for any production / network-facing - * use. The only acceptable null is in-process tests where there's no real - * server identity to authenticate (e.g. [TlsRoundTripTest]'s loopback). A - * null validator here means "no MITM protection" — a misconfigured caller - * must fail loudly, not silently accept any certificate. + * Audit-4 #1: certificate validator is REQUIRED (non-null). For tests that + * connect to a self-signed in-process server, pass an explicit + * [PermissiveCertificateValidator] — the type system makes "no MITM + * protection" a deliberate, code-review-visible choice instead of a quiet + * forgotten null. */ - val certificateValidator: CertificateValidator?, + val certificateValidator: CertificateValidator, + /** + * The ALPN values we offered in ClientHello. Used to validate the server's + * EncryptedExtensions ALPN selection (audit-4 #20: a server picking an + * unknown ALPN was previously accepted silently). + */ + val offeredAlpns: List = listOf(TlsConstants.ALPN_H3), /** When non-null, used as the X25519 ephemeral key (for deterministic tests). */ val fixedKeyPair: X25519KeyPair? = null, /** When non-null, used as the ClientHello random (for deterministic tests). */ @@ -97,6 +103,11 @@ class TlsClient( mutableMapOf( Level.INITIAL to ByteArrayBuilder(), Level.HANDSHAKE to ByteArrayBuilder(), + // Audit-4 #6: include APPLICATION so post-handshake CRYPTO + // (NewSessionTicket / KeyUpdate detection) actually reaches the + // SENT_CLIENT_FINISHED handler. Pre-fix, pushHandshakeBytes at + // APPLICATION threw because the buffer wasn't registered. + Level.APPLICATION to ByteArrayBuilder(), ) private val transcript = TlsTranscriptHash() @@ -138,6 +149,13 @@ class TlsClient( level: Level, bytes: ByteArray, ) { + // Audit-4 #7: once a handshake error has fired, refuse further bytes + // rather than re-entering parsing on stale state. The QUIC layer + // sees the FAILED state via the bubbled QuicCodecException and + // closes the connection. + if (state == State.FAILED) { + throw QuicCodecException("TLS handshake already failed; ignoring further bytes at $level") + } val buf = inboundBuffers[level] ?: throw QuicCodecException("no buffer at level $level") buf.append(bytes) drainInbound(level, buf) @@ -149,7 +167,14 @@ class TlsClient( ) { while (true) { val msg = buf.takeHandshakeMessage() ?: break - handleHandshakeMessage(level, msg) + try { + handleHandshakeMessage(level, msg) + } catch (t: Throwable) { + // Audit-4 #7: any throw from a handler transitions to FAILED + // so a retry doesn't re-enter parsing on inconsistent state. + state = State.FAILED + throw t + } } } @@ -207,7 +232,17 @@ class TlsClient( if (type != TlsConstants.HS_ENCRYPTED_EXTENSIONS) throw QuicCodecException("expected EncryptedExtensions, got type=$type") if (level != Level.HANDSHAKE) throw QuicCodecException("EncryptedExtensions must arrive at Handshake level") val ee = TlsEncryptedExtensions.decodeBody(bodyReader) - negotiatedAlpn = ee.alpn + // Audit-4 #20: validate the server actually selected one of + // the ALPNs we offered. Pre-fix any negotiated ALPN was + // accepted; a server picking an unknown ALPN would silently + // proceed with HTTP/3 code paths assuming h3. + val alpn = ee.alpn + if (alpn != null && !offeredAlpns.any { it.contentEquals(alpn) }) { + throw QuicCodecException( + "server selected ALPN '${alpn.decodeToString()}' which we did not offer", + ) + } + negotiatedAlpn = alpn peerTransportParameters = ee.quicTransportParameters transcript.append(msg) state = State.WAITING_CERTIFICATE_OR_FINISHED @@ -217,15 +252,22 @@ class TlsClient( when (type) { TlsConstants.HS_CERTIFICATE -> { val cert = TlsCertificateChain.decodeBody(bodyReader) - certificateValidator?.validateChain(cert.certificates, serverName) + certificateValidator.validateChain(cert.certificates, serverName) transcript.append(msg) state = State.WAITING_CERTIFICATE_VERIFY } TlsConstants.HS_FINISHED -> { - // PSK-only handshake skips Certificate/CertificateVerify. We never use PSK, - // but the state machine handles the transition for completeness. - handleServerFinished(msg, bodyReader, len) + // Audit-4 #3: we never offer a `pre_shared_key` + // extension, so a server MUST send Certificate + + // CertificateVerify. A Finished here means a + // misbehaving server (or an MITM that stripped the + // cert messages). Hard-fail rather than completing + // a handshake with no peer authentication. + throw QuicCodecException( + "server skipped Certificate/CertificateVerify but we never offered PSK " + + "(unauthenticated handshake refused)", + ) } else -> { @@ -238,7 +280,7 @@ class TlsClient( if (type != TlsConstants.HS_CERTIFICATE_VERIFY) throw QuicCodecException("expected CertificateVerify, got type=$type") val cv = TlsCertificateVerify.decodeBody(bodyReader) val transcriptHash = transcript.snapshot() - certificateValidator?.verifySignature(cv.signatureAlgorithm, cv.signature, transcriptHash) + certificateValidator.verifySignature(cv.signatureAlgorithm, cv.signature, transcriptHash) transcript.append(msg) state = State.WAITING_SERVER_FINISHED } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/QuicWebTransportSessionState.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/QuicWebTransportSessionState.kt index 98fe9c7971..103cf779dc 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/QuicWebTransportSessionState.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/QuicWebTransportSessionState.kt @@ -114,10 +114,18 @@ class QuicWebTransportSessionState( // application needs them. } } - } catch (_: Throwable) { - // Stream closed before a capsule arrived. Leave peerCloseDeferred - // uncompleted — the connection-level close path is the source - // of truth in that case. + } catch (ce: kotlinx.coroutines.CancellationException) { + // Audit-4 #17: do NOT swallow CancellationException — the + // session's scope.cancel() needs it to actually terminate + // the coroutine, not get caught here. + throw ce + } catch (t: Throwable) { + // Audit-4 #15: a malformed capsule (e.g. truncated CLOSE_SESSION + // body) used to leave peerCloseDeferred forever-suspended. + // Surface the error so awaitPeerClose() exits with cause. + if (!peerCloseDeferred.isCompleted) { + peerCloseDeferred.completeExceptionally(t) + } } } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtCapsule.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtCapsule.kt index 9a9ba9259e..fb036a8b44 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtCapsule.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtCapsule.kt @@ -106,17 +106,30 @@ class CapsuleReader { pos = bodyEnd return when (typeRes.value) { WtCapsuleType.WT_CLOSE_SESSION -> { + // Audit-4 #13: a body shorter than the mandatory 4-byte + // application_error_code field is malformed. Pre-fix we + // silently substituted (0, "") which the application could + // not distinguish from a legitimate clean close. if (body.size < 4) { - WtCloseSession(0, "") - } else { - val errorCode = - ((body[0].toInt() and 0xFF) shl 24) or - ((body[1].toInt() and 0xFF) shl 16) or - ((body[2].toInt() and 0xFF) shl 8) or - (body[3].toInt() and 0xFF) - val reason = body.copyOfRange(4, body.size).decodeToString() - WtCloseSession(errorCode, reason) + throw com.vitorpamplona.quic.QuicCodecException( + "WT_CLOSE_SESSION body too short (${body.size} < 4)", + ) } + // Audit-4 #14: draft-ietf-webtrans-http3 §5 caps the reason + // string at 8192 bytes. Reject overlong reasons rather than + // letting them through. + if (body.size - 4 > 8192) { + throw com.vitorpamplona.quic.QuicCodecException( + "WT_CLOSE_SESSION reason exceeds 8192 bytes (${body.size - 4})", + ) + } + val errorCode = + ((body[0].toInt() and 0xFF) shl 24) or + ((body[1].toInt() and 0xFF) shl 16) or + ((body[2].toInt() and 0xFF) shl 8) or + (body[3].toInt() and 0xFF) + val reason = body.copyOfRange(4, body.size).decodeToString() + WtCloseSession(errorCode, reason) } else -> { diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtPeerStreamDemux.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtPeerStreamDemux.kt index 1475243c9b..b6753ea5fd 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtPeerStreamDemux.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/WtPeerStreamDemux.kt @@ -163,6 +163,10 @@ class WtPeerStreamDemux( } emitStripped(stream, pending, chunkChannel, isUni = false) } + } catch (ce: kotlinx.coroutines.CancellationException) { + // Audit-4 #17: don't swallow cancellation — needs to propagate + // to actually tear down the coroutine when scope is cancelled. + throw ce } catch (_: Throwable) { // peer closed mid-prefix or framing error — drop quietly } @@ -193,11 +197,24 @@ class WtPeerStreamDemux( is Http3Frame.Goaway -> { // GOAWAY body is a single varint Stream ID. Decode it so // applications can observe drain state via - // [peerGoawayStreamId]. Malformed bodies are dropped — RFC - // 9114 §7.2.6 says servers SHOULD send a single varint - // but we don't kill the connection over a parse error here. + // [peerGoawayStreamId]. Malformed bodies are dropped. + // + // Audit-4 #5: RFC 9114 §5.2 — a subsequent GOAWAY id MUST + // be ≤ the previous one (the "last accepted" id only + // shrinks). A peer regressing this is H3_ID_ERROR; we + // throw, the surrounding `route` catch maps that to a + // black-hole, and the application sees the previously + // recorded id stay put. val res = Varint.decode(frame.body, 0) - if (res != null) peerGoawayStreamId = res.value + if (res != null) { + val prev = peerGoawayStreamId + if (prev != null && res.value > prev) { + throw com.vitorpamplona.quic.QuicCodecException( + "H3_ID_ERROR: GOAWAY id increased ($prev → ${res.value})", + ) + } + peerGoawayStreamId = res.value + } } // no new requests; we don't enforce yet diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CoalescedPacketSkipTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CoalescedPacketSkipTest.kt index 37941527a0..6535a28a3e 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CoalescedPacketSkipTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CoalescedPacketSkipTest.kt @@ -88,7 +88,9 @@ class CoalescedPacketSkipTest { QuicConnection( serverName = "example.test", config = QuicConnectionConfig(), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) val secrets = InitialSecrets.derive(client.destinationConnectionId.bytes) val serverScid = ConnectionId.random(8) @@ -119,7 +121,9 @@ class CoalescedPacketSkipTest { QuicConnection( serverName = "example.test", config = QuicConnectionConfig(), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) val secrets = InitialSecrets.derive(client.destinationConnectionId.bytes) val serverScid = ConnectionId.random(8) @@ -155,7 +159,9 @@ class CoalescedPacketSkipTest { QuicConnection( serverName = "example.test", config = QuicConnectionConfig(), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) val secrets = InitialSecrets.derive(client.destinationConnectionId.bytes) val serverScid = ConnectionId.random(8) diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipe.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipe.kt index 22ca6e4a2b..dbe027e693 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipe.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipe.kt @@ -59,11 +59,34 @@ class InMemoryQuicPipe( val client: QuicConnection, val initialDcid: ByteArray, /** - * Optional pre-configured TLS server. Tests that need to advertise non-empty - * QUIC transport parameters (e.g. to exercise MAX_STREAMS routing) build - * their own [InProcessTlsServer] and pass it here. + * Server-side source connection id. Exposed on the constructor so the + * [tlsServer] can advertise it as `initial_source_connection_id` in + * transport parameters (RFC 9000 §7.3 — REQUIRED). Defaults to a fresh + * 8-byte random id. */ - private val tlsServer: InProcessTlsServer = InProcessTlsServer(), + val serverScid: ConnectionId = ConnectionId.random(8), + /** + * Optional pre-configured TLS server. The default builds one that + * advertises the bare-minimum transport parameters required by audit-4 + * #7's CID-validation: `initial_source_connection_id = serverScid` plus + * generous data/stream caps so handshake tests work without each having + * to construct their own. Tests that want to exercise specific TP values + * build their own server and pass it. + */ + private val tlsServer: InProcessTlsServer = + InProcessTlsServer( + transportParameters = + TransportParameters( + initialMaxData = 1_000_000, + initialMaxStreamDataBidiLocal = 100_000, + initialMaxStreamDataBidiRemote = 100_000, + initialMaxStreamDataUni = 100_000, + initialMaxStreamsBidi = 16, + initialMaxStreamsUni = 16, + initialSourceConnectionId = serverScid.bytes, + originalDestinationConnectionId = initialDcid, + ).encode(), + ), ) { private val initial = InitialSecrets.derive(initialDcid) private val hp = AesEcbHeaderProtection(PlatformAesOneBlock) @@ -74,7 +97,6 @@ class InMemoryQuicPipe( private var serverApplicationRx: PacketProtection? = null private var serverApplicationTx: PacketProtection? = null - private val serverScid = ConnectionId.random(8) private val initialPnSpace = PacketNumberSpaceState() private val handshakePnSpace = PacketNumberSpaceState() private val applicationPnSpace = PacketNumberSpaceState() diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipeTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipeTest.kt index 5edc27af04..d0538fea49 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipeTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipeTest.kt @@ -44,7 +44,9 @@ class InMemoryQuicPipeTest { QuicConnection( serverName = "example.test", config = QuicConnectionConfig(), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) val pipe = InMemoryQuicPipe(client = client, initialDcid = client.destinationConnectionId.bytes) diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PeerStreamLimitTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PeerStreamLimitTest.kt index bb8b7518b8..651545fa35 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PeerStreamLimitTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PeerStreamLimitTest.kt @@ -49,11 +49,34 @@ class PeerStreamLimitTest { QuicConnection( serverName = "example.test", config = QuicConnectionConfig(), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), + ) + // Explicitly advertise zero bidi streams. (The pipe's default TPs + // grant 16, so we override.) + val serverScid = ConnectionId.random(8) + val tlsServer = + InProcessTlsServer( + transportParameters = + TransportParameters( + initialMaxData = 1_000_000, + initialMaxStreamDataBidiLocal = 100_000, + initialMaxStreamDataBidiRemote = 100_000, + initialMaxStreamDataUni = 100_000, + initialMaxStreamsBidi = 0, + initialMaxStreamsUni = 0, + initialSourceConnectionId = serverScid.bytes, + originalDestinationConnectionId = client.destinationConnectionId.bytes, + ).encode(), + ) + val pipe = + InMemoryQuicPipe( + client = client, + initialDcid = client.destinationConnectionId.bytes, + serverScid = serverScid, + tlsServer = tlsServer, ) - // Default InProcessTlsServer sends empty transport parameters — so - // the client's peerMaxStreamsBidi resolves to 0 after handshake. - val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes) client.start() pipe.drive(maxRounds = 16) assertEquals(QuicConnection.Status.CONNECTED, client.status) @@ -71,8 +94,13 @@ class PeerStreamLimitTest { QuicConnection( serverName = "example.test", config = QuicConnectionConfig(), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) + // Build the TLS server with the audit-4 #7 required CIDs plus + // tight stream caps for the boundary test. + val serverScid = ConnectionId.random(8) val serverTpBytes = TransportParameters( initialMaxData = 1_000_000, @@ -81,12 +109,15 @@ class PeerStreamLimitTest { initialMaxStreamDataUni = 100_000, initialMaxStreamsBidi = 3, initialMaxStreamsUni = 0, + initialSourceConnectionId = serverScid.bytes, + originalDestinationConnectionId = client.destinationConnectionId.bytes, ).encode() val tlsServer = InProcessTlsServer(transportParameters = serverTpBytes) val pipe = InMemoryQuicPipe( client = client, initialDcid = client.destinationConnectionId.bytes, + serverScid = serverScid, tlsServer = tlsServer, ) client.start() diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ReceiveLimitEnforcementTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ReceiveLimitEnforcementTest.kt index 967adbeca5..865d0300aa 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ReceiveLimitEnforcementTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ReceiveLimitEnforcementTest.kt @@ -58,8 +58,14 @@ class ReceiveLimitEnforcementTest { initialMaxStreamDataBidiRemote = 32, initialMaxStreamDataBidiLocal = 32, ), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) + // Audit-4 #7: TPs MUST include initial_source_connection_id matching + // the SCID the server uses on the wire — otherwise the post-handshake + // CID-validation step closes the connection. + val serverScid = ConnectionId.random(8) val tlsServer = InProcessTlsServer( transportParameters = @@ -70,9 +76,11 @@ class ReceiveLimitEnforcementTest { initialMaxStreamDataUni = 100_000, initialMaxStreamsBidi = 16, initialMaxStreamsUni = 16, + initialSourceConnectionId = serverScid.bytes, + originalDestinationConnectionId = client.destinationConnectionId.bytes, ).encode(), ) - val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, tlsServer) + val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, serverScid, tlsServer) client.start() pipe.drive(maxRounds = 16) assertEquals(QuicConnection.Status.CONNECTED, client.status) @@ -113,8 +121,14 @@ class ReceiveLimitEnforcementTest { initialMaxStreamDataBidiRemote = 64, initialMaxStreamDataBidiLocal = 64, ), - tlsCertificateValidator = null, + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), ) + // Audit-4 #7: TPs MUST include initial_source_connection_id matching + // the SCID the server uses on the wire — otherwise the post-handshake + // CID-validation step closes the connection. + val serverScid = ConnectionId.random(8) val tlsServer = InProcessTlsServer( transportParameters = @@ -125,9 +139,11 @@ class ReceiveLimitEnforcementTest { initialMaxStreamDataUni = 100_000, initialMaxStreamsBidi = 16, initialMaxStreamsUni = 16, + initialSourceConnectionId = serverScid.bytes, + originalDestinationConnectionId = client.destinationConnectionId.bytes, ).encode(), ) - val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, tlsServer) + val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, serverScid, tlsServer) client.start() pipe.drive(maxRounds = 16) assertEquals(QuicConnection.Status.CONNECTED, client.status) diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/HelloRetryRequestTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/HelloRetryRequestTest.kt index e631868248..4b2639ac34 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/HelloRetryRequestTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/HelloRetryRequestTest.kt @@ -42,7 +42,7 @@ class HelloRetryRequestTest { serverName = "example.test", transportParameters = ByteArray(0), secretsListener = NoopSecretsListener, - certificateValidator = null, + certificateValidator = PermissiveCertificateValidator(), ) tls.start() // Drain (and discard) ClientHello. diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/InProcessTlsServer.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/InProcessTlsServer.kt index 9121a2f56e..356b8b2527 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/InProcessTlsServer.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/InProcessTlsServer.kt @@ -136,6 +136,20 @@ class InProcessTlsServer( transcript.append(ee) outboundHandshake.addLast(ee) + // Audit-4 #3: TlsClient now hard-fails any handshake that skips + // Certificate + CertificateVerify (no PSK was offered, so a peer that + // omits them is either misbehaving or a partial-MITM stripping the + // cert proof). Emit syntactically-valid stubs that + // [PermissiveCertificateValidator] will accept; the test path goes + // through the same code as a real handshake. + val cert = buildCertificateStub() + transcript.append(cert) + outboundHandshake.addLast(cert) + + val cv = buildCertificateVerifyStub() + transcript.append(cv) + outboundHandshake.addLast(cv) + // 7. Build server Finished val sf = buildFinished(serverHandshakeSecret!!) transcript.append(sf) @@ -208,4 +222,35 @@ class InProcessTlsServer( w.withUint24Length { writeBytes(tag) } return w.toByteArray() } + + /** + * Encode a Certificate message with one stub leaf cert. The DER bytes are + * not a real cert — [PermissiveCertificateValidator] doesn't parse them. + * We just need the framing to round-trip through TlsCertificateChain.decodeBody. + */ + private fun buildCertificateStub(): ByteArray { + val w = QuicWriter() + w.writeByte(TlsConstants.HS_CERTIFICATE) + w.withUint24Length { + // certificate_request_context (opaque<0..255>) — empty for server cert. + writeTlsOpaque1(ByteArray(0)) + // certificate_list — single CertificateEntry with one stub cert and zero exts. + withUint24Length { + writeTlsOpaque3(byteArrayOf(0x30, 0x00)) // minimal DER-ish placeholder + writeTlsOpaque2(ByteArray(0)) // per-cert extensions + } + } + return w.toByteArray() + } + + /** Encode a CertificateVerify message with a fake RSA-PSS-SHA256 signature. */ + private fun buildCertificateVerifyStub(): ByteArray { + val w = QuicWriter() + w.writeByte(TlsConstants.HS_CERTIFICATE_VERIFY) + w.withUint24Length { + writeUint16(TlsConstants.SIG_RSA_PSS_RSAE_SHA256) + writeTlsOpaque2(ByteArray(64)) // any bytes — Permissive accepts + } + return w.toByteArray() + } } diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/TlsRoundTripTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/TlsRoundTripTest.kt index 5d9d523620..4e7929a035 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/TlsRoundTripTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/TlsRoundTripTest.kt @@ -49,7 +49,7 @@ class TlsRoundTripTest { serverName = "example.test", transportParameters = ByteArray(0), secretsListener = capturedSecrets, - certificateValidator = null, // in-process loopback; no cert chain to validate + certificateValidator = PermissiveCertificateValidator(), ) client.start() @@ -63,14 +63,14 @@ class TlsRoundTripTest { assertNotNull(sh, "server should produce ServerHello at Initial level") client.pushHandshakeBytes(TlsClient.Level.INITIAL, sh) - // 3) Drain EncryptedExtensions + Finished (Handshake level) → client - val ee = server.pollOutboundHandshake() - assertNotNull(ee, "server should produce EncryptedExtensions") - client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, ee) - - val sf = server.pollOutboundHandshake() - assertNotNull(sf, "server should produce server Finished") - client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, sf) + // 3) Drain EncryptedExtensions + Certificate + CertificateVerify + + // Finished (Handshake level) → client. The InProcessTlsServer now + // emits all four (audit-4 #3 — TlsClient hard-fails any non-PSK + // handshake that omits Certificate/CertificateVerify). + while (true) { + val msg = server.pollOutboundHandshake() ?: break + client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, msg) + } // 4) Drain client Finished → server val cf = client.pollOutbound(TlsClient.Level.HANDSHAKE) @@ -109,15 +109,18 @@ class TlsRoundTripTest { serverName = "example.test", transportParameters = ByteArray(0), secretsListener = capturedSecrets, - certificateValidator = null, + certificateValidator = PermissiveCertificateValidator(), ) client.start() val ch = client.pollOutbound(TlsClient.Level.INITIAL)!! server.receiveClientHello(ch) client.pushHandshakeBytes(TlsClient.Level.INITIAL, server.pollOutboundInitial()!!) - client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, server.pollOutboundHandshake()!!) - client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, server.pollOutboundHandshake()!!) + // EE + Certificate + CertificateVerify + Finished (audit-4 #3). + while (true) { + val msg = server.pollOutboundHandshake() ?: break + client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, msg) + } server.receiveClientFinished(client.pollOutbound(TlsClient.Level.HANDSHAKE)!!) assertEquals(TlsConstants.CIPHER_TLS_CHACHA20_POLY1305_SHA256, server.negotiatedCipherSuite) diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/webtransport/CapsuleReaderTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/webtransport/CapsuleReaderTest.kt index 454d7eada9..3588679592 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/webtransport/CapsuleReaderTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/webtransport/CapsuleReaderTest.kt @@ -82,15 +82,30 @@ class CapsuleReaderTest { } @Test - fun decodes_empty_body_close_session_as_zero_error_empty_reason() { - // Spec lower bound: body length 0, no error code present. - val empty = encodeCapsule(WtCapsuleType.WT_CLOSE_SESSION, ByteArray(0)) + fun rejects_close_session_with_truncated_body_below_4_bytes() { + // Audit-4 #13: body shorter than the mandatory 4-byte error_code + // field is malformed; the decoder MUST surface this rather than + // synthesising a `WtCloseSession(0, "")` that the application can't + // distinguish from a clean close. + val truncated = encodeCapsule(WtCapsuleType.WT_CLOSE_SESSION, ByteArray(0)) val reader = CapsuleReader() - reader.push(empty) - val parsed = reader.next() - assertIs(parsed) - assertEquals(0, parsed.errorCode) - assertEquals("", parsed.reason) + reader.push(truncated) + kotlin.test.assertFailsWith { + reader.next() + } + } + + @Test + fun rejects_close_session_with_oversized_reason() { + // Audit-4 #14: draft-ietf-webtrans-http3 §5 caps the reason at 8192 + // bytes. We reject overlong reasons rather than passing them on. + val body = ByteArray(4 + 8193) // 4 bytes error code + 8193-byte reason + val capsule = encodeCapsule(WtCapsuleType.WT_CLOSE_SESSION, body) + val reader = CapsuleReader() + reader.push(capsule) + kotlin.test.assertFailsWith { + reader.next() + } } @Test 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 00e8e7504e..92a2597a3a 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt @@ -113,12 +113,21 @@ class JdkCertificateValidator( private fun jcaSignatureFor(algorithm: Int): Signature = when (algorithm) { TlsConstants.SIG_ECDSA_SECP256R1_SHA256 -> Signature.getInstance("SHA256withECDSA") + TlsConstants.SIG_ECDSA_SECP384R1_SHA384 -> Signature.getInstance("SHA384withECDSA") + 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_SHA512 -> rsaPss("SHA-512", 64) - TlsConstants.SIG_RSA_PKCS1_SHA256 -> Signature.getInstance("SHA256withRSA") + TlsConstants.SIG_ED25519 -> Signature.getInstance("Ed25519") + + // 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)}") } @@ -139,10 +148,17 @@ class JdkCertificateValidator( // Normalize host once: IDN → ASCII for DNS comparison, parsed-and- // re-stringified for IP literals so v6 forms compare equal. val normalizedHost = idnAscii(host) + // Audit-4 #4: do NOT call InetAddress.getByName on a hostname — it + // performs a DNS A/AAAA lookup, leaking the hostname over plaintext + // DNS at the TLS-validation step. Only resolve confirmed IP literals. val hostAsIp = - try { - InetAddress.getByName(host).hostAddress - } catch (_: Throwable) { + if (looksLikeIpLiteral(host)) { + try { + InetAddress.getByName(host).hostAddress + } catch (_: Throwable) { + null + } + } else { null } for (entry in sans) { @@ -170,6 +186,19 @@ class JdkCertificateValidator( name.lowercase() } + /** + * Pattern-match check for IPv4 / IPv6 literals so we don't trigger DNS + * lookups for hostnames during cert validation (audit-4 #4). IPv4 is + * "digits and dots only"; IPv6 is "contains a colon". Bracketed IPv6 + * literals (`[::1]`) are accepted by stripping the brackets first. + */ + private fun looksLikeIpLiteral(host: String): Boolean { + val unbracketed = + if (host.startsWith("[") && host.endsWith("]")) host.substring(1, host.length - 1) else host + if (unbracketed.contains(':')) return true // IPv6 + return unbracketed.all { it.isDigit() || it == '.' } && unbracketed.contains('.') + } + private fun dnsMatches( pattern: String, host: String,