From 59438d528355e0cf6732bee13f95d4dac5a8ab8d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 22:37:55 +0000 Subject: [PATCH] =?UTF-8?q?fix(quic):=20RFC=209000=20=C2=A710.2.3=20+=20?= =?UTF-8?q?=C2=A714.1=20in=20CONNECTION=5FCLOSE-only=20datagrams?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The quic-interop-runner against aioquic surfaced two real bugs in the writer's CLOSING-status branch, both visible on the wire as a single ~45-byte UDP datagram instead of a properly framed close. §10.2.3 — at Initial / Handshake levels only CONNECTION_CLOSE (Transport, 0x1c) is allowed. The application-level form (0x1d) leaks app state pre-handshake. The writer was unconditionally building a 0x1d frame and shipping it inside an Initial packet; aioquic dropped it silently. §14.1 — any client datagram containing an Initial MUST be ≥ 1200 bytes in UDP-payload terms. The CLOSING branch bypassed the existing padding logic, so close-only Initial datagrams went out at ~45 bytes and servers correctly rejected them (also as malformed). Fix replaces the CLOSING branch with a dedicated `buildClosingDatagram` helper: - Application keys present → 0x1d, original error code + reason. - Pre-1-RTT (Handshake or Initial) → 0x1c with errorCode = APPLICATION_ERROR (0x0c), frameType=0, empty reason. - Initial level: build at natural size, rewind PN if < 1200, rebuild with PADDING-frame deficit inside the AEAD envelope. Plus a regression test covering both: pre-handshake close datagram size ≥ 1200, and ConnectionCloseFrame round-trips 0x1c vs 0x1d for the right constructor inputs. NOT addressed yet: why our ClientHello at PN=0 doesn't appear in the runner pcap (only PN=1 close does). With this fix the close packet is now well-formed; the next runner run will tell us whether the missing ClientHello is a sim/capture artifact or a separate writer bug. https://claude.ai/code/session_01HcvfQq1ttPV9PkRoJb4nyT --- .../quic/connection/QuicConnectionWriter.kt | 91 ++++++++++++++++++- .../CloseDatagramRfcComplianceTest.kt | 91 +++++++++++++++++++ 2 files changed, 178 insertions(+), 4 deletions(-) create mode 100644 quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt 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 96bd15f8f..b6ca3f7c2 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -62,12 +62,19 @@ fun drainOutbound( ): ByteArray? { val parts = mutableListOf() - // Closing — emit a CONNECTION_CLOSE at the highest available level. + // Closing — emit a CONNECTION_CLOSE at the highest available level. The + // datagram-build paths below MUST satisfy two RFC 9000 constraints we + // got wrong before: + // - §10.2.3: at Initial / Handshake levels, only CONNECTION_CLOSE + // (Transport, 0x1c) is allowed; the application-level close (0x1d) is + // forbidden because app state would leak before the handshake is + // encrypted with the application key. + // - §14.1: a client datagram containing an Initial MUST be ≥ 1200 bytes + // in UDP-payload terms, even when carrying only a CONNECTION_CLOSE. if (conn.status == QuicConnection.Status.CLOSING) { - val frame = ConnectionCloseFrame(conn.closeErrorCode, null, conn.closeReason ?: "") - val packet = buildBestLevelPacket(conn, listOf(frame)) ?: return null + val datagram = buildClosingDatagram(conn, nowMillis) conn.status = QuicConnection.Status.CLOSED - return packet + return datagram } // Drain destructive frame sources into local lists, ONCE. @@ -166,6 +173,82 @@ private fun concat(parts: List): ByteArray { return out } +/** + * Build a CONNECTION_CLOSE-only datagram at the highest encryption level we + * have keys for. Two RFC 9000 constraints make this trickier than a normal + * packet build: + * + * - §10.2.3 — at Initial / Handshake levels, only CONNECTION_CLOSE + * (Transport, 0x1c) is allowed. An application-level close is replaced + * with `APPLICATION_ERROR (0x0c)` + frameType=0 + empty reason so we don't + * leak app state pre-handshake. + * - §14.1 — a client datagram containing an Initial MUST be ≥ 1200 bytes, + * even a close-only one. We do this by building once at natural size and, + * if short, rewinding the PN and rebuilding with a PADDING-frame deficit + * inside the AEAD envelope. + */ +private fun buildClosingDatagram( + conn: QuicConnection, + nowMillis: Long, +): ByteArray? { + val app = conn.application + if (app.sendProtection != null) { + // 1-RTT level reached: app close (0x1d) is allowed and carries the + // original error code + reason. + val frame = ConnectionCloseFrame(conn.closeErrorCode, null, conn.closeReason ?: "") + return buildBestLevelPacket(conn, listOf(frame)) + } + + // Pre-1-RTT: must use transport-encoded close. RFC 9000 §20.1 + // APPLICATION_ERROR = 0x0c — "the application or application protocol + // caused the connection to be closed during the handshake". + val transportFrame = + ConnectionCloseFrame( + errorCode = APPLICATION_ERROR, + frameType = 0L, + reason = "", + ) + + val hs = conn.handshake + if (hs.sendProtection != null) { + return buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.HANDSHAKE, + frames = listOf(transportFrame), + tokens = emptyList(), + nowMillis = nowMillis, + padBytes = 0, + ) + } + + val init = conn.initial + if (init.sendProtection != null && !init.keysDiscarded) { + val natural = + buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.INITIAL, + frames = listOf(transportFrame), + tokens = emptyList(), + nowMillis = nowMillis, + padBytes = 0, + ) + if (natural.size >= 1200) return natural + init.pnSpace.rewindOutboundForRebuild() + return buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.INITIAL, + frames = listOf(transportFrame), + tokens = emptyList(), + nowMillis = nowMillis, + padBytes = 1200 - natural.size, + ) + } + return null +} + +/** RFC 9000 §20.1 — application-or-protocol caused close during handshake. */ +private const val APPLICATION_ERROR: Long = 0x0cL + private fun buildBestLevelPacket( conn: QuicConnection, frames: List, diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt new file mode 100644 index 000000000..96c789aef --- /dev/null +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt @@ -0,0 +1,91 @@ +/* + * 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.QuicWriter +import com.vitorpamplona.quic.frame.ConnectionCloseFrame +import com.vitorpamplona.quic.tls.PermissiveCertificateValidator +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Regression tests for two RFC 9000 violations found via the + * `quic-interop-runner` against aioquic on 2026-05-06: + * + * - §10.2.3 — `CONNECTION_CLOSE (Application)` (0x1d) MUST NOT appear in + * Initial / Handshake packets. Application-error closes that fire before + * 1-RTT keys exist must be encoded as `CONNECTION_CLOSE (Transport)` + * (0x1c) with `errorCode = APPLICATION_ERROR (0x0c)`. + * - §14.1 — any client datagram containing an Initial MUST be ≥ 1200 + * bytes, even when carrying only a `CONNECTION_CLOSE`. + * + * Pre-fix, the writer's CLOSING branch built a tiny ~45-byte Initial with + * frame type 0x1d. The runner's aioquic server silently dropped it (correct + * per spec) and our handshake hung until our 10s timeout. + */ +class CloseDatagramRfcComplianceTest { + @Test + fun `pre-handshake close datagram is padded to at least 1200 bytes per RFC 9000 sec 14_1`() = + runBlocking { + val conn = + QuicConnection( + serverName = "example.test", + config = QuicConnectionConfig(), + tlsCertificateValidator = PermissiveCertificateValidator(), + ) + // Initial sendProtection is wired in QuicConnection's init block; + // no handshake required to exercise the close path. + conn.status = QuicConnection.Status.CLOSING + + val datagram = drainOutbound(conn, nowMillis = 0L) + requireNotNull(datagram) { "drainOutbound must produce a close datagram when CLOSING with Initial keys" } + + assertTrue( + datagram.size >= 1200, + "client Initial datagram MUST be ≥ 1200 bytes per RFC 9000 §14.1, " + + "got ${datagram.size} (this was bug A from the aioquic interop run)", + ) + // First byte: 1100????b — long-header form + Initial type. + val firstByte = datagram[0].toInt() and 0xff + assertEquals(0xc0, firstByte and 0xf0, "must be a long-header packet") + assertEquals(0x00, firstByte and 0x30, "must be type=Initial (00)") + } + + @Test + fun `ConnectionCloseFrame encodes type 0x1c when frameType is non-null (transport close)`() { + // Bug B fix relies on the writer passing frameType=0L (instead of + // null) when emitting a close at Initial / Handshake level. Encode + // path must produce 0x1c for that case, 0x1d only for app-level. + val transportClose = ConnectionCloseFrame(errorCode = 0x0c, frameType = 0L, reason = "") + val w1 = QuicWriter() + transportClose.encode(w1) + val transportBytes = w1.toByteArray() + assertEquals(0x1c.toByte(), transportBytes[0], "transport CONNECTION_CLOSE must serialize as 0x1c") + + val appClose = ConnectionCloseFrame(errorCode = 0, frameType = null, reason = "") + val w2 = QuicWriter() + appClose.encode(w2) + val appBytes = w2.toByteArray() + assertEquals(0x1d.toByte(), appBytes[0], "application CONNECTION_CLOSE must serialize as 0x1d") + } +}