fix(marmot): a joiner needs its Welcome path secret, and a tree it can accept

Two MLS defects that between them meant nobody but a group's creator could
successfully invite anyone into a group with three or more members.

**GroupSecrets.path_secret was neither sent nor used.** A Commit's
UpdatePath refreshes every node from the committer's leaf to the root, and
a refreshed node has no unmerged leaves — so a member added by that same
Commit is MERGED at their common ancestor the instant it joins. RFC 9420
§12.4.1 also excludes newly-added leaves from the copath resolution, so
that ancestor's secret is not in the UpdatePath at all. The only place it
exists is `GroupSecrets.path_secret` (§12.4.3.1), and we sent `null` and
ignored what MDK sent us. The joiner therefore held nothing above its own
leaf, and the first commit from the other side of the tree — which
resolves the joiner's sibling subtree to that merged ancestor — was
undecryptable. MDK reported it exactly:

    UpdatePath at common ancestor carries no ciphertext for us
    (my_leaf=1, my_node=2, resolution=[1], held_path_nodes=[])

**Parent-hash validation was stricter than RFC 9420 and rejected valid
trees.** We re-derived every COMMIT-source leaf's `parent_hash` top-down
from the CURRENT tree and demanded a match. §7.9.2 makes a much weaker
claim, per PARENT node: for each non-blank parent P, exactly one of its
subtrees must contain a node whose `parent_hash` equals `ParentHash(P,
other_subtree)`. The strong version cannot hold — a later commit
refreshes ancestors and a later Add changes the tree's shape, so a leaf
set two epochs ago legitimately no longer re-derives — and it rejected the
GroupInfo of every group whose inviter was not the last committer.

Both halves need the RFC's `original_sibling_tree_hash`: the sibling
subtree's tree hash with the parent's `unmerged_leaves` removed. Those are
exactly the leaves added since the parent was populated, so excluding them
reconstructs the tree as the parent's author saw it. `RatchetTree` gains
`originalTreeHash` and `resolutionExcluding` for it.

The regression test builds the case that no two-party test can reach: a
member added by its own sibling, so its ancestor is merged on arrival,
followed by a commit from the other subtree. It fails on either half of
this change alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kCuA6tc4JQzHPCDd39GHq
This commit is contained in:
Claude
2026-09-09 07:03:18 +00:00
parent b29decc620
commit 6fb364394d
4 changed files with 283 additions and 103 deletions
@@ -67,6 +67,7 @@ import com.vitorpamplona.quartz.marmot.mls.tree.Extension
import com.vitorpamplona.quartz.marmot.mls.tree.LeafNode
import com.vitorpamplona.quartz.marmot.mls.tree.LeafNodeSource
import com.vitorpamplona.quartz.marmot.mls.tree.Lifetime
import com.vitorpamplona.quartz.marmot.mls.tree.PathSecretAndKey
import com.vitorpamplona.quartz.marmot.mls.tree.RatchetTree
import com.vitorpamplona.quartz.marmot.mls.tree.UpdatePathNode
import com.vitorpamplona.quartz.nip01Core.core.HexKey
@@ -938,7 +939,7 @@ class MlsGroup private constructor(
// Build Welcome for added members
val welcomeBytes =
if (addedMembers.isNotEmpty()) {
buildWelcome(addedMembers)
buildWelcome(addedMembers, pathSecrets)
} else {
null
}
@@ -2849,7 +2850,34 @@ class MlsGroup private constructor(
groupContext = groupContext.copy(extensions = newExtensions)
}
private fun buildWelcome(addedMembers: List<Pair<Int, MlsKeyPackage>>): ByteArray {
/**
* Lowest common ancestor of [myLeafIndex] and [otherLeafIndex] expressed as
* an index INTO our own direct path, or -1 when there is none.
*
* Our direct path runs leaf-ward to root-ward, so the first node it shares
* with the other leaf's direct path is their lowest common ancestor — and
* its position is also the index of that node's path secret in
* `derivePathSecrets`, which walks the same list.
*/
private fun directPathIndexOfAncestorWith(otherLeafIndex: Int): Int {
val mine = BinaryTree.directPath(myLeafIndex, tree.leafCount)
val theirs = BinaryTree.directPath(otherLeafIndex, tree.leafCount).toSet()
return mine.indexOfFirst { it in theirs }
}
/**
* @param committerPathSecrets the path secrets this commit minted for the
* committer's own direct path, in direct-path order. RFC 9420 §12.4.3.1:
* when the Commit carries an UpdatePath, each new member's GroupSecrets
* MUST carry the path secret at the lowest common ancestor of that
* member's leaf and the committer's. Without it the joiner holds no key
* for any ancestor, and the FIRST later commit that addresses it at one —
* which is every commit once its subtree is merged — is undecryptable.
*/
private fun buildWelcome(
addedMembers: List<Pair<Int, MlsKeyPackage>>,
committerPathSecrets: List<PathSecretAndKey>,
): ByteArray {
// Add ratchet tree as GroupInfo extension (RFC 9420 Section 12.4.3.3)
val treeWriter = TlsWriter()
tree.encodeTls(treeWriter)
@@ -2905,10 +2933,11 @@ class MlsGroup private constructor(
// Build per-member encrypted group secrets
val secrets =
addedMembers.map { (leafIdx, kp) ->
val ancestorIdx = directPathIndexOfAncestorWith(leafIdx)
val groupSecrets =
GroupSecrets(
joinerSecret = epochSecrets.joinerSecret,
pathSecret = null,
pathSecret = committerPathSecrets.getOrNull(ancestorIdx)?.pathSecret,
)
val gsBytes = groupSecrets.toTlsBytes()
@@ -3279,103 +3308,72 @@ class MlsGroup private constructor(
}
/**
* RFC 9420 §7.9 parent_hash chain verification for a STATIC tree —
* specifically, the ratchet_tree extension a joiner reconstructs
* from a Welcome's GroupInfo. Without this, a malicious or
* misconfigured GroupInfo signer could ship a tree whose stored
* parent_hash values are inconsistent with the actual tree shape;
* peers that DO validate would reject every commit produced from
* this tree, but the joiner wouldn't notice until the next epoch
* silently rolled back.
*
* For each leaf with `source == COMMIT` (the only source that
* carries a parent_hash payload), recompute the parent_hash chain
* top-down on the leaf's filtered direct path and verify the
* leaf's stored parent_hash matches what the chain produces.
* RFC 9420 §7.9.2 "Verifying Parent Hashes", over the STATIC tree a
* joiner reconstructs from a Welcome's GroupInfo.
*
* Without it a malicious or misconfigured GroupInfo signer could ship
* a tree whose stored parent_hash values do not match its shape; peers
* that DO validate would reject every commit produced from it, and the
* joiner would not notice until an epoch silently rolled back.
* Returns `null` on success, or a human-readable failure reason.
* Skips KEY_PACKAGE and UPDATE leaves — those don't carry a
* meaningful parent_hash on the wire.
*
* The rule is per PARENT node, not per leaf: for each non-blank parent
* P, EXACTLY ONE of its two subtrees must contain a node whose
* `parent_hash` equals `ParentHash(P, other_subtree)`. That node is the
* child the committer descended through when it set P; the other
* subtree supplies the sibling hash.
*
* We used to re-derive every COMMIT-source leaf's `parent_hash`
* top-down from the CURRENT tree and demand a match. That is a much
* stronger claim than the RFC makes, and a false one: a later commit
* refreshes ancestors and a later Add changes the tree's shape, so a
* leaf set two epochs ago legitimately no longer re-derives. It
* rejected every tree where the inviter was not the last committer —
* in practice, every group invitation sent by anyone but the creator.
*
* Both sibling hashes and both resolutions exclude P's
* `unmerged_leaves`: those are precisely the leaves added after P was
* populated, so removing them reconstructs the tree as P's author saw
* it.
*/
internal fun verifyTreeParentHashesForJoin(tree: RatchetTree): String? {
if (tree.leafCount == 0) return null
val nodeCount = BinaryTree.nodeCount(tree.leafCount)
for (leafIdx in 0 until tree.leafCount) {
val leaf = tree.getLeaf(leafIdx) ?: continue
if (leaf.leafNodeSource != LeafNodeSource.COMMIT) continue
val expected = computeStaticLeafParentHash(tree, leafIdx, nodeCount)
val stored = leaf.parentHash ?: ByteArray(0)
if (!stored.contentEquals(expected)) {
return "leaf $leafIdx parent_hash mismatch (stored=${stored.size}B, expected=${expected.size}B)"
for (parentIdx in tree.parentNodeIndices()) {
val key = tree.parentEncryptionKeyOf(parentIdx) ?: continue
val storedParentHash = tree.parentHashOf(parentIdx) ?: ByteArray(0)
val excluded = tree.unmergedLeavesOf(parentIdx)
val leftIdx = BinaryTree.left(parentIdx)
val rightIdx = BinaryTree.right(parentIdx)
fun hashWithSibling(siblingIdx: Int) =
MlsCryptoProvider.hash(
encodeParentHashInput(
encryptionKey = key,
parentHash = storedParentHash,
originalSiblingTreeHash = tree.originalTreeHash(siblingIdx, excluded),
),
)
val expectedInLeft = hashWithSibling(rightIdx)
val expectedInRight = hashWithSibling(leftIdx)
val foundLeft =
tree.resolutionExcluding(leftIdx, excluded).any {
tree.parentHashOf(it)?.contentEquals(expectedInLeft) == true
}
val foundRight =
tree.resolutionExcluding(rightIdx, excluded).any {
tree.parentHashOf(it)?.contentEquals(expectedInRight) == true
}
if (foundLeft == foundRight) {
return "parent node $parentIdx is not parent-hash valid " +
"(matched left=$foundLeft right=$foundRight)"
}
}
return null
}
/**
* Top-down recomputation of the parent_hash that a COMMIT-source
* leaf at [leafIdx] should carry, given the current tree shape.
* Mirrors [computeSenderParentHashes] but uses
* [RatchetTree.treeHashNode] for sibling tree hashes (no
* pre-update / post-update distinction in static validation).
*/
private fun computeStaticLeafParentHash(
tree: RatchetTree,
leafIdx: Int,
nodeCount: Int,
): ByteArray {
val (filteredDp, _) = tree.filteredDirectPath(leafIdx)
if (filteredDp.isEmpty()) return ByteArray(0)
// Walk top-down from root, propagating the expected parent_hash.
val hashes = mutableMapOf<Int, ByteArray>()
hashes[filteredDp.last()] = ByteArray(0)
for (i in filteredDp.size - 2 downTo 0) {
val xIdx = filteredDp[i]
val parentIdx = filteredDp[i + 1]
val parentNode = tree.getNode(parentIdx)
if (parentNode !is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent) {
hashes[xIdx] = ByteArray(0)
continue
}
// x's sibling under parent — parent has children left/right;
// sibling is whichever isn't x's ancestor.
val left = BinaryTree.left(parentIdx)
val right = BinaryTree.right(parentIdx)
val siblingIdx = if (xIdx == left) right else left
val siblingTreeHash = tree.treeHashNode(siblingIdx)
hashes[xIdx] =
MlsCryptoProvider.hash(
encodeParentHashInput(
encryptionKey = parentNode.parentNode.encryptionKey,
parentHash = hashes[parentIdx] ?: ByteArray(0),
originalSiblingTreeHash = siblingTreeHash,
),
)
}
// The leaf's expected parent_hash is the chain value AT the
// immediate parent (filteredDp[0]) — same convention as the
// committer-side computation in [computeSenderParentHashes].
val immediateParentIdx = filteredDp.first()
val immediateParent = tree.getNode(immediateParentIdx)
if (immediateParent !is com.vitorpamplona.quartz.marmot.mls.tree.TreeNode.Parent) {
return ByteArray(0)
}
// Sibling of the leaf's node at the immediate parent.
val leafNodeIdx = BinaryTree.leafToNode(leafIdx)
val left = BinaryTree.left(immediateParentIdx)
val right = BinaryTree.right(immediateParentIdx)
val leafSiblingIdx = if (leafNodeIdx == left) right else left
return MlsCryptoProvider.hash(
encodeParentHashInput(
encryptionKey = immediateParent.parentNode.encryptionKey,
parentHash = hashes[immediateParentIdx] ?: ByteArray(0),
originalSiblingTreeHash = tree.treeHashNode(leafSiblingIdx),
),
)
}
/**
* Default MLS leaf Capabilities that advertise support for Marmot's
* required extensions and proposals so new members can join a group
@@ -3760,17 +3758,31 @@ class MlsGroup private constructor(
interimInput.putOpaqueVarInt(confirmationTag)
val interimTranscriptHash = MlsCryptoProvider.hash(interimInput.toByteArray())
return MlsGroup(
groupContext = groupContext,
tree = tree,
myLeafIndex = myLeafIndex,
epochSecrets = epochSecrets,
secretTree = secretTree,
initSecret = epochSecrets.initSecret,
signingPrivateKey = bundle.signaturePrivateKey,
encryptionPrivateKey = bundle.encryptionPrivateKey,
interimTranscriptHash = interimTranscriptHash,
)
// RFC 9420 §12.4.3.1: when the Commit that added us carried an
// UpdatePath, GroupSecrets carries the path secret at the lowest
// common ancestor of our leaf and the committer's. Deriving our
// direct-path keys from it is not optional bookkeeping — our
// subtree is already MERGED in the tree this Welcome hands us, so
// the very next commit addresses us at an ancestor, and a joiner
// that dropped this secret cannot decrypt a single one of them.
val joined =
MlsGroup(
groupContext = groupContext,
tree = tree,
myLeafIndex = myLeafIndex,
epochSecrets = epochSecrets,
secretTree = secretTree,
initSecret = epochSecrets.initSecret,
signingPrivateKey = bundle.signaturePrivateKey,
encryptionPrivateKey = bundle.encryptionPrivateKey,
interimTranscriptHash = interimTranscriptHash,
)
groupSecrets.pathSecret?.let { pathSecret ->
val ancestorIdx = joined.directPathIndexOfAncestorWith(groupInfo.signer)
val fullPath = BinaryTree.directPath(myLeafIndex, tree.leafCount)
fullPath.getOrNull(ancestorIdx)?.let { joined.rememberPathKeys(it, pathSecret) }
}
return joined
}
/**
@@ -266,6 +266,113 @@ class RatchetTree(
return MlsCryptoProvider.hash(writer.toByteArray())
}
/**
* RFC 9420 §7.9.2 `original_sibling_tree_hash`: the tree hash of the
* subtree rooted at [nodeIndex] computed as if every leaf in
* [excludedLeaves] were blank.
*
* This is what makes a stored `parent_hash` verifiable LATER. The plain
* tree hash of a sibling subtree changes every time a leaf is added to it,
* so re-deriving a parent_hash from the current tree disagrees with the
* value its author computed — even though nothing about that author's
* commit was wrong. Excluding the parent's `unmerged_leaves` removes
* exactly the leaves added since the parent was last set, which is the set
* that moved.
*/
internal fun originalTreeHash(
nodeIndex: Int,
excludedLeaves: Set<Int>,
): ByteArray {
if (BinaryTree.isLeaf(nodeIndex)) {
val leafIndex = BinaryTree.nodeToLeaf(nodeIndex)
val writer = TlsWriter()
writer.putUint8(1)
writer.putUint32(leafIndex.toLong())
val leaf = getNode(nodeIndex).takeIf { leafIndex !in excludedLeaves }
if (leaf != null) {
writer.putUint8(1)
(leaf as TreeNode.Leaf).leafNode.encodeTls(writer)
} else {
writer.putUint8(0)
}
return MlsCryptoProvider.hash(writer.toByteArray())
}
val leftHash = originalTreeHash(BinaryTree.left(nodeIndex), excludedLeaves)
val rightHash = originalTreeHash(BinaryTree.right(nodeIndex), excludedLeaves)
val writer = TlsWriter()
writer.putUint8(2)
val parent = getNode(nodeIndex)
if (parent != null) {
writer.putUint8(1)
// The excluded leaves are removed from this node's own
// unmerged_leaves too: they are the leaves whose addition this
// hash is meant to be blind to.
val node = (parent as TreeNode.Parent).parentNode
node.copy(unmergedLeaves = node.unmergedLeaves.filterNot { it in excludedLeaves }).encodeTls(writer)
} else {
writer.putUint8(0)
}
writer.putOpaqueVarInt(leftHash)
writer.putOpaqueVarInt(rightHash)
return MlsCryptoProvider.hash(writer.toByteArray())
}
/**
* Resolution of [nodeIndex] with [excludedLeaves] treated as blank.
*
* Parent-hash validation has to reconstruct the tree as it stood when the
* parent was populated, and the leaves added since are exactly the ones in
* that parent's `unmerged_leaves`.
*/
fun resolutionExcluding(
nodeIndex: Int,
excludedLeaves: Set<Int>,
): List<Int> {
val node = getNode(nodeIndex)
if (BinaryTree.isLeaf(nodeIndex)) {
val leafIndex = BinaryTree.nodeToLeaf(nodeIndex)
return if (node == null || leafIndex in excludedLeaves) emptyList() else listOf(nodeIndex)
}
if (node != null) {
val result = mutableListOf(nodeIndex)
if (node is TreeNode.Parent) {
for (leaf in node.parentNode.unmergedLeaves) {
if (leaf !in excludedLeaves) result.add(BinaryTree.leafToNode(leaf))
}
}
return result
}
return resolutionExcluding(BinaryTree.left(nodeIndex), excludedLeaves) +
resolutionExcluding(BinaryTree.right(nodeIndex), excludedLeaves)
}
/** The `parent_hash` field a node carries, or null when it has none. */
internal fun parentHashOf(nodeIndex: Int): ByteArray? =
when (val node = getNode(nodeIndex)) {
is TreeNode.Parent -> node.parentNode.parentHash
is TreeNode.Leaf -> node.leafNode.parentHash
else -> null
}
/** The encryption key of the parent node at [nodeIndex], if it is one. */
internal fun parentEncryptionKeyOf(nodeIndex: Int): ByteArray? = (getNode(nodeIndex) as? TreeNode.Parent)?.parentNode?.encryptionKey
/** Node indices of every non-blank parent node, root-inclusive. */
internal fun parentNodeIndices(): List<Int> =
(0 until BinaryTree.nodeCount(_leafCount))
.filter { !BinaryTree.isLeaf(it) && getNode(it) is TreeNode.Parent }
/** Unmerged leaves recorded on the parent node at [nodeIndex], if any. */
internal fun unmergedLeavesOf(nodeIndex: Int): Set<Int> =
(getNode(nodeIndex) as? TreeNode.Parent)
?.parentNode
?.unmergedLeaves
?.toSet()
.orEmpty()
/**
* RFC 9420 §4.1.2 "filtered direct path":
* the direct path of a leaf node L, with any parent node removed whose
@@ -773,12 +773,18 @@ class MarmotMipBehaviorTest {
}
assertTrue(tamperedLeafIdx >= 0, "test setup must produce a COMMIT-source leaf")
// RFC 9420 §7.9.2 states the rule per PARENT node — exactly one of
// its subtrees must contain the matching parent_hash — so the
// rejection names the parent whose invariant broke rather than the
// leaf that was edited. The tamper is still caught: that leaf was
// the one descendant carrying its parent's expected hash.
val reason = MlsGroup.verifyTreeParentHashesForJoin(originalTree)
assertNotNull(reason)
assertTrue(
reason.contains("leaf $tamperedLeafIdx parent_hash mismatch"),
"rejection message must name the tampered leaf: $reason",
reason.contains("is not parent-hash valid"),
"rejection must name the parent whose invariant broke: $reason",
)
assertTrue(tamperedLeafIdx >= 0)
}
// ----------------------------------------------------------------------
@@ -127,6 +127,61 @@ class UpdatePathAncestorTest {
bob.exporterSecret("marmot", "group-event".encodeToByteArray(), 32),
)
}
/**
* The case MDK actually produced, and the one no two-party test can reach.
*
* A Commit's UpdatePath refreshes every node from the committer's leaf to
* the root, and a refreshed node has no unmerged leaves — so a member added
* by that same Commit is MERGED at their common ancestor the instant it
* joins. RFC 9420 §12.4.1 also excludes newly-added leaves from the copath
* resolution, so that ancestor's secret is not in the UpdatePath at all.
* The only place it exists is `GroupSecrets.path_secret` (§12.4.3.1).
*
* Drop it and the joiner holds nothing above its own leaf. The next commit
* from the OTHER side of the tree resolves the joiner's sibling subtree to
* that merged ancestor, and the joiner cannot decrypt it — which is exactly
* what MDK produced: `resolution=[1], held_path_nodes=[]`.
*/
@Test
fun aJoinerDerivesItsMergedAncestorKeyFromTheWelcomePathSecret() {
val alice = MlsGroup.create(identity = ByteArray(32) { 0x0a })
val bobBundle = bundleFor(0x0b)
val carolBundle = bundleFor(0x0c)
val daveBundle = bundleFor(0x0d)
alice.proposeAdd(bobBundle.keyPackage.toTlsBytes())
val addBob = alice.commit()
val bob = MlsGroup.processWelcome(assertNotNull(addBob.welcomeBytes), bobBundle)
alice.proposeAdd(carolBundle.keyPackage.toTlsBytes())
val addCarol = alice.commit()
bob.processFramedCommit(addCarol.framedCommitBytes)
val carol = MlsGroup.processWelcome(assertNotNull(addCarol.welcomeBytes), carolBundle)
// Dave is added by CAROL, his own sibling. Their common ancestor is the
// parent of their two leaves, and Carol's UpdatePath refreshes it — so
// Dave is merged there on arrival and gets that node's secret only
// through the Welcome.
carol.proposeAdd(daveBundle.keyPackage.toTlsBytes())
val addDave = carol.commit()
alice.processFramedCommit(addDave.framedCommitBytes)
bob.processFramedCommit(addDave.framedCommitBytes)
val dave = MlsGroup.processWelcome(assertNotNull(addDave.welcomeBytes), daveBundle)
// A commit from the other subtree. Dave's sibling subtree resolves to
// the merged ancestor, so this addresses Dave there, not at his leaf.
val aliceCommit = alice.commit()
bob.processFramedCommit(aliceCommit.framedCommitBytes)
carol.processFramedCommit(aliceCommit.framedCommitBytes)
dave.processFramedCommit(aliceCommit.framedCommitBytes)
assertEquals(alice.epoch, dave.epoch)
assertContentEquals(
alice.exporterSecret("marmot", "group-event".encodeToByteArray(), 32),
dave.exporterSecret("marmot", "group-event".encodeToByteArray(), 32),
)
}
}
private object MlsGroupStateCodec {