From a42499435b077d49a902a5db83f9acca4df0af93 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 04:22:42 +0000 Subject: [PATCH] =?UTF-8?q?fix(marmot):=20enforce=20required=5Fcapabilitie?= =?UTF-8?q?s=20on=20Add=20and=20Welcome=20(RFC=209420=20=C2=A77.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a group installs a `required_capabilities` extension (every Marmot group does, listing MarmotGroupData=0xF2EE, SelfRemove=0x000A, Basic credential), every member's leaf MUST advertise those types in its own [Capabilities]. We were validating none of that: - `applyProposalAdd` checked version + ciphersuite but never matched the new KP's capabilities against the group's required_capabilities. A non-conformant member silently joined; the next commit that touched their leaf got rejected by spec-conformant peers, splitting the group. - `processWelcome` similarly never checked the joiner's own KP against the group's required set, nor did it sweep existing members' leaves. A misconfigured GroupInfo signer could invite us into an incoherent group whose first commit we'd silently reject forever. Adds two helpers in `MlsGroup.Companion`: - `findRequiredCapabilities(extensions)`: decodes the §7.2 struct from a GroupContext extension list, or returns null if absent. - `requireCapabilitiesMeetRequirements(caps, req, who)`: throws with a specific (extensions=, proposals=, credentials=) diff naming the missing types — turns silent interop breaks into one debuggable line. Wires the gate in two places: - `applyProposalAdd` rejects the Add proposal if the new leaf falls short of the group's required set. - `processWelcome` rejects the join if either (a) our own KP doesn't meet the group's requirements or (b) any existing member's leaf doesn't — the latter catches a malformed GroupInfo at join time. 5 new tests: round-trip decoding, rejection on missing extension / proposal, acceptance when caps are a superset, and an end-to-end `addMember` rejection of a hand-crafted KP with SelfRemove stripped (re-signed so the rejection is from the capability gate, not the signature check). Quartz test suite green; marmot interop 16/16. Closes audit gaps #4, #5, #10. https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ --- .../quartz/marmot/mls/group/MlsGroup.kt | 91 +++++++++++ .../quartz/marmot/MarmotMipBehaviorTest.kt | 149 ++++++++++++++++++ 2 files changed, 240 insertions(+) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt index 994e006f6b..eee74727cc 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt @@ -2053,6 +2053,16 @@ class MlsGroup private constructor( "KeyPackage does not support ciphersuite 0x0001" } + // RFC 9420 §12.4.2: an Add proposal MUST be rejected if the new + // member's leaf capabilities don't advertise every type listed in + // the group's `required_capabilities` extension. Without this gate + // a non-conformant member silently joins, and any subsequent commit + // that touches their leaf is rejected by peers that DO enforce the + // requirement — splitting the group on the next epoch. + findRequiredCapabilities(groupContext.extensions)?.let { req -> + requireCapabilitiesMeetRequirements(caps, req, "Add KeyPackage leaf") + } + return tree.addLeaf(leafNode) } @@ -2545,6 +2555,67 @@ class MlsGroup private constructor( ) } + /** + * Parsed view of the RFC 9420 §7.2 `required_capabilities` extension. + * + * The on-wire struct is three `uint16` vectors — + * `extensions / proposals / credentials` — that name the types every + * member of the group MUST advertise in their leaf [Capabilities]. + */ + internal data class RequiredCapabilities( + val extensions: List, + val proposals: List, + val credentials: List, + ) + + /** + * Decode the `required_capabilities` extension from the GroupContext + * extension list, or `null` if the group hasn't installed one (some + * peers may omit it; treat missing as "no restriction"). + */ + internal fun findRequiredCapabilities(extensions: List): RequiredCapabilities? { + val ext = extensions.find { it.extensionType == REQUIRED_CAPABILITIES_EXTENSION_TYPE } ?: return null + val r = TlsReader(ext.extensionData) + + fun readUint16List(data: ByteArray): List { + val rr = TlsReader(data) + val list = mutableListOf() + while (rr.hasRemaining) list.add(rr.readUint16()) + return list + } + val exts = readUint16List(r.readOpaqueVarInt()) + val props = readUint16List(r.readOpaqueVarInt()) + val creds = readUint16List(r.readOpaqueVarInt()) + return RequiredCapabilities(exts, props, creds) + } + + /** + * RFC 9420 §7.2 + §12.4.2: every member's leaf [Capabilities] MUST + * advertise every type listed in the group's `required_capabilities` + * extension. Adding a member whose KeyPackage doesn't meet the + * requirement leaves the group in a non-conformant state where + * peers that DO enforce the requirement will reject any commit that + * touches that leaf. + * + * Throws [IllegalStateException] with a precise diff so debugging + * an interop break against another implementation is one log line. + */ + internal fun requireCapabilitiesMeetRequirements( + caps: Capabilities, + req: RequiredCapabilities, + who: String, + ) { + val missingExts = req.extensions.filter { it !in caps.extensions } + val missingProps = req.proposals.filter { it !in caps.proposals } + val missingCreds = req.credentials.filter { it !in caps.credentials } + if (missingExts.isNotEmpty() || missingProps.isNotEmpty() || missingCreds.isNotEmpty()) { + throw IllegalStateException( + "$who capabilities don't meet required_capabilities: " + + "missing extensions=$missingExts proposals=$missingProps credentials=$missingCreds", + ) + } + } + /** * Default MLS leaf Capabilities that advertise support for Marmot's * required extensions and proposals so new members can join a group @@ -2734,6 +2805,26 @@ class MlsGroup private constructor( "GroupInfo tree_hash does not match ratchet_tree extension" } + // RFC 9420 §7.2 + §12.4.3.1: a joiner MUST refuse to join a + // group whose `required_capabilities` lists types the joiner's + // own KeyPackage doesn't advertise — peers that DO enforce the + // requirement would reject every commit the joiner produces + // anyway. Catching this at join time turns a silent "your + // commits get dropped forever" into an actionable error. + findRequiredCapabilities(groupContext.extensions)?.let { req -> + val myLeaf = tree.getLeaf(myLeafIndex) + requireNotNull(myLeaf) { "Joiner's leaf is blank after tree reconstruction" } + requireCapabilitiesMeetRequirements(myLeaf.capabilities, req, "Joiner KeyPackage") + // Mirror the same check across every existing member — + // if a peer's leaf doesn't meet the group's stated + // requirements, the GroupInfo signer mis-installed the + // requirement set and the group is incoherent. + for (i in 0 until tree.leafCount) { + val leaf = tree.getLeaf(i) ?: continue + requireCapabilitiesMeetRequirements(leaf.capabilities, req, "Member leaf $i") + } + } + // Derive epoch secrets directly from memberSecret (RFC 9420 Section 8.3) // For Welcome, epoch_secret = ExpandWithLabel(member_secret, "epoch", GroupContext, Nh) val epochSecret = diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index 546b725972..18bf4e76fb 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -551,6 +551,155 @@ class MarmotMipBehaviorTest { ) } + // ---------------------------------------------------------------------- + // RFC 9420 §7.2 / §12.4.2 required_capabilities enforcement + // ---------------------------------------------------------------------- + + /** + * Round-trip: the `required_capabilities` extension Marmot installs on + * every fresh group decodes back to its declared (extensions, proposals, + * credentials) triple. + */ + @Test + fun findRequiredCapabilities_decodesMarmotExtensionInstalledByCreate() { + val alice = MlsGroup.create(aliceId.hexToByteArray()) + val req = + MlsGroup.findRequiredCapabilities(alice.extensions) + ?: error("required_capabilities must be present after create()") + assertEquals(listOf(0xF2EE), req.extensions, "MarmotGroupData (0xF2EE) must be required") + assertEquals(listOf(0x000A), req.proposals, "SelfRemove (0x000A) must be required") + assertEquals(listOf(0x0001), req.credentials, "Basic credential must be required") + } + + /** + * `requireCapabilitiesMeetRequirements` must throw when ANY required + * type is missing — extension OR proposal OR credential — and the + * error must name the missing types so an interop debugger can + * diagnose without grepping. + */ + @Test + fun requireCapabilitiesMeetRequirements_rejectsMissingExtension() { + val req = + MlsGroup.Companion.RequiredCapabilities( + extensions = listOf(0xF2EE), + proposals = listOf(0x000A), + credentials = listOf(0x0001), + ) + // Missing 0xF2EE. + val caps = + com.vitorpamplona.quartz.marmot.mls.tree.Capabilities( + extensions = emptyList(), + proposals = listOf(0x000A), + credentials = listOf(0x0001), + ) + val ex = + assertFailsWith { + MlsGroup.requireCapabilitiesMeetRequirements(caps, req, "test") + } + assertTrue( + ex.message!!.contains("extensions=[62190]") || ex.message!!.contains("0xF2EE"), + "error must name the missing extension type: ${ex.message}", + ) + } + + @Test + fun requireCapabilitiesMeetRequirements_rejectsMissingProposal() { + val req = + MlsGroup.Companion.RequiredCapabilities( + extensions = emptyList(), + proposals = listOf(0x000A), + credentials = emptyList(), + ) + val caps = + com.vitorpamplona.quartz.marmot.mls.tree.Capabilities( + extensions = emptyList(), + proposals = emptyList(), + credentials = listOf(0x0001), + ) + assertFailsWith { + MlsGroup.requireCapabilitiesMeetRequirements(caps, req, "test") + } + } + + @Test + fun requireCapabilitiesMeetRequirements_passesWhenCapsAreSuperset() { + val req = + MlsGroup.Companion.RequiredCapabilities( + extensions = listOf(0xF2EE), + proposals = listOf(0x000A), + credentials = listOf(0x0001), + ) + val caps = + com.vitorpamplona.quartz.marmot.mls.tree.Capabilities( + extensions = listOf(0xF2EE, 0x1234), + proposals = listOf(0x000A, 0x000B), + credentials = listOf(0x0001, 0x0002), + ) + // Should not throw. + MlsGroup.requireCapabilitiesMeetRequirements(caps, req, "test") + } + + /** + * End-to-end gate: `addMember` MUST reject a KeyPackage whose leaf + * doesn't advertise the group's `required_capabilities`. We tamper the + * KP's leaf capabilities to drop SelfRemove, then re-encode and re-sign + * to keep the KP signature valid (RFC 9420 §10.1) so the rejection is + * coming from the capability gate and not the signature check. + */ + @Test + fun addMember_rejectsKeyPackageMissingRequiredProposal() = + runBlocking { + // Setup: standard Marmot group (required_capabilities lists + // SelfRemove + MarmotGroupData + Basic). + val manager = createGroupManager() + manager.createGroup(groupId, aliceId.hexToByteArray()) + + // Bob's KP, but with SelfRemove stripped from his leaf + // capabilities. He's still announcing himself as a Marmot peer + // — just lying about supporting SelfRemove. + val tampered = createKeyPackageWithoutSelfRemove(bobId) + + assertFailsWith { + manager.addMember(groupId, tampered.toTlsBytes()) + } + } + + /** + * Build a KeyPackage whose leaf [Capabilities] does NOT list 0x000A + * (SelfRemove), then re-sign so the KP's outer signature still + * validates. Useful for testing the §7.2 gate in isolation. + */ + private fun createKeyPackageWithoutSelfRemove(identity: String): com.vitorpamplona.quartz.marmot.mls.messages.MlsKeyPackage { + val tempGroup = MlsGroup.create(identity.hexToByteArray()) + val bundle = tempGroup.createKeyPackage(identity.hexToByteArray(), ByteArray(0)) + val original = bundle.keyPackage + val originalLeaf = original.leafNode + + // Strip SelfRemove (0x000A) from the leaf's advertised proposals. + val tamperedCaps = + originalLeaf.capabilities.copy( + proposals = originalLeaf.capabilities.proposals.filter { it != 0x000A }, + ) + // Re-build the leaf node and re-sign its TBS so the leaf signature + // still verifies (otherwise we'd hit the LeafNode signature check + // before the capability gate fires). + val tamperedLeaf = + originalLeaf.copy(capabilities = tamperedCaps).let { lf -> + val tbs = lf.encodeTbs() + val sig = + com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider + .signWithLabel(bundle.signaturePrivateKey, "LeafNodeTBS", tbs) + lf.copy(signature = sig) + } + // Re-sign the KeyPackage TBS over the new leaf. + val unsigned = original.copy(leafNode = tamperedLeaf, signature = ByteArray(0)) + return unsigned.copy( + signature = + com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider + .signWithLabel(bundle.signaturePrivateKey, "KeyPackageTBS", unsigned.encodeTbs()), + ) + } + // ---------------------------------------------------------------------- // RFC 9420 §5.3 psk_secret derivation // ----------------------------------------------------------------------