fix(quic): RFC 9000 §18.2 transport-param bounds + §13.2.5 ack-delay-exponent + §7.2.4.1 reserved SETTINGS + RFC 9221 §3 outbound DATAGRAM size

Five spec-compliance fixes from the 2026-05-09 audit, all in the same "hostile-peer hardening" tier. * RFC 9000 §18.2 — `applyPeerTransportParameters` now closes the connection with TRANSPORT_PARAMETER_ERROR when the peer advertises `max_udp_payload_size < 1200`, `ack_delay_exponent > 20`, or `active_connection_id_limit < 2`. Pre-fix the values were decoded but never bounds-checked. * RFC 9000 §13.2.5 — the parser's ACK-delay decoding now uses the PEER's advertised `ack_delay_exponent` (with the §18.2 default of 3 as the pre-handshake fallback) instead of our own config value. Defensive coercion to 0..20 is preserved so a bypass of the bounds check above can't desync the RTT estimator. * RFC 9114 §7.2.4.1 — `Http3Settings.decodeBody` now rejects the HTTP/2-reserved SETTINGS ids 0x02, 0x03, 0x04, 0x05 with H3_SETTINGS_ERROR. Pre-fix they fell through to the generic 1<<32 cap and were accepted. * RFC 9221 §3 — the writer now drops outbound DATAGRAM frames when the peer didn't advertise `max_datagram_frame_size` (or advertised 0), or when the encoded frame (type byte + length varint + payload) would exceed the peer's advertised value. Pre-fix the writer emitted DATAGRAM regardless and let spec-conformant peers close us with PROTOCOL_VIOLATION. * Added `TransportParameterDefaults` for the §18.2 defaults (ack_delay_exponent=3, max_ack_delay=25ms, active_connection_id_limit=2) so callers don't hard-code the magic numbers. Tests: `TransportParameterBoundsTest` (8 cases) drives a real handshake through the in-process pipe and asserts CLOSED on each violation + CONNECTED at the boundary; `Http3ReservedSettingsTest` (5 cases) covers the four reserved ids and the adjacent legal ids. Plan tier dropped from 🟡 Medium to resolved for all five items. https://claude.ai/code/session_01XGmmaVuy2wsSZQx2AqN2ik
2026-05-09 04:37:50 +00:00
parent a80f4ea19d
commit 657306392d
7 changed files with 379 additions and 3 deletions
@@ -1132,6 +1132,42 @@ class QuicConnection(
            )
            return
        }
+        // RFC 9000 §18.2 bounds checks. A peer that violates these is in
+        // protocol violation and the connection MUST close with
+        // TRANSPORT_PARAMETER_ERROR.
+        //
+        //  - max_udp_payload_size: minimum 1200 (the §14 datagram size
+        //    floor). A smaller value would force fragmented Initials,
+        //    which our writer can't produce.
+        //  - ack_delay_exponent: maximum 20. Beyond that, the
+        //    `ackDelay << exponent` shift in the parser overflows even
+        //    with the clamp.
+        //  - active_connection_id_limit: minimum 2. A value < 2 leaves
+        //    the peer no spare CID to migrate to.
+        tp.maxUdpPayloadSize?.let { v ->
+            if (v < 1200L) {
+                markClosedExternally(
+                    "TRANSPORT_PARAMETER_ERROR: max_udp_payload_size $v < 1200",
+                )
+                return
+            }
+        }
+        tp.ackDelayExponent?.let { v ->
+            if (v > 20L) {
+                markClosedExternally(
+                    "TRANSPORT_PARAMETER_ERROR: ack_delay_exponent $v > 20",
+                )
+                return
+            }
+        }
+        tp.activeConnectionIdLimit?.let { v ->
+            if (v < 2L) {
+                markClosedExternally(
+                    "TRANSPORT_PARAMETER_ERROR: active_connection_id_limit $v < 2",
+                )
+                return
+            }
+        }
        peerTransportParameters = tp
        qlogObserver.onTransportParametersSet("remote", peerTransportParametersSummary(tp))
        sendConnectionFlowCredit = tp.initialMaxData ?: 0L
@@ -550,8 +550,19 @@ private fun dispatchFrames(
                    // before consuming. Per RFC 9000 §18.2 the exponent is
                    // 0..20; we further clamp ackDelay so the shift never
                    // overflows (`ackDelay <= Long.MAX_VALUE >>> exponent`).
-                    val rawExponent = conn.config.ackDelayExponent
-                    val exponent = rawExponent.coerceIn(0L, 20L).toInt()
+                    //
+                    // RFC 9000 §13.2.5: a received ACK is decoded using the
+                    // PEER's `ack_delay_exponent`, not ours. Pre-handshake
+                    // peer params haven't arrived yet — fall back to the
+                    // default of 3 per §18.2. Coercion below ensures even
+                    // a malicious peer that smuggles a >20 value through
+                    // the §18.2 bounds check (e.g. on a connection where
+                    // the peer-params bounds enforcement is bypassed by a
+                    // race) can't desync our RTT estimator.
+                    val peerExponent =
+                        conn.peerTransportParameters?.ackDelayExponent
+                            ?: TransportParameterDefaults.ACK_DELAY_EXPONENT
+                    val exponent = peerExponent.coerceIn(0L, 20L).toInt()
                    val maxAckDelayPreShift =
                        if (exponent == 0) Long.MAX_VALUE else Long.MAX_VALUE ushr exponent
                    val safeAckDelay = frame.ackDelay.coerceIn(0L, maxAckDelayPreShift)
@@ -669,9 +669,36 @@ private fun buildApplicationPacket(
    // stream and MAX_DATA at the connection level.
    appendFlowControlUpdates(conn, frames, tokens)

-    // Pending datagrams
+    // Pending datagrams. RFC 9221 §3 enforcement:
+    //   - if the peer didn't advertise `max_datagram_frame_size` (or
+    //     advertised 0), DATAGRAM MUST NOT be sent — drop with diagnostic;
+    //   - if the encoded frame would exceed the peer's advertised
+    //     `max_datagram_frame_size` (frame type byte + length varint +
+    //     payload), drop with diagnostic.
+    // Pre-fix the writer emitted DATAGRAM regardless and let
+    // spec-conformant peers close the connection with PROTOCOL_VIOLATION.
+    val peerDatagramCap = conn.peerTransportParameters?.maxDatagramFrameSize ?: 0L
    while (conn.pendingDatagramsLocked().isNotEmpty()) {
        val payload = conn.pendingDatagramsLocked().removeFirst()
+        if (peerDatagramCap <= 0L) {
+            conn.qlogObserver.onPacketDropped(
+                "outbound DATAGRAM dropped — peer did not advertise max_datagram_frame_size",
+                payload.size,
+            )
+            continue
+        }
+        // Frame total = 1 byte type (0x31) + varint(payload.size) + payload.
+        val frameSize =
+            1 +
+                com.vitorpamplona.quic.Varint
+                    .size(payload.size.toLong()) + payload.size
+        if (frameSize > peerDatagramCap) {
+            conn.qlogObserver.onPacketDropped(
+                "outbound DATAGRAM dropped — frame size $frameSize > peer cap $peerDatagramCap",
+                payload.size,
+            )
+            continue
+        }
        frames += DatagramFrame(payload, explicitLength = true)
        if (frames.size >= 16) break
    }
@@ -52,6 +52,23 @@ object TransportParameterId {
    const val MAX_DATAGRAM_FRAME_SIZE: Long = 0x20
 }

+/**
+ * RFC 9000 §18.2 default values for parameters that are NOT advertised
+ * by the peer. Surfacing them as constants so the parser / writer can
+ * use them as the "peer didn't tell us" fallback without each call site
+ * hard-coding the numeric default.
+ */
+object TransportParameterDefaults {
+    /** §18.2: default `ack_delay_exponent` is 3 if not advertised. */
+    const val ACK_DELAY_EXPONENT: Long = 3L
+
+    /** §18.2: default `max_ack_delay` is 25 ms if not advertised. */
+    const val MAX_ACK_DELAY_MS: Long = 25L
+
+    /** §18.2: default `active_connection_id_limit` is 2 if not advertised. */
+    const val ACTIVE_CONNECTION_ID_LIMIT: Long = 2L
+}
+
 /**
 * QUIC transport parameters as exchanged inside the TLS QUIC transport_params
 * extension.
@@ -86,6 +86,15 @@ data class Http3Settings(
            id: Long,
            value: Long,
        ) {
+            // RFC 9114 §7.2.4.1: SETTINGS identifiers in the HTTP/2 range
+            // (0x02, 0x03, 0x04, 0x05) are reserved to prevent confusion
+            // with HTTP/2 settings — receipt MUST be a connection error of
+            // type H3_SETTINGS_ERROR. Pre-fix we accepted them silently.
+            if (id == 0x02L || id == 0x03L || id == 0x04L || id == 0x05L) {
+                throw com.vitorpamplona.quic.QuicCodecException(
+                    "H3_SETTINGS_ERROR: reserved HTTP/2 SETTINGS id 0x${id.toString(16)}",
+                )
+            }
            if (value < 0L) {
                throw com.vitorpamplona.quic.QuicCodecException(
                    "negative HTTP/3 SETTINGS value for id 0x${id.toString(16)}: $value",
@@ -0,0 +1,188 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.quic.connection
+
+import com.vitorpamplona.quic.tls.InProcessTlsServer
+import com.vitorpamplona.quic.tls.PermissiveCertificateValidator
+import kotlinx.coroutines.runBlocking
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNotNull
+import kotlin.test.assertTrue
+
+/**
+ * RFC 9000 §18.2 transport-parameter bounds checks. These values are
+ * advertised by the peer in their EncryptedExtensions
+ * `quic_transport_parameters` extension; out-of-range values MUST close
+ * the connection with TRANSPORT_PARAMETER_ERROR.
+ *
+ *  - `max_udp_payload_size` minimum 1200 (the §14 datagram floor).
+ *  - `ack_delay_exponent` maximum 20.
+ *  - `active_connection_id_limit` minimum 2.
+ *
+ * Pre-fix the values were decoded into [TransportParameters] but no
+ * runtime check enforced the spec ranges — a hostile peer could ship a
+ * value of e.g. `ack_delay_exponent = 60` and our parser's
+ * `ackDelay << 60` shift would have desynced RTT (the parser still
+ * caps the exponent defensively, but the connection should not have
+ * been allowed in the first place).
+ */
+class TransportParameterBoundsTest {
+    private fun runHandshake(serverParams: TransportParameters): QuicConnection.Status =
+        runBlocking {
+            val client =
+                QuicConnection(
+                    serverName = "example.test",
+                    config = QuicConnectionConfig(),
+                    tlsCertificateValidator = PermissiveCertificateValidator(),
+                )
+            val serverScid = ConnectionId.random(8)
+            val tlsServer =
+                InProcessTlsServer(
+                    transportParameters =
+                        serverParams
+                            .copy(
+                                initialSourceConnectionId = serverScid.bytes,
+                                originalDestinationConnectionId = client.destinationConnectionId.bytes,
+                            ).encode(),
+                )
+            val pipe =
+                InMemoryQuicPipe(
+                    client = client,
+                    initialDcid = client.destinationConnectionId.bytes,
+                    serverScid = serverScid,
+                    tlsServer = tlsServer,
+                )
+            client.start()
+            // Drive enough rounds for ServerHello + EE + handshake completion.
+            // Once peer params are applied, `applyPeerTransportParameters`
+            // runs the §18.2 bounds checks which (on violation) flip the
+            // connection to CLOSED.
+            pipe.drive(maxRounds = 16)
+            client.status
+        }
+
+    private fun baselineLegalParams() =
+        TransportParameters(
+            initialMaxData = 1L * 1024 * 1024,
+            initialMaxStreamDataBidiLocal = 64L * 1024,
+            initialMaxStreamDataBidiRemote = 64L * 1024,
+            initialMaxStreamDataUni = 64L * 1024,
+            initialMaxStreamsBidi = 16,
+            initialMaxStreamsUni = 16,
+        )
+
+    @Test
+    fun max_udp_payload_size_below_1200_closes() {
+        val status = runHandshake(baselineLegalParams().copy(maxUdpPayloadSize = 1199L))
+        assertEquals(
+            QuicConnection.Status.CLOSED,
+            status,
+            "RFC 9000 §18.2: max_udp_payload_size < 1200 MUST be TRANSPORT_PARAMETER_ERROR",
+        )
+    }
+
+    @Test
+    fun max_udp_payload_size_at_1200_accepted() {
+        val status = runHandshake(baselineLegalParams().copy(maxUdpPayloadSize = 1200L))
+        assertEquals(QuicConnection.Status.CONNECTED, status)
+    }
+
+    @Test
+    fun ack_delay_exponent_above_20_closes() {
+        val status = runHandshake(baselineLegalParams().copy(ackDelayExponent = 21L))
+        assertEquals(
+            QuicConnection.Status.CLOSED,
+            status,
+            "RFC 9000 §18.2: ack_delay_exponent > 20 MUST be TRANSPORT_PARAMETER_ERROR",
+        )
+    }
+
+    @Test
+    fun ack_delay_exponent_at_20_accepted() {
+        val status = runHandshake(baselineLegalParams().copy(ackDelayExponent = 20L))
+        assertEquals(QuicConnection.Status.CONNECTED, status)
+    }
+
+    @Test
+    fun active_connection_id_limit_below_2_closes() {
+        val status = runHandshake(baselineLegalParams().copy(activeConnectionIdLimit = 1L))
+        assertEquals(
+            QuicConnection.Status.CLOSED,
+            status,
+            "RFC 9000 §18.2: active_connection_id_limit < 2 MUST be TRANSPORT_PARAMETER_ERROR",
+        )
+    }
+
+    @Test
+    fun active_connection_id_limit_at_2_accepted() {
+        val status = runHandshake(baselineLegalParams().copy(activeConnectionIdLimit = 2L))
+        assertEquals(QuicConnection.Status.CONNECTED, status)
+    }
+
+    @Test
+    fun missing_optional_params_accepted() {
+        // No bounds-checked params advertised → fall back to defaults,
+        // connection completes normally.
+        val status = runHandshake(baselineLegalParams())
+        assertEquals(QuicConnection.Status.CONNECTED, status)
+    }
+
+    @Test
+    fun close_reason_mentions_specific_violated_param() {
+        // Capture which bound was reported so future audit rounds can
+        // confirm the message contains a usable diagnostic, not just a
+        // generic "transport params bad".
+        val client =
+            runBlocking {
+                val client =
+                    QuicConnection(
+                        serverName = "example.test",
+                        config = QuicConnectionConfig(),
+                        tlsCertificateValidator = PermissiveCertificateValidator(),
+                    )
+                val serverScid = ConnectionId.random(8)
+                val tlsServer =
+                    InProcessTlsServer(
+                        transportParameters =
+                            baselineLegalParams()
+                                .copy(
+                                    initialSourceConnectionId = serverScid.bytes,
+                                    originalDestinationConnectionId = client.destinationConnectionId.bytes,
+                                    ackDelayExponent = 25L,
+                                ).encode(),
+                    )
+                val pipe =
+                    InMemoryQuicPipe(
+                        client = client,
+                        initialDcid = client.destinationConnectionId.bytes,
+                        serverScid = serverScid,
+                        tlsServer = tlsServer,
+                    )
+                client.start()
+                pipe.drive(maxRounds = 16)
+                client
+            }
+        val reason = client.closeReason
+        assertNotNull(reason)
+        assertTrue(reason.contains("ack_delay_exponent"), "reason should name the violated param: $reason")
+    }
+}
@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.quic.http3
+
+import com.vitorpamplona.quic.QuicCodecException
+import com.vitorpamplona.quic.QuicWriter
+import kotlin.test.Test
+import kotlin.test.assertFailsWith
+import kotlin.test.assertTrue
+
+/**
+ * RFC 9114 §7.2.4.1 — SETTINGS identifiers in the HTTP/2 range
+ * (0x02, 0x03, 0x04, 0x05) are reserved to prevent confusion with
+ * HTTP/2 settings. Receipt of any of these MUST be a connection error
+ * of type H3_SETTINGS_ERROR.
+ *
+ * Pre-fix the decoder validated values per known id but accepted the
+ * reserved ids silently (since they weren't in the known cap table —
+ * they fell through to the generic 1<<32 cap).
+ */
+class Http3ReservedSettingsTest {
+    private fun encodeRawSetting(
+        id: Long,
+        value: Long,
+    ): ByteArray {
+        val w = QuicWriter()
+        w.writeVarint(id)
+        w.writeVarint(value)
+        return w.toByteArray()
+    }
+
+    @Test
+    fun reserved_id_0x02_rejected() {
+        val ex =
+            assertFailsWith<QuicCodecException> {
+                Http3Settings.decodeBody(encodeRawSetting(0x02L, 1L))
+            }
+        assertTrue(ex.message!!.contains("H3_SETTINGS_ERROR"))
+    }
+
+    @Test
+    fun reserved_id_0x03_rejected() {
+        assertFailsWith<QuicCodecException> {
+            Http3Settings.decodeBody(encodeRawSetting(0x03L, 1L))
+        }
+    }
+
+    @Test
+    fun reserved_id_0x04_rejected() {
+        assertFailsWith<QuicCodecException> {
+            Http3Settings.decodeBody(encodeRawSetting(0x04L, 1L))
+        }
+    }
+
+    @Test
+    fun reserved_id_0x05_rejected() {
+        assertFailsWith<QuicCodecException> {
+            Http3Settings.decodeBody(encodeRawSetting(0x05L, 1L))
+        }
+    }
+
+    @Test
+    fun adjacent_legal_ids_still_accepted() {
+        // 0x01 is RFC 9204 QPACK_MAX_TABLE_CAPACITY, 0x06 is the next
+        // legal HTTP/3 setting. Confirm the reserved-range check does
+        // not over-reach.
+        Http3Settings.decodeBody(encodeRawSetting(0x01L, 4096L))
+        Http3Settings.decodeBody(encodeRawSetting(0x06L, 1024L))
+    }
+}