nick/amethyst
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(quic): round-4 tier-1 + tier-2 audit fixes

Browse Source 
Critical interop blockers + security/correctness gaps surfaced by the
parallel round-4 audit. All fixes have inline comments referencing the
audit finding number.

Frame layer:
  * Decode RESET_STREAM (0x04), STOP_SENDING (0x05), NEW_TOKEN (0x07).
    Pre-fix these fell through to the `unknown frame type` branch and
    threw QuicCodecException through the read loop, killing the
    connection. aioquic and picoquic emit RESET_STREAM regularly.
  * Wrap decodeFrames in try/catch in dispatchFrames; on a decode
    error, transition to CLOSED gracefully via markClosedExternally
    instead of letting the exception escape the read loop.

Connection layer:
  * Reject peer-attempted CLIENT_BIDI / CLIENT_UNI stream IDs that don't
    map to a stream we opened (RFC 9000 §19.8 STREAM_STATE_ERROR).
  * MaxDataFrame now actually updates sendConnectionFlowCredit (was a
    no-op pre-fix; sustained sends silently stalled).
  * Writer enforces sendConnectionFlowCredit and tracks
    sendConnectionFlowConsumed so cumulative bytes stay under the
    peer's initial_max_data cap.
  * SERVER_BIDI peer-opened streams inherit sendCredit from
    peer.initialMaxStreamDataBidiLocal (was 0L; reply path was wedged
    until MAX_STREAM_DATA arrived).
  * applyPeerTransportParameters validates initial_source_connection_id
    and original_destination_connection_id (RFC 9000 §7.3 MUST checks);
    mismatch closes with TRANSPORT_PARAMETER_ERROR.
  * Cap incomingDatagrams queue at 256 (audio rooms ~50/sec; 5-second
    burst). On overflow, drop oldest — fresh frames matter more for
    live media. Pre-fix RFC 9221 datagrams were unbounded.

Stream layer:
  * QuicStream.deliverIncoming now returns Boolean; parser closes the
    connection with INTERNAL_ERROR on saturation rather than silently
    dropping bytes (peer believes the bytes were delivered, application
    sees a hole).
  * ReceiveBuffer tracks finOffset and exposes isFullyRead(); parser
    only closes the incoming channel after the contiguous read frontier
    reaches the FIN offset (pre-fix closing on FIN-frame arrival
    truncated streams that had gaps).

TLS hardening:
  * certificateValidator is non-null. Tests pass an explicit
    PermissiveCertificateValidator; null was a silent-MITM hazard.
  * Drop SIG_RSA_PKCS1_SHA256 from accepted CertificateVerify
    schemes (forbidden by RFC 8446 §4.2.3 in CertificateVerify).
  * Hard-fail the PSK-Finished path: we never offer a pre_shared_key
    extension, so a server skipping Certificate/CertificateVerify is
    either misbehaving or a partial-MITM stripping cert proof.
  * Validate ALPN: reject any ALPN the server selected that we didn't
    offer (was previously accepted silently).
  * Add APPLICATION-level inboundBuffer so post-handshake CRYPTO
    (NewSessionTicket, KeyUpdate detection) reaches the
    SENT_CLIENT_FINISHED handler.
  * State.FAILED is now actually assigned on any handler throw;
    pushHandshakeBytes refuses further bytes when in FAILED.
  * IP-literal precheck before InetAddress.getByName so cert
    validation doesn't trigger DNS A/AAAA lookups for hostnames
    (audit-4 #4: leaked SNI/hostname over plaintext DNS).

WT layer:
  * GOAWAY id-regression check (RFC 9114 §5.2: MUST NOT increase).
    A server sending an increasing id raises QuicCodecException.
  * WT_CLOSE_SESSION decoder rejects bodies < 4 bytes (mandatory
    error-code field) and reasons > 8192 bytes.
  * Capsule reader catches Throwable but separately rethrows
    CancellationException; on parse error, completes peerCloseDeferred
    exceptionally so awaitPeerClose() doesn't hang forever.

HTTP/3 + QPACK:
  * Http3Settings.decodeBody rejects duplicate ids (RFC 9114 §7.2.4.1
    H3_SETTINGS_ERROR).
  * QpackInteger.decode bounds-checks shift before extending value;
    defence-in-depth Long-overflow check on accumulated value.
  * QpackDecoder static-table accesses go through a bounds-checking
    helper that throws typed QuicCodecException; literal lengths are
    range-checked before allocation.

Test infra:
  * InMemoryQuicPipe accepts an injectable serverScid and constructs
    its tlsServer with TPs that include the required CIDs.
  * InProcessTlsServer emits stub Certificate + CertificateVerify
    so the real (non-PSK) handshake path is exercised.
  * Updated all test callers to use PermissiveCertificateValidator.
  * Updated CapsuleReaderTest with negative-path assertions for the
    new strictness.

https://claude.ai/code/session_01EC1tfXfap8k8GyKvrxkxZx
This commit is contained in:
Claude
2026-04-26 00:31:21 +00:00
parent 0023c73aeb
commit 222a4e7d42
23 changed files with 702 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files
quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CoalescedPacketSkipTest.kt
+9 -3
View File
@@ -88,7 +88,9 @@ class CoalescedPacketSkipTest {
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
val secrets = InitialSecrets.derive(client.destinationConnectionId.bytes)
val serverScid = ConnectionId.random(8)
@@ -119,7 +121,9 @@ class CoalescedPacketSkipTest {
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
val secrets = InitialSecrets.derive(client.destinationConnectionId.bytes)
val serverScid = ConnectionId.random(8)
@@ -155,7 +159,9 @@ class CoalescedPacketSkipTest {
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
val secrets = InitialSecrets.derive(client.destinationConnectionId.bytes)
val serverScid = ConnectionId.random(8)
quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipe.kt
+27 -5
View File
@@ -59,11 +59,34 @@ class InMemoryQuicPipe(
val client: QuicConnection,
val initialDcid: ByteArray,
/**
* Optional pre-configured TLS server. Tests that need to advertise non-empty
* QUIC transport parameters (e.g. to exercise MAX_STREAMS routing) build
* their own [InProcessTlsServer] and pass it here.
* Server-side source connection id. Exposed on the constructor so the
* [tlsServer] can advertise it as `initial_source_connection_id` in
* transport parameters (RFC 9000 §7.3 — REQUIRED). Defaults to a fresh
* 8-byte random id.
*/
private val tlsServer: InProcessTlsServer = InProcessTlsServer(),
val serverScid: ConnectionId = ConnectionId.random(8),
/**
* Optional pre-configured TLS server. The default builds one that
* advertises the bare-minimum transport parameters required by audit-4
* #7's CID-validation: `initial_source_connection_id = serverScid` plus
* generous data/stream caps so handshake tests work without each having
* to construct their own. Tests that want to exercise specific TP values
* build their own server and pass it.
*/
private val tlsServer: InProcessTlsServer =
InProcessTlsServer(
transportParameters =
TransportParameters(
initialMaxData = 1_000_000,
initialMaxStreamDataBidiLocal = 100_000,
initialMaxStreamDataBidiRemote = 100_000,
initialMaxStreamDataUni = 100_000,
initialMaxStreamsBidi = 16,
initialMaxStreamsUni = 16,
initialSourceConnectionId = serverScid.bytes,
originalDestinationConnectionId = initialDcid,
).encode(),
),
) {
private val initial = InitialSecrets.derive(initialDcid)
private val hp = AesEcbHeaderProtection(PlatformAesOneBlock)
@@ -74,7 +97,6 @@ class InMemoryQuicPipe(
private var serverApplicationRx: PacketProtection? = null
private var serverApplicationTx: PacketProtection? = null
private val serverScid = ConnectionId.random(8)
private val initialPnSpace = PacketNumberSpaceState()
private val handshakePnSpace = PacketNumberSpaceState()
private val applicationPnSpace = PacketNumberSpaceState()
quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/InMemoryQuicPipeTest.kt
+3 -1
View File
@@ -44,7 +44,9 @@ class InMemoryQuicPipeTest {
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
val pipe = InMemoryQuicPipe(client = client, initialDcid = client.destinationConnectionId.bytes)
quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PeerStreamLimitTest.kt
+36 -5
View File
@@ -49,11 +49,34 @@ class PeerStreamLimitTest {
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
// Explicitly advertise zero bidi streams. (The pipe's default TPs
// grant 16, so we override.)
val serverScid = ConnectionId.random(8)
val tlsServer =
InProcessTlsServer(
transportParameters =
TransportParameters(
initialMaxData = 1_000_000,
initialMaxStreamDataBidiLocal = 100_000,
initialMaxStreamDataBidiRemote = 100_000,
initialMaxStreamDataUni = 100_000,
initialMaxStreamsBidi = 0,
initialMaxStreamsUni = 0,
initialSourceConnectionId = serverScid.bytes,
originalDestinationConnectionId = client.destinationConnectionId.bytes,
).encode(),
)
val pipe =
InMemoryQuicPipe(
client = client,
initialDcid = client.destinationConnectionId.bytes,
serverScid = serverScid,
tlsServer = tlsServer,
)
// Default InProcessTlsServer sends empty transport parameters — so
// the client's peerMaxStreamsBidi resolves to 0 after handshake.
val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes)
client.start()
pipe.drive(maxRounds = 16)
assertEquals(QuicConnection.Status.CONNECTED, client.status)
@@ -71,8 +94,13 @@ class PeerStreamLimitTest {
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
// Build the TLS server with the audit-4 #7 required CIDs plus
// tight stream caps for the boundary test.
val serverScid = ConnectionId.random(8)
val serverTpBytes =
TransportParameters(
initialMaxData = 1_000_000,
@@ -81,12 +109,15 @@ class PeerStreamLimitTest {
initialMaxStreamDataUni = 100_000,
initialMaxStreamsBidi = 3,
initialMaxStreamsUni = 0,
initialSourceConnectionId = serverScid.bytes,
originalDestinationConnectionId = client.destinationConnectionId.bytes,
).encode()
val tlsServer = InProcessTlsServer(transportParameters = serverTpBytes)
val pipe =
InMemoryQuicPipe(
client = client,
initialDcid = client.destinationConnectionId.bytes,
serverScid = serverScid,
tlsServer = tlsServer,
)
client.start()
quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ReceiveLimitEnforcementTest.kt
+20 -4
View File
@@ -58,8 +58,14 @@ class ReceiveLimitEnforcementTest {
initialMaxStreamDataBidiRemote = 32,
initialMaxStreamDataBidiLocal = 32,
),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
// Audit-4 #7: TPs MUST include initial_source_connection_id matching
// the SCID the server uses on the wire — otherwise the post-handshake
// CID-validation step closes the connection.
val serverScid = ConnectionId.random(8)
val tlsServer =
InProcessTlsServer(
transportParameters =
@@ -70,9 +76,11 @@ class ReceiveLimitEnforcementTest {
initialMaxStreamDataUni = 100_000,
initialMaxStreamsBidi = 16,
initialMaxStreamsUni = 16,
initialSourceConnectionId = serverScid.bytes,
originalDestinationConnectionId = client.destinationConnectionId.bytes,
).encode(),
)
val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, tlsServer)
val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, serverScid, tlsServer)
client.start()
pipe.drive(maxRounds = 16)
assertEquals(QuicConnection.Status.CONNECTED, client.status)
@@ -113,8 +121,14 @@ class ReceiveLimitEnforcementTest {
initialMaxStreamDataBidiRemote = 64,
initialMaxStreamDataBidiLocal = 64,
),
tlsCertificateValidator = null,
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
// Audit-4 #7: TPs MUST include initial_source_connection_id matching
// the SCID the server uses on the wire — otherwise the post-handshake
// CID-validation step closes the connection.
val serverScid = ConnectionId.random(8)
val tlsServer =
InProcessTlsServer(
transportParameters =
@@ -125,9 +139,11 @@ class ReceiveLimitEnforcementTest {
initialMaxStreamDataUni = 100_000,
initialMaxStreamsBidi = 16,
initialMaxStreamsUni = 16,
initialSourceConnectionId = serverScid.bytes,
originalDestinationConnectionId = client.destinationConnectionId.bytes,
).encode(),
)
val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, tlsServer)
val pipe = InMemoryQuicPipe(client, client.destinationConnectionId.bytes, serverScid, tlsServer)
client.start()
pipe.drive(maxRounds = 16)
assertEquals(QuicConnection.Status.CONNECTED, client.status)
quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/HelloRetryRequestTest.kt
+1 -1
View File
@@ -42,7 +42,7 @@ class HelloRetryRequestTest {
serverName = "example.test",
transportParameters = ByteArray(0),
secretsListener = NoopSecretsListener,
certificateValidator = null,
certificateValidator = PermissiveCertificateValidator(),
)
tls.start()
// Drain (and discard) ClientHello.
quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/InProcessTlsServer.kt
+45
View File
@@ -136,6 +136,20 @@ class InProcessTlsServer(
transcript.append(ee)
outboundHandshake.addLast(ee)
// Audit-4 #3: TlsClient now hard-fails any handshake that skips
// Certificate + CertificateVerify (no PSK was offered, so a peer that
// omits them is either misbehaving or a partial-MITM stripping the
// cert proof). Emit syntactically-valid stubs that
// [PermissiveCertificateValidator] will accept; the test path goes
// through the same code as a real handshake.
val cert = buildCertificateStub()
transcript.append(cert)
outboundHandshake.addLast(cert)
val cv = buildCertificateVerifyStub()
transcript.append(cv)
outboundHandshake.addLast(cv)
// 7. Build server Finished
val sf = buildFinished(serverHandshakeSecret!!)
transcript.append(sf)
@@ -208,4 +222,35 @@ class InProcessTlsServer(
w.withUint24Length { writeBytes(tag) }
return w.toByteArray()
}
/**
* Encode a Certificate message with one stub leaf cert. The DER bytes are
* not a real cert — [PermissiveCertificateValidator] doesn't parse them.
* We just need the framing to round-trip through TlsCertificateChain.decodeBody.
*/
private fun buildCertificateStub(): ByteArray {
val w = QuicWriter()
w.writeByte(TlsConstants.HS_CERTIFICATE)
w.withUint24Length {
// certificate_request_context (opaque<0..255>) — empty for server cert.
writeTlsOpaque1(ByteArray(0))
// certificate_list — single CertificateEntry with one stub cert and zero exts.
withUint24Length {
writeTlsOpaque3(byteArrayOf(0x30, 0x00)) // minimal DER-ish placeholder
writeTlsOpaque2(ByteArray(0)) // per-cert extensions
}
}
return w.toByteArray()
}
/** Encode a CertificateVerify message with a fake RSA-PSS-SHA256 signature. */
private fun buildCertificateVerifyStub(): ByteArray {
val w = QuicWriter()
w.writeByte(TlsConstants.HS_CERTIFICATE_VERIFY)
w.withUint24Length {
writeUint16(TlsConstants.SIG_RSA_PSS_RSAE_SHA256)
writeTlsOpaque2(ByteArray(64)) // any bytes — Permissive accepts
}
return w.toByteArray()
}
}
quic/src/commonTest/kotlin/com/vitorpamplona/quic/tls/TlsRoundTripTest.kt
+15 -12
View File
@@ -49,7 +49,7 @@ class TlsRoundTripTest {
serverName = "example.test",
transportParameters = ByteArray(0),
secretsListener = capturedSecrets,
certificateValidator = null, // in-process loopback; no cert chain to validate
certificateValidator = PermissiveCertificateValidator(),
)
client.start()
@@ -63,14 +63,14 @@ class TlsRoundTripTest {
assertNotNull(sh, "server should produce ServerHello at Initial level")
client.pushHandshakeBytes(TlsClient.Level.INITIAL, sh)
// 3) Drain EncryptedExtensions + Finished (Handshake level) → client
val ee = server.pollOutboundHandshake()
assertNotNull(ee, "server should produce EncryptedExtensions")
client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, ee)
val sf = server.pollOutboundHandshake()
assertNotNull(sf, "server should produce server Finished")
client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, sf)
// 3) Drain EncryptedExtensions + Certificate + CertificateVerify +
// Finished (Handshake level) → client. The InProcessTlsServer now
// emits all four (audit-4 #3 — TlsClient hard-fails any non-PSK
// handshake that omits Certificate/CertificateVerify).
while (true) {
val msg = server.pollOutboundHandshake() ?: break
client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, msg)
}
// 4) Drain client Finished → server
val cf = client.pollOutbound(TlsClient.Level.HANDSHAKE)
@@ -109,15 +109,18 @@ class TlsRoundTripTest {
serverName = "example.test",
transportParameters = ByteArray(0),
secretsListener = capturedSecrets,
certificateValidator = null,
certificateValidator = PermissiveCertificateValidator(),
)
client.start()
val ch = client.pollOutbound(TlsClient.Level.INITIAL)!!
server.receiveClientHello(ch)
client.pushHandshakeBytes(TlsClient.Level.INITIAL, server.pollOutboundInitial()!!)
client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, server.pollOutboundHandshake()!!)
client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, server.pollOutboundHandshake()!!)
// EE + Certificate + CertificateVerify + Finished (audit-4 #3).
while (true) {
val msg = server.pollOutboundHandshake() ?: break
client.pushHandshakeBytes(TlsClient.Level.HANDSHAKE, msg)
}
server.receiveClientFinished(client.pollOutbound(TlsClient.Level.HANDSHAKE)!!)
assertEquals(TlsConstants.CIPHER_TLS_CHACHA20_POLY1305_SHA256, server.negotiatedCipherSuite)
quic/src/commonTest/kotlin/com/vitorpamplona/quic/webtransport/CapsuleReaderTest.kt
+23 -8
View File
@@ -82,15 +82,30 @@ class CapsuleReaderTest {
}
@Test
fun decodes_empty_body_close_session_as_zero_error_empty_reason() {
// Spec lower bound: body length 0, no error code present.
val empty = encodeCapsule(WtCapsuleType.WT_CLOSE_SESSION, ByteArray(0))
fun rejects_close_session_with_truncated_body_below_4_bytes() {
// Audit-4 #13: body shorter than the mandatory 4-byte error_code
// field is malformed; the decoder MUST surface this rather than
// synthesising a `WtCloseSession(0, "")` that the application can't
// distinguish from a clean close.
val truncated = encodeCapsule(WtCapsuleType.WT_CLOSE_SESSION, ByteArray(0))
val reader = CapsuleReader()
reader.push(empty)
val parsed = reader.next()
assertIs<WtCloseSession>(parsed)
assertEquals(0, parsed.errorCode)
assertEquals("", parsed.reason)
reader.push(truncated)
kotlin.test.assertFailsWith<com.vitorpamplona.quic.QuicCodecException> {
reader.next()
}
}
@Test
fun rejects_close_session_with_oversized_reason() {
// Audit-4 #14: draft-ietf-webtrans-http3 §5 caps the reason at 8192
// bytes. We reject overlong reasons rather than passing them on.
val body = ByteArray(4 + 8193) // 4 bytes error code + 8193-byte reason
val capsule = encodeCapsule(WtCapsuleType.WT_CLOSE_SESSION, body)
val reader = CapsuleReader()
reader.push(capsule)
kotlin.test.assertFailsWith<com.vitorpamplona.quic.QuicCodecException> {
reader.next()
}
}
@Test