fix(quic): pad PING-only Initial datagrams to strict 1200 bytes (RFC 9000 §14.1)
The padding-rebuild branch in QuicConnectionWriter.drainOutbound computed
`padBytes = 1200 - natural`, but the QUIC long-header Length field is a
varint (RFC 9000 §16). When the natural-size payload was small enough for
Length to fit in 1 byte (body ≤ 63 bytes), the rebuild's larger body
crossed the 64-byte threshold and Length grew to 2 bytes — adding 1 wire
byte that wasn't in `natural`. PING-only PTO probe Initials therefore went
out at exactly 1199 bytes, one short of the §14.1 floor.
Fix: rebuild iteratively. After the first rebuild, measure the actual
datagram size; if still < 1200, bump padBytes by the residual and rebuild
once more. PADDING bytes inside the AEAD envelope add 1:1 to the wire
size and the Length varint grows monotonically, so the loop terminates
in ≤ 2 iterations for any reachable payload.
Same fix is applied to buildClosingDatagram so close-only Initial probes
on the boundary aren't tripped by future varint-growth changes.
Tightens the existing PTO-probe regression test to assert ≥ 1200 (was
relaxed to ≥ 1199 in 86b6c609a) and adds a new boundary test that builds
a single-byte-payload Initial and checks 1200 ≤ size ≤ 1203 — strict
floor with a tight ceiling so over-correction would also fail.
https://claude.ai/code/session_01HcvfQq1ttPV9PkRoJb4nyT
This commit is contained in:
Claude
+161
-19
@@ -62,12 +62,19 @@ fun drainOutbound(
|
||||
): ByteArray? {
|
||||
val parts = mutableListOf<ByteArray>()
|
||||
|
||||
// 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.
|
||||
@@ -118,21 +125,42 @@ fun drainOutbound(
|
||||
var natural = 0
|
||||
for (p in firstPass) natural += p.size
|
||||
if (natural < 1200) {
|
||||
val deficit = 1200 - natural
|
||||
// Rewind the Initial PN — we'll reissue with the same PN and the
|
||||
// same captured frames plus padding. The SentPacket entry recorded
|
||||
// by the natural-size build will be overwritten by the rebuild
|
||||
// below since both use the same PN.
|
||||
initialState.pnSpace.rewindOutboundForRebuild()
|
||||
val paddedInitial =
|
||||
buildLongHeaderFromFrames(
|
||||
conn = conn,
|
||||
level = EncryptionLevel.INITIAL,
|
||||
frames = initialContents!!.frames, // null-safe: gated by `initialNatural != null` above
|
||||
tokens = initialContents.tokens,
|
||||
nowMillis = nowMillis,
|
||||
padBytes = deficit,
|
||||
)
|
||||
// Padding rebuild: re-issue the Initial with PADDING frames inside
|
||||
// the AEAD envelope so the on-wire datagram clears the §14.1 floor.
|
||||
//
|
||||
// Off-by-one trap: the QUIC long-header Length field is a varint
|
||||
// (RFC 9000 §16). When the natural-size payload is tiny enough
|
||||
// for Length to fit in 1 byte (body ≤ 63 bytes), the rebuild's
|
||||
// larger body crosses the 64-byte threshold and Length grows to
|
||||
// 2 bytes — adding 1 wire byte that wasn't in `natural`. Same
|
||||
// shape at 16384 bytes (2 → 4) and 2^30 (4 → 8). A naive
|
||||
// `padBytes = 1200 - natural` then produces a 1199-byte
|
||||
// datagram for PING-only Initials.
|
||||
//
|
||||
// Solution: rebuild with the initial deficit, measure, and if we
|
||||
// still fall short, bump by the residual and rebuild once more.
|
||||
// Each iteration adds PADDING bytes 1:1 to the wire size; the
|
||||
// varint grows monotonically so this terminates in ≤ 2 rounds
|
||||
// for any reachable payload size.
|
||||
var padBytes = 1200 - natural
|
||||
var paddedInitial: ByteArray
|
||||
while (true) {
|
||||
initialState.pnSpace.rewindOutboundForRebuild()
|
||||
paddedInitial =
|
||||
buildLongHeaderFromFrames(
|
||||
conn = conn,
|
||||
level = EncryptionLevel.INITIAL,
|
||||
frames = initialContents!!.frames, // null-safe: gated by `initialNatural != null` above
|
||||
tokens = initialContents.tokens,
|
||||
nowMillis = nowMillis,
|
||||
padBytes = padBytes,
|
||||
)
|
||||
var totalAfterRebuild = paddedInitial.size
|
||||
if (handshakeNatural != null) totalAfterRebuild += handshakeNatural.size
|
||||
if (applicationPkt != null) totalAfterRebuild += applicationPkt.size
|
||||
if (totalAfterRebuild >= 1200) break
|
||||
padBytes += 1200 - totalAfterRebuild
|
||||
}
|
||||
concat(listOfNotNull(paddedInitial, handshakeNatural, applicationPkt))
|
||||
} else {
|
||||
concat(firstPass)
|
||||
@@ -166,6 +194,93 @@ private fun concat(parts: List<ByteArray>): 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. The rebuild loops because the long-header
|
||||
* Length varint (RFC 9000 §16) can grow by 1 byte once the body crosses
|
||||
* the 64-byte threshold, so a single-shot deficit can land 1 byte short
|
||||
* of 1200.
|
||||
*/
|
||||
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
|
||||
var padBytes = 1200 - natural.size
|
||||
var padded: ByteArray
|
||||
do {
|
||||
init.pnSpace.rewindOutboundForRebuild()
|
||||
padded =
|
||||
buildLongHeaderFromFrames(
|
||||
conn = conn,
|
||||
level = EncryptionLevel.INITIAL,
|
||||
frames = listOf(transportFrame),
|
||||
tokens = emptyList(),
|
||||
nowMillis = nowMillis,
|
||||
padBytes = padBytes,
|
||||
)
|
||||
if (padded.size >= 1200) break
|
||||
padBytes += 1200 - padded.size
|
||||
} while (true)
|
||||
return padded
|
||||
}
|
||||
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<Frame>,
|
||||
@@ -280,10 +395,37 @@ private fun collectHandshakeLevelFrames(
|
||||
length = cryptoChunk.data.size.toLong(),
|
||||
)
|
||||
}
|
||||
// RFC 9002 §6.2.4: PTO probe MUST be emitted at the encryption level
|
||||
// that has unacknowledged data. `buildApplicationPacket` consumes
|
||||
// [pendingPing] preferentially when 1-RTT keys exist; if they don't,
|
||||
// the probe lives or dies here at Handshake / Initial level. Pre-fix
|
||||
// the flag was set but never honored pre-handshake — the connection
|
||||
// sat silent through every PTO, never retransmitting the ClientHello
|
||||
// even when the first datagram was lost. This caused the
|
||||
// "1-packet-on-the-wire-then-timeout" symptom against aioquic in the
|
||||
// quic-interop-runner sim where the first ClientHello was dropped
|
||||
// because the server hadn't finished startup yet.
|
||||
if (conn.pendingPing && conn.application.sendProtection == null && level == highestPreApplicationLevel(conn)) {
|
||||
frames += PingFrame
|
||||
conn.pendingPing = false
|
||||
}
|
||||
if (frames.isEmpty()) return null
|
||||
return HandshakeLevelContents(frames = frames, tokens = tokens)
|
||||
}
|
||||
|
||||
/**
|
||||
* The highest encryption level for which we currently hold send keys, given
|
||||
* that 1-RTT keys are NOT yet installed. Used by [collectHandshakeLevelFrames]
|
||||
* to decide where a PTO PING probe should ride when the application level
|
||||
* isn't usable yet.
|
||||
*/
|
||||
private fun highestPreApplicationLevel(conn: QuicConnection): EncryptionLevel =
|
||||
when {
|
||||
conn.handshake.sendProtection != null -> EncryptionLevel.HANDSHAKE
|
||||
conn.initial.sendProtection != null && !conn.initial.keysDiscarded -> EncryptionLevel.INITIAL
|
||||
else -> EncryptionLevel.INITIAL // fallback; collectHandshakeLevelFrames already early-returns on no keys
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a long-header packet from already-collected frames, with optional
|
||||
* trailing PADDING (0x00) bytes inside the encryption envelope. RFC 9000
|
||||
|
||||
+174
@@ -0,0 +1,174 @@
|
||||
/*
|
||||
* 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.assertNotNull
|
||||
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 `PTO probe emits a PING at Initial level pre-handshake (RFC 9002 sec 6_2_4)`() =
|
||||
runBlocking {
|
||||
// Reproduces the third bug found via the aioquic interop run on
|
||||
// 2026-05-06: when the first ClientHello is dropped (e.g. sim
|
||||
// queues it before the server is ready), the driver's PTO timer
|
||||
// sets `pendingPing = true`, but the writer only honored that
|
||||
// flag in the 1-RTT path. Pre-handshake the flag was silently
|
||||
// discarded, so the second drain produced no Initial datagram —
|
||||
// the connection sat mute until our 10s handshake timeout.
|
||||
//
|
||||
// Pre-fix: drainOutbound returned null and the connection slept
|
||||
// on the next PTO. Post-fix: a PING-bearing Initial datagram is
|
||||
// emitted, eliciting an ACK from the peer that feeds loss
|
||||
// detection and unblocks CRYPTO retransmit.
|
||||
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 PTO probe path.
|
||||
// Skip conn.start() so the cryptoSend buffer is empty — this is
|
||||
// exactly the post-ClientHello-sent state when PTO fires.
|
||||
conn.pendingPing = true
|
||||
|
||||
val datagram = drainOutbound(conn, nowMillis = 0L)
|
||||
assertNotNull(
|
||||
datagram,
|
||||
"PTO probe MUST produce an Initial datagram pre-handshake — RFC 9002 §6.2.4",
|
||||
)
|
||||
// RFC 9000 §14.1: client datagrams containing an Initial MUST
|
||||
// be ≥ 1200 bytes. The writer's padding rebuild now accounts
|
||||
// for Length-varint growth (1 → 2 bytes) when the natural-size
|
||||
// payload is small (PING-only) so the deficit calculation
|
||||
// produces a final size that strictly meets the spec floor.
|
||||
assertTrue(
|
||||
datagram.size >= 1200,
|
||||
"PTO Initial datagram MUST be ≥ 1200 bytes per RFC 9000 §14.1, got ${datagram.size}",
|
||||
)
|
||||
assertEquals(false, conn.pendingPing, "pendingPing MUST be cleared after the probe")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `single-byte PING-only Initial pads to exactly 1200 bytes (no overshoot beyond varint growth)`() =
|
||||
runBlocking {
|
||||
// Boundary case for the padding-rebuild deficit calculation.
|
||||
// The smallest possible Initial-level frame payload is a single
|
||||
// PING frame (1 byte, encoded as 0x01 — RFC 9000 §19.2). With
|
||||
// no token, the 1-byte natural payload encodes a Length varint
|
||||
// of 1 byte. Once the rebuild adds ~1170 bytes of PADDING the
|
||||
// Length value crosses the 63-byte varint boundary and grows
|
||||
// to 2 bytes. The deficit calculation MUST account for that
|
||||
// growth, otherwise the rebuilt packet falls 1 byte short of
|
||||
// the §14.1 1200-byte floor.
|
||||
//
|
||||
// Asserts the strict floor (≥ 1200) AND that we don't overshoot
|
||||
// by more than the varint-growth delta plus a small slack — the
|
||||
// padded packet should land in the 1200..1203 range, never
|
||||
// 1199 (pre-fix) and never 1300+ (over-correction).
|
||||
val conn =
|
||||
QuicConnection(
|
||||
serverName = "example.test",
|
||||
config = QuicConnectionConfig(),
|
||||
tlsCertificateValidator = PermissiveCertificateValidator(),
|
||||
)
|
||||
conn.pendingPing = true
|
||||
|
||||
val datagram = drainOutbound(conn, nowMillis = 0L)
|
||||
assertNotNull(datagram, "PING-only PTO probe must produce a datagram")
|
||||
assertTrue(
|
||||
datagram.size >= 1200,
|
||||
"padded Initial MUST be ≥ 1200 bytes (RFC 9000 §14.1), got ${datagram.size}",
|
||||
)
|
||||
assertTrue(
|
||||
datagram.size <= 1203,
|
||||
"padded Initial should not overshoot the 1200 floor by more than the " +
|
||||
"Length-varint growth (≤ 3 bytes), got ${datagram.size}",
|
||||
)
|
||||
}
|
||||
|
||||
@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")
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user