From ce8d852518c21eeba1ee6852abf07f64c8fae5fc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 23:57:34 +0000 Subject: [PATCH] =?UTF-8?q?fix(quic):=20final=20stabilization=20sweep=20?= =?UTF-8?q?=E2=80=94=20CID=20picking,=20atomic=20gates,=20defensive=20chec?= =?UTF-8?q?ks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 of the audit follow-ups. The remaining low-leverage items from the original audit all addressed in one pass. * PathValidator: pick the smallest spare CID sequence number rather than LinkedHashMap insertion order. RFC 9000 §19.15 lets the peer issue NEW_CONNECTION_ID out of sequence (e.g. retransmits arriving after newer offers); insertion-order picking would land on whichever offer arrived first instead of the lowest seq, drifting away from the RFC-expected ordering. forceRotateToHigherSequence also now filters >= retirePriorToWatermark explicitly so we never pick a sequence below the watermark even if the pool somehow holds one. * QuicWebTransportSessionState.close: driver.wakeup() AFTER enqueuing the WT_CLOSE_SESSION capsule + FIN but BEFORE driver.close, so the capsule actually reaches the wire instead of being short-circuited by the driver shutdown. Pre-fix the peer saw an abrupt UDP-level tear-down with no application-error-code surfaced. * QuicStream.resetStream / stopSending: synchronized(this) atomic CAS for the "first call wins" gate. Pre-fix two concurrent callers could both observe `resetState == null` and both write — the second caller's errorCode would clobber the first while resetEmitPending was already set, so the writer emitted the RESET_STREAM with whichever value landed last. * Http3Settings.decodeBody: per-id value range checks. A peer that advertises e.g. MAX_FIELD_SECTION_SIZE = 2^60 could otherwise drive our encoder into unbounded heap. Bounds chosen above any legitimate value (1 GiB for table-capacity / field-section caps, 1 for boolean flags) and below 2^32 for unknown ids. * Privatize crypto-relevant static byte arrays: InitialSecrets.V1_INITIAL_SALT, RetryPacket.V1_RETRY_KEY, RetryPacket.V1_RETRY_NONCE. Pre-fix these were public mutable ByteArrays — any caller could stomp on them, and any toString / reflection would leak the bytes. Crypto material doesn't need to be reachable outside the deriving / sealing path. * peekHeader length cast: re-verified as already safe (lengthRaw is bounded by r.remaining before .toInt() — Int.MAX_VALUE ceiling enforced). All 269 :quic:jvmTest tests pass. BUILD SUCCESSFUL in 2m 17s. https://claude.ai/code/session_01AhGvbMV8uPRse3TmAGaddM --- .../quic/connection/PathValidator.kt | 21 +++++++- .../quic/crypto/InitialSecrets.kt | 9 +++- .../vitorpamplona/quic/http3/Http3Settings.kt | 52 +++++++++++++++++++ .../vitorpamplona/quic/packet/RetryPacket.kt | 16 ++++-- .../vitorpamplona/quic/stream/QuicStream.kt | 36 +++++++++---- .../QuicWebTransportSessionState.kt | 7 +++ 6 files changed, 124 insertions(+), 17 deletions(-) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt index 8ed2a7289..eb6534bbf 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt @@ -430,7 +430,15 @@ class PathValidator( currentPtoMillis: Long, ): PathMigrationResult { if (state is PathValidationState.Validating) return PathMigrationResult.AlreadyInProgress - val (seq, entry) = unusedCids.entries.firstOrNull() ?: return PathMigrationResult.NoSpareCid + // Pick the SMALLEST sequence number, not insertion order. The + // peer is allowed (RFC 9000 §19.15) to issue NEW_CONNECTION_ID + // out of sequence — e.g. retransmits arriving after newer + // higher-seq offers. LinkedHashMap iteration is by insertion, + // so the prior `firstOrNull` would pick whichever offer landed + // first, not the lowest seq. Picking the smallest preserves + // RFC's expected ordering (lower seq retired first) and lines + // up with what other clients (quicly, neqo) do. + val (seq, entry) = unusedCids.entries.minByOrNull { it.key } ?: return PathMigrationResult.NoSpareCid unusedCids.remove(seq) val payload = challengePayloadFactory().also { @@ -580,7 +588,16 @@ class PathValidator( */ fun forceRotateToHigherSequence(): ForcedRotationResult? { if (activeCidSequence >= retirePriorToWatermark) return null - val (seq, entry) = unusedCids.entries.firstOrNull() ?: return ForcedRotationResult.NoSpareCid + // Pick the smallest seq above the watermark — see + // [tryStartValidation]'s rationale. LinkedHashMap is insertion- + // ordered, not seq-ordered, so a peer that retransmits an old + // offer can shift the "first" entry away from the actual + // smallest. + val (seq, entry) = + unusedCids.entries + .filter { it.key >= retirePriorToWatermark } + .minByOrNull { it.key } + ?: return ForcedRotationResult.NoSpareCid unusedCids.remove(seq) val priorSeq = activeCidSequence queueRetireSequence(priorSeq) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/InitialSecrets.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/InitialSecrets.kt index 66051349b..cd5b30dd6 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/InitialSecrets.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/InitialSecrets.kt @@ -37,7 +37,14 @@ package com.vitorpamplona.quic.crypto * protection epoch. */ object InitialSecrets { - val V1_INITIAL_SALT: ByteArray = + /** + * RFC 9001 §5.2 fixed Initial-secret salt for QUIC v1. Private + only + * read internally by [derive] — pre-fix the constant was a public + * mutable [ByteArray] that any caller could stomp on (or that any + * stack trace / `toString()` could leak). Crypto material doesn't + * need to be reachable outside the derive path. + */ + private val V1_INITIAL_SALT: ByteArray = byteArrayOf( 0x38.toByte(), 0x76.toByte(), 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 212e4dbc3..cab63e806 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/http3/Http3Settings.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/http3/Http3Settings.kt @@ -63,10 +63,62 @@ data class Http3Settings( "duplicate HTTP/3 SETTINGS id 0x${id.toString(16)}", ) } + // RFC 9114 §7.2.4.1 / RFC 9204 §5: per-id range checks. + // A peer that advertises e.g. `MAX_FIELD_SECTION_SIZE = + // 2^60` could otherwise drive the encoder into + // unbounded heap (we'd emit headers up to that cap in a + // single allocation). Bounds chosen to comfortably + // exceed any legitimate value while staying inside + // [Int.MAX_VALUE] for safe `.toInt()` casts at use + // sites. + validateValue(id, value) map[id] = value } return Http3Settings(map) } + + /** + * Reject obviously-malicious SETTINGS values per the relevant + * RFCs. Negative varints are already impossible (varint is + * unsigned), but we double-check defensively. + */ + private fun validateValue( + id: Long, + value: Long, + ) { + if (value < 0L) { + throw com.vitorpamplona.quic.QuicCodecException( + "negative HTTP/3 SETTINGS value for id 0x${id.toString(16)}: $value", + ) + } + // Per-id sanity caps. Unknown ids fall through (RFC 9114 + // §7.2.4.1 says unknown SETTINGS MUST be ignored, but we + // still bound the value to avoid attacker-controlled + // long-tail allocation if any consumer ever uses the + // unknown id directly). + val cap: Long = + when (id) { + Http3SettingsId.QPACK_MAX_TABLE_CAPACITY -> 1L shl 30 + + // 1 GiB + Http3SettingsId.MAX_FIELD_SECTION_SIZE -> 1L shl 30 + + Http3SettingsId.QPACK_BLOCKED_STREAMS -> 65535L + + Http3SettingsId.ENABLE_CONNECT_PROTOCOL -> 1L + + Http3SettingsId.H3_DATAGRAM -> 1L + + Http3SettingsId.ENABLE_WEBTRANSPORT -> 1L + + else -> 1L shl 32 // generic cap for unknown ids + } + if (value > cap) { + throw com.vitorpamplona.quic.QuicCodecException( + "HTTP/3 SETTINGS id 0x${id.toString(16)} value $value exceeds cap $cap", + ) + } + } } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/RetryPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/RetryPacket.kt index b3e78a76e..5a3bdf511 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/RetryPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/RetryPacket.kt @@ -107,8 +107,13 @@ data class RetryPacket( ) } - /** RFC 9001 §5.8 — fixed retry-integrity AES-128-GCM key for QUIC v1. */ - val V1_RETRY_KEY: ByteArray = + /** + * RFC 9001 §5.8 — fixed retry-integrity AES-128-GCM key for QUIC v1. + * Private — only read by [computeIntegrityTag] inside this file. + * Pre-fix exposing the public mutable [ByteArray] let any caller + * stomp on it or surface it via reflection / `toString()`. + */ + private val V1_RETRY_KEY: ByteArray = byteArrayOf( 0xbe.toByte(), 0x0c.toByte(), @@ -128,8 +133,11 @@ data class RetryPacket( 0x4e.toByte(), ) - /** RFC 9001 §5.8 — fixed retry-integrity AES-128-GCM nonce for QUIC v1. */ - val V1_RETRY_NONCE: ByteArray = + /** + * RFC 9001 §5.8 — fixed retry-integrity AES-128-GCM nonce for QUIC v1. + * Private; same rationale as [V1_RETRY_KEY]. + */ + private val V1_RETRY_NONCE: ByteArray = byteArrayOf( 0x46.toByte(), 0x15.toByte(), 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 46d1cd888..ef410099c 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt @@ -220,13 +220,26 @@ class QuicStream( * (two app threads racing the writer's clear-after-emit). */ fun resetStream(errorCode: Long) { - if (resetState != null) return - resetState = - ResetState( - errorCode = errorCode, - finalSize = send.nextOffset, - ) - resetEmitPending = true + // Synchronized atomic compare-and-set: pre-fix the + // `if (resetState != null) return` plus the assignment was + // racy. Two concurrent callers (e.g. the application aborting + // a request while STOP_SENDING from the peer triggers our own + // resetStream from the parser) could both observe null and + // both write — the second write would clobber the first + // errorCode while [resetEmitPending] was already set. The + // writer would then emit a RESET_STREAM with whichever + // errorCode landed last, possibly different from what the + // application asked for. The synchronized block makes + // first-call-wins genuinely first-call-wins. + synchronized(this) { + if (resetState != null) return + resetState = + ResetState( + errorCode = errorCode, + finalSize = send.nextOffset, + ) + resetEmitPending = true + } } /** @@ -244,9 +257,12 @@ class QuicStream( * original frame already on the wire). */ fun stopSending(errorCode: Long) { - if (stopSendingState != null) return - stopSendingState = StopSendingState(errorCode = errorCode) - stopSendingEmitPending = true + // Same atomic-CAS rationale as [resetStream]. + synchronized(this) { + if (stopSendingState != null) return + stopSendingState = StopSendingState(errorCode = errorCode) + stopSendingEmitPending = true + } } /** 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 e9eac528c..7f1fe1638 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/QuicWebTransportSessionState.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/webtransport/QuicWebTransportSessionState.kt @@ -223,6 +223,13 @@ class QuicWebTransportSessionState( connection.streamById(connectStreamId)?.let { it.send.enqueue(encodeCloseSessionCapsule(errorCode, reason)) it.send.finish() + // Wake the driver BEFORE asking it to close — otherwise + // [driver.close] may short-circuit the send loop before our + // newly-enqueued WT_CLOSE_SESSION capsule + FIN drain to + // the wire. The peer would then see an abrupt UDP-level + // tear-down instead of the graceful WT close, and any + // application-error-code we wanted to deliver is lost. + driver.wakeup() } driver.close() // Round-5 concurrency #1: cancel the WT scope so the demux pump