fix(marmot): bound forward-ratchet steps and tighten PrivateMessage AAD cap

Two DoS surfaces in the inbound path: **#8 — unbounded forward-ratchet on PrivateMessage decrypt.** A malicious sender (or any peer who can put bytes in the wire frame) controls the `generation` field of an inbound PrivateMessage. The SecretTree was happy to fast-forward the sender's application or handshake ratchet by however many steps that field implied — every step costing one HKDF-Expand. A single packet with `generation = 2^31` would have pinned a CPU at SHA-256 for minutes. The skipped-key cache (`MAX_SKIPPED_KEYS = 1000`) bounds memory but NOT compute: it just stops *caching* past the cap, the ratchet keeps walking. Adds `MAX_RATCHET_STEPS_PER_CALL = 4096` and rejects any decrypt that asks for a larger jump from the sender's current head, applied at both `applicationKeyNonceForGeneration` and `handshakeKeyNonceForGeneration`. 4096 leaves room for legitimate catch-up (mobile waking from a long sleep) while bounding the worst-case per-packet cost to ~4096 SHA-256 invocations. **#11 — oversized PrivateMessage AAD allocation.** The underlying [TlsReader] already caps every opaque<V> read at 1 MiB, so the ciphertext field is bounded — but `authenticated_data` and `encrypted_sender_data` were also allowed up to 1 MiB even though both fields legitimately carry a few hundred bytes at most. A pre-verification frame would force ~3 MiB of allocation per inbound packet. Tightens both fields to 64 KiB at PrivateMessage decode (below TlsReader's global cap) so the per-frame floor is predictable. 2 new tests: `secretTree_rejectsRatchetJumpsBeyondCap` exercises the boundary (4096 OK, larger throws with a "jump too large" message) and `privateMessage_rejectsOversizedAuthenticatedData` hand-builds a frame with a 65 KiB AAD field and confirms rejection. Marmot test suite green; interop 16/16. Closes audit gaps #8 and #11. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ
2026-04-25 04:48:40 +00:00
parent 1f42afa61a
commit 8c38394385
3 changed files with 138 additions and 8 deletions
@@ -196,11 +196,17 @@ data class PublicMessage(
    override fun hashCode(): Int = groupId.contentHashCode()

    companion object {
+        /** Same DoS cap that PrivateMessage applies to its AAD field. */
+        const val MAX_AUTHENTICATED_DATA_BYTES = 1 shl 16 // 64 KiB
+
        fun decodeTls(reader: TlsReader): PublicMessage {
            val groupId = reader.readOpaqueVarInt()
            val epoch = reader.readUint64()
            val sender = decodeSender(reader)
            val authenticatedData = reader.readOpaqueVarInt()
+            require(authenticatedData.size <= MAX_AUTHENTICATED_DATA_BYTES) {
+                "PublicMessage authenticated_data too large: ${authenticatedData.size} > $MAX_AUTHENTICATED_DATA_BYTES"
+            }
            val contentType = ContentType.fromValue(reader.readUint8())

            // RFC 9420 §6 FramedContent.content body varies by content_type.
@@ -289,15 +295,47 @@ data class PrivateMessage(
    override fun hashCode(): Int = groupId.contentHashCode()

    companion object {
-        fun decodeTls(reader: TlsReader): PrivateMessage =
-            PrivateMessage(
-                groupId = reader.readOpaqueVarInt(),
-                epoch = reader.readUint64(),
-                contentType = ContentType.fromValue(reader.readUint8()),
-                authenticatedData = reader.readOpaqueVarInt(),
-                encryptedSenderData = reader.readOpaqueVarInt(),
-                ciphertext = reader.readOpaqueVarInt(),
+        /**
+         * Per-field DoS cap for `authenticated_data` and
+         * `encrypted_sender_data`. The underlying [TlsReader] already
+         * refuses any opaque<V> field larger than 1 MiB (its global
+         * `MAX_OPAQUE_SIZE`), but those fields shouldn't carry anything
+         * close to that — `encrypted_sender_data` is always exactly the
+         * sender-data AEAD ciphertext (a couple hundred bytes), and
+         * Marmot doesn't put anything in `authenticated_data` today.
+         * Tightening to 64 KiB makes the per-frame memory floor
+         * predictable and turns "we'd allocate up to 3 MiB on every
+         * inbound packet that escapes verification" into "we'll
+         * allocate at most ~1 MiB even before AEAD."
+         */
+        const val MAX_AAD_BYTES = 1 shl 16 // 64 KiB
+
+        fun decodeTls(reader: TlsReader): PrivateMessage {
+            val groupId = reader.readOpaqueVarInt()
+            val epoch = reader.readUint64()
+            val contentType = ContentType.fromValue(reader.readUint8())
+            val authenticatedData = reader.readOpaqueVarInt()
+            require(authenticatedData.size <= MAX_AAD_BYTES) {
+                "PrivateMessage authenticated_data too large: ${authenticatedData.size} > $MAX_AAD_BYTES"
+            }
+            val encryptedSenderData = reader.readOpaqueVarInt()
+            require(encryptedSenderData.size <= MAX_AAD_BYTES) {
+                "PrivateMessage encrypted_sender_data too large: ${encryptedSenderData.size} > $MAX_AAD_BYTES"
+            }
+            // `ciphertext` size is bounded by [TlsReader.MAX_OPAQUE_SIZE]
+            // (1 MiB) — which dominates any peer-side practical limit
+            // (Marmot kind:445 events ride a Nostr relay's per-event
+            // ceiling, typically 64 KiB). No tighter cap needed here.
+            val ciphertext = reader.readOpaqueVarInt()
+            return PrivateMessage(
+                groupId = groupId,
+                epoch = epoch,
+                contentType = contentType,
+                authenticatedData = authenticatedData,
+                encryptedSenderData = encryptedSenderData,
+                ciphertext = ciphertext,
            )
+        }
    }
 }

@@ -71,6 +71,23 @@ class SecretTree(

        /** Maximum consumed generation entries to track per sender before pruning. */
        const val MAX_CONSUMED_GENERATIONS_PER_SENDER = 1000
+
+        /**
+         * Hard cap on how many ratchet steps a single decrypt request may
+         * fast-forward through. Without this an attacker who can deliver a
+         * single PrivateMessage with a forged `generation` field can force
+         * the receiver into ~`generation` HKDF-Expand steps — trivially
+         * many seconds of CPU per packet. The skipped-key cache itself
+         * stops at [MAX_SKIPPED_KEYS] entries, but the ratchet keeps
+         * advancing past that point, so the cache cap doesn't bound
+         * compute cost.
+         *
+         * 4096 leaves room for a realistic application/handshake gap
+         * (e.g. mobile catching up after a long sleep) while making the
+         * attack worst-case ~4096 SHA-256 invocations — bounded enough
+         * that the receiver stays responsive.
+         */
+        const val MAX_RATCHET_STEPS_PER_CALL = 4096
    }

    /**
@@ -149,6 +166,15 @@ class SecretTree(
        require(generation >= state.applicationGeneration) {
            "Generation $generation already consumed (current: ${state.applicationGeneration})"
        }
+        // DoS guard: a malicious sender can put any uint32 in the
+        // PrivateMessage generation field, and without this cap a single
+        // packet would force unbounded HKDF-Expand work. See
+        // MAX_RATCHET_STEPS_PER_CALL.
+        require(generation - state.applicationGeneration <= MAX_RATCHET_STEPS_PER_CALL) {
+            "Application generation jump too large: " +
+                "${generation - state.applicationGeneration} steps (cap $MAX_RATCHET_STEPS_PER_CALL); " +
+                "sender $leafIndex from ${state.applicationGeneration} to $generation"
+        }

        // Replay detection: reject if this (sender, generation) was already used
        val senderConsumed = consumedGenerations.getOrPut(leafIndex) { mutableSetOf() }
@@ -219,6 +245,11 @@ class SecretTree(
        require(generation >= state.handshakeGeneration) {
            "Handshake generation $generation already consumed (current: ${state.handshakeGeneration})"
        }
+        require(generation - state.handshakeGeneration <= MAX_RATCHET_STEPS_PER_CALL) {
+            "Handshake generation jump too large: " +
+                "${generation - state.handshakeGeneration} steps (cap $MAX_RATCHET_STEPS_PER_CALL); " +
+                "sender $leafIndex from ${state.handshakeGeneration} to $generation"
+        }

        val senderConsumed = consumedHandshakeGenerations.getOrPut(leafIndex) { mutableSetOf() }
        require(generation !in senderConsumed) {
@@ -551,6 +551,67 @@ class MarmotMipBehaviorTest {
            )
        }

+    // ----------------------------------------------------------------------
+    // DoS guards: forward-ratchet cap + PrivateMessage size caps
+    // ----------------------------------------------------------------------
+
+    /**
+     * A receiver that's been silent at generation 0 for one peer must
+     * NOT be forced to do unbounded HKDF-Expand work just because the
+     * peer (or an attacker) sets a multi-billion `generation` field on
+     * an inbound PrivateMessage. The SecretTree caps the per-call
+     * ratchet jump at 4096 — anything past that throws before any HKDF
+     * runs.
+     */
+    @Test
+    fun secretTree_rejectsRatchetJumpsBeyondCap() {
+        val st =
+            com.vitorpamplona.quartz.marmot.mls.schedule.SecretTree(
+                encryptionSecret = ByteArray(32),
+                leafCount = 1,
+            )
+        // 4096 is allowed (boundary case — fast-forwards 4096 steps).
+        st.applicationKeyNonceForGeneration(0, 4096)
+        // 4097 above the previous head is rejected.
+        val ex =
+            assertFailsWith<IllegalArgumentException> {
+                st.applicationKeyNonceForGeneration(0, 4096 + 4097 + 1)
+            }
+        assertTrue(ex.message!!.contains("jump too large"), "must explain the cap: ${ex.message}")
+    }
+
+    /**
+     * Marmot tightens `authenticated_data` to 64 KiB even though the
+     * underlying TlsReader cap is 1 MiB — those fields are never
+     * legitimately that large, and tightening turns a 1 MiB pre-
+     * verification allocation into 64 KiB. Hand-build a frame with a
+     * 65 KiB authenticated_data and verify rejection.
+     */
+    @Test
+    fun privateMessage_rejectsOversizedAuthenticatedData() {
+        val w =
+            com.vitorpamplona.quartz.marmot.mls.codec
+                .TlsWriter()
+        w.putOpaqueVarInt(ByteArray(32)) // group_id
+        w.putUint64(0L) // epoch
+        w.putUint8(1) // content_type = APPLICATION
+        w.putOpaqueVarInt(ByteArray((1 shl 16) + 1)) // authenticated_data: 64 KiB + 1
+        w.putOpaqueVarInt(ByteArray(0)) // encrypted_sender_data
+        w.putOpaqueVarInt(ByteArray(64)) // ciphertext (small)
+        val ex =
+            assertFailsWith<IllegalArgumentException> {
+                com.vitorpamplona.quartz.marmot.mls.framing.PrivateMessage
+                    .decodeTls(
+                        com.vitorpamplona.quartz.marmot.mls.codec
+                            .TlsReader(w.toByteArray()),
+                    )
+            }
+        assertTrue(
+            ex.message!!.contains("authenticated_data too large"),
+            "must name the field: ${ex.message}",
+        )
+    }
+
    // ----------------------------------------------------------------------
    // RFC 9420 §7.9 parent_hash chain validation on Welcome
    // ----------------------------------------------------------------------