From e4d96421c2e79f2b7681380d8801def5f8c7012d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 23:40:20 +0000 Subject: [PATCH] fix(quic): reserved-bit checks, TLS bounds, SendBuffer shrink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of the audit follow-ups. * Reserved-bit enforcement on unmasked QUIC headers (RFC 9000 §17.2 / §17.3.1). Pre-fix the parser silently accepted long-header packets with bits 0x0C set or short-header packets with bits 0x18 set after HP unmasking — the spec mandates PROTOCOL_VIOLATION close. Added [QuicProtocolViolationException] and a top-level catch in feedDatagram that translates the throw into markClosedExternally. Long-header parse also drops a now-dead `if form==1` branch on the first-byte mask: we already early-returned in non-long paths above, so the mask is always 0x0F. * TLS handshake-message bounds: - TlsCertificateChain.decodeBody: reject `listLen > r.remaining` up front; assert `r.position == end` after the cert loop. Without this, a malicious peer could push us into reading past the message limit on per-cert extensions. - TlsServerHello.decodeBody / TlsEncryptedExtensions.decodeBody: reject trailing bytes after the extensions block. - TlsEncryptedExtensions.alpn: enforce RFC 7301 §3.1 (server returns EXACTLY one protocol_name); validate outerLen matches remaining and reject multi-name responses. * SendBuffer.data shrink: pre-fix the doubling-on-grow buffer never shrank, so a stream that ever held N bytes pinned `data.size = N` for the connection's lifetime. Long-tail memory retention on per-stream basis. advanceFlushedFloorIfPossible now releases capacity once live bytes occupy ≤ 1/4 of the allocation, shrinking to max(SHRINK_FLOOR_BYTES=4096, 2*dataLen). Below the floor the doubling cost is negligible; above it the multi-MiB transients release back to the heap. All 269 :quic:jvmTest tests pass. BUILD SUCCESSFUL in 47s. https://claude.ai/code/session_01AhGvbMV8uPRse3TmAGaddM --- .../kotlin/com/vitorpamplona/quic/Buffer.kt | 14 +++++ .../quic/connection/QuicConnectionParser.kt | 16 +++++ .../quic/packet/LongHeaderPacket.kt | 25 ++++++-- .../quic/packet/ShortHeaderPacket.kt | 15 ++++- .../vitorpamplona/quic/stream/SendBuffer.kt | 28 +++++++++ .../quic/tls/TlsHandshakeMessages.kt | 58 +++++++++++++++++-- 6 files changed, 147 insertions(+), 9 deletions(-) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/Buffer.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/Buffer.kt index 97e902969..1d94428fd 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/Buffer.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/Buffer.kt @@ -291,3 +291,17 @@ class QuicCodecException( message: String, cause: Throwable? = null, ) : RuntimeException(message, cause) + +/** + * Peer protocol violation that mandates connection close per RFC 9000 / 9001. + * Distinct from [QuicCodecException] (which is also "drop the packet" for + * AEAD-failed inputs) — a [QuicProtocolViolationException] means the peer + * sent something well-formed enough to AEAD-decrypt but inconsistent with + * the wire spec, so the connection MUST be closed with PROTOCOL_VIOLATION. + * + * Typical sources: reserved-bit-set in the unmasked QUIC header + * (RFC 9000 §17.2 / §17.3.1). + */ +class QuicProtocolViolationException( + message: String, +) : RuntimeException(message) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt index a983ddbe1..92648006b 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -71,6 +71,22 @@ fun feedDatagram( conn: QuicConnection, datagram: ByteArray, nowMillis: Long, +) { + try { + feedDatagramInner(conn, datagram, nowMillis) + } catch (e: com.vitorpamplona.quic.QuicProtocolViolationException) { + // RFC 9000 §17.2 / §17.3.1: peer set reserved bits in the + // header after HP unmask (or a similar invariant violation + // bubbled out of the parse path). Spec MUST close with + // PROTOCOL_VIOLATION. + conn.markClosedExternally(e.message ?: "PROTOCOL_VIOLATION") + } +} + +private fun feedDatagramInner( + conn: QuicConnection, + datagram: ByteArray, + nowMillis: Long, ) { var offset = 0 while (offset < datagram.size) { diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt index 4554cd1f3..5f5416baf 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt @@ -183,10 +183,27 @@ object LongHeaderPacket { val packet = bytes.copyOfRange(packetStart, packetEnd) val localPnOffset = pnOffset - packetStart - // Step 1: unmask the first byte so we can read pnLen. - val firstByteMask = if ((first and 0x80) != 0) 0x0F else 0x1F - packet[0] = (first xor (mask[0].toInt() and firstByteMask)).toByte() - val pnLen = ((packet[0].toInt() and 0xFF) and 0x03) + 1 + // Step 1: unmask the first byte so we can read pnLen. The + // long-header form bit (0x80) was already validated above (we + // wouldn't be in this function otherwise), so the first-byte + // mask is fixed at 0x0F — pre-fix the conditional `if (form == 1)` + // path was dead code. + packet[0] = (first xor (mask[0].toInt() and 0x0F)).toByte() + val unmaskedFirst = packet[0].toInt() and 0xFF + // RFC 9000 §17.2: long header layout is `1|1|T|T|R|R|P|P` — + // the two reserved bits at 0x0C MUST be zero after HP unmasking. + // A peer that sets either bit is in protocol violation. The + // form-bit (0x80) and fixed-bit (0x40) are not header-protected, + // so they're already correct; we skip the AEAD here on + // reserved-bit set rather than letting a malformed-but-AEAD-OK + // packet drift our state. + if ((unmaskedFirst and 0x0C) != 0) { + throw com.vitorpamplona.quic.QuicProtocolViolationException( + "PROTOCOL_VIOLATION: long-header reserved bits set " + + "(0x${unmaskedFirst.toString(16)})", + ) + } + val pnLen = (unmaskedFirst and 0x03) + 1 // Step 2: unmask exactly `pnLen` packet-number bytes. for (i in 0 until pnLen) { diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt index c14d3a845..c5d4ca073 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt @@ -158,7 +158,20 @@ object ShortHeaderPacket { val localPnOffset = pnOffset - offset val firstByteMask = 0x1F packet[0] = (first xor (mask[0].toInt() and firstByteMask)).toByte() - val pnLen = ((packet[0].toInt() and 0xFF) and 0x03) + 1 + val unmaskedFirst = packet[0].toInt() and 0xFF + // RFC 9000 §17.3.1: short header layout is `0|1|S|R|R|K|P|P` — + // the two reserved bits at 0x18 MUST be zero after HP unmasking. + // A peer that sets either bit is in protocol violation. Pre-fix + // we silently accepted the packet and let the AEAD pass; an + // off-spec server (or a fuzzer) could then push us to interop + // bugs that don't reproduce against well-behaved peers. + if ((unmaskedFirst and 0x18) != 0) { + throw com.vitorpamplona.quic.QuicProtocolViolationException( + "PROTOCOL_VIOLATION: short-header reserved bits set " + + "(0x${unmaskedFirst.toString(16)})", + ) + } + val pnLen = (unmaskedFirst and 0x03) + 1 for (i in 0 until pnLen) { packet[localPnOffset + i] = (packet[localPnOffset + i].toInt() xor mask[1 + i].toInt()).toByte() } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt index ed63d19bd..f4100c509 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt @@ -591,6 +591,23 @@ class SendBuffer( } dataLen -= advanceInt flushedFloor += advance + // Shrink the backing buffer when a transient burst has been fully + // drained. Without this, a stream that ever held N bytes pins + // `data.size = N` for the rest of the connection — long-tail + // memory retention. Trigger when: + // * the buffer is large enough to bother (above the doubling + // floor of 64 bytes) AND + // * live data fits in 1/4 of the allocation (capacity is + // ≥ 4× live size). + // Shrink to twice the live size (or [SHRINK_FLOOR_BYTES], + // whichever is larger) to leave headroom for the next push + // without re-doubling immediately. + if (data.size > SHRINK_FLOOR_BYTES && dataLen * 4 < data.size) { + val newCap = maxOf(SHRINK_FLOOR_BYTES, dataLen * 2) + if (newCap < data.size) { + data = data.copyOf(newCap) + } + } } /** @@ -614,6 +631,17 @@ class SendBuffer( } } + private companion object { + /** + * Backing-buffer floor below which [advanceFlushedFloorIfPossible] + * does NOT shrink. Below this size the doubling-on-grow is so + * cheap that shrinking would be a wash; above it, transient + * bursts that bloat the buffer to MiBs leave a long-tail + * memory footprint we want to release. + */ + const val SHRINK_FLOOR_BYTES: Int = 4096 + } + /** * One contiguous offset range tracked by the buffer's bookkeeping. * [length] is `Long` to match QUIC's offset arithmetic — practical diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsHandshakeMessages.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsHandshakeMessages.kt index 1a2e45ada..618d31788 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsHandshakeMessages.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/tls/TlsHandshakeMessages.kt @@ -77,6 +77,13 @@ data class TlsServerHello( val cipherSuite = r.readUint16() r.readByte() // legacy_compression_method = 0 val extensions = TlsExtension.decodeList(r) + // RFC 8446 §4.1.3: ServerHello body ends with the extensions + // block. Any trailing bytes are a malformed handshake message. + if (r.remaining != 0) { + throw QuicCodecException( + "ServerHello has ${r.remaining} trailing bytes after extensions", + ) + } return TlsServerHello(random, sessionId, cipherSuite, extensions) } } @@ -92,14 +99,38 @@ data class TlsEncryptedExtensions( val alpn: ByteArray? get() = extensions.firstOrNull { it.type == TlsConstants.EXT_ALPN }?.data?.let { - // ALPN response carries a single protocol_name<1..2^8-1> inside protocols<3..2^16-1> + // ALPN response carries EXACTLY one protocol_name<1..2^8-1> + // inside protocols<3..2^16-1> per RFC 7301 §3.1. Reject + // multi-name responses — a server that returns more than + // one is in protocol violation, and silently picking the + // first masks an interop bug. val r = QuicReader(it) - r.skip(2) // outer length - r.readTlsOpaque1() + val outerLen = r.readUint16() + if (outerLen != r.remaining) { + throw QuicCodecException( + "ALPN extension outerLen=$outerLen does not match remaining ${r.remaining}", + ) + } + val name = r.readTlsOpaque1() + if (r.remaining != 0) { + throw QuicCodecException( + "ALPN response carries multiple protocol names (RFC 7301 §3.1 forbids)", + ) + } + name } companion object { - fun decodeBody(r: QuicReader): TlsEncryptedExtensions = TlsEncryptedExtensions(TlsExtension.decodeList(r)) + fun decodeBody(r: QuicReader): TlsEncryptedExtensions { + val extensions = TlsExtension.decodeList(r) + // RFC 8446 §4.3.1: EE body is exactly the extensions block. + if (r.remaining != 0) { + throw QuicCodecException( + "EncryptedExtensions has ${r.remaining} trailing bytes", + ) + } + return TlsEncryptedExtensions(extensions) + } } } @@ -115,6 +146,17 @@ data class TlsCertificateChain( fun decodeBody(r: QuicReader): TlsCertificateChain { val ctx = r.readTlsOpaque1() val listLen = r.readUint24() + // Pre-fix: a malformed peer setting `listLen` larger than the + // remaining body bytes caused `end = r.position + listLen` to + // point past `r.limit`. The while-loop then ran on garbage + // until each `readTlsOpaque*` ran off the end and threw — + // delivering corrupt cert blobs to certs[] before that + // happened. Bound `end` against `r.limit` up front. + if (listLen > r.remaining) { + throw QuicCodecException( + "Certificate listLen=$listLen exceeds remaining body ${r.remaining}", + ) + } val end = r.position + listLen val certs = mutableListOf() while (r.position < end) { @@ -123,6 +165,14 @@ data class TlsCertificateChain( r.readTlsOpaque2() certs += cert } + // RFC 8446 §4.4.2: the certificate_list ends at exactly `end`; + // if a per-cert extensions length pushed us past it, the cert + // chain is malformed and we MUST close. + if (r.position != end) { + throw QuicCodecException( + "Certificate listLen mismatch: position=${r.position} expected=$end", + ) + } return TlsCertificateChain(ctx, certs) } }