fix(quic): reserved-bit checks, TLS bounds, SendBuffer shrink
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
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<ByteArray>()
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user