fix(quic): final stabilization sweep — CID picking, atomic gates, defensive checks

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
2026-05-08 23:57:34 +00:00
parent e4d96421c2
commit ce8d852518
6 changed files with 124 additions and 17 deletions
@@ -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)
@@ -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(),
@@ -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",
+                )
+            }
+        }
    }
 }

@@ -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(),
@@ -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
+        }
    }

    /**
@@ -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