nick/amethyst
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(marmot): derive SecretTree leaf secrets by walking down from root

Browse Source 
Fred (third member of a three-member group, `leafIndex = 2`) crashed
with `StackOverflowError` on ART / NPE on JVM the moment he tried to
send his first application message. The recursion / null is in
`SecretTree.getNodeSecret`, which assumes `nodeIndex` is always a
direct child of `BinaryTree.parent(nodeIndex)`:

    val parentIdx = BinaryTree.parent(nodeIndex, BinaryTree.nodeCount(leafCount))
    val parentSecret = getNodeSecret(parentIdx)
    val leftIdx = BinaryTree.left(parentIdx)
    val rightIdx = BinaryTree.right(parentIdx)
    treeSecrets[leftIdx] = leftSecret
    treeSecrets[rightIdx] = rightSecret
    return treeSecrets[nodeIndex]!!

That invariant holds only for power-of-two trees. For Fred (leafCount
= 3, nodeCount = 5), `BinaryTree.parent(4, 5)` walks via
`parentInRange(5, 5)` and returns **3** (the root) — skipping the
virtual intermediate node 5 that would normally sit between node 4
and the root in a 4-leaf tree. But `left(3) = 1` and `right(3) = 5`;
neither is 4. The cache fills entries 1 and 5 from the root's secret,
node 4's entry is never populated, and the final
`treeSecrets[nodeIndex]!!` throws NPE. On Android the error path
instead re-enters `getNodeSecret` until the stack is exhausted.

The fix walks **down** from the root to the target leaf, picking
left or right at each step based on `targetNode < currentNode`
(holds in left-balanced MLS trees) and using `BinaryTree.left` /
`BinaryTree.right` which correctly descend through virtual
intermediate nodes. The node-level cache is no longer needed —
`getOrInitSender` caches the per-sender ratchet state once, so
re-deriving the leaf secret each time a new sender is initialized
costs a handful of HKDF calls at worst.

Side effects (all in the right direction):

- Re-enabled 6 previously `@Ignore`d tests in
  `MlsGroupLifecycleTest` and `MlsGroupEdgeCaseTest` that exercised
  exactly the three-member / non-full-tree path this fix addresses:
  `testThreeMemberGroup_SequentialAdditions`,
  `testExternalJoin_ZaraJoinsViaGroupInfo`,
  `testExternalJoin_ExporterSecretsAgree`,
  `testSigningKeyRotation_EpochAdvances`,
  `testEncryptDecryptAfterSigningKeyRotation`,
  `testPskProposal_EpochAdvancesWithPsk`. They now pass.

- `testEmptyCommit_AdvancesEpoch` and
  `testMultipleEpochTransitions_EncryptDecryptStillWorks` remain
  `@Ignore`d — they hit a **different** pre-existing bug where
  Alice's and Bob's exporter secrets diverge after a no-proposal
  commit (commit with only an UpdatePath). Unrelated to the
  non-full-tree fix; tracked separately. I retagged them with an
  explanatory note instead of the old "see
  testThreeMemberGroup_SequentialAdditions" reference.

- Fixed an unrelated typo in
  `testMultipleEpochTransitions_EncryptDecryptStillWorks` that
  asserted `7L` and then `6L` for the same epoch value.

Regression test:
- `testFredEncryptsAfterJoiningThreeMemberGroup` creates a
  three-member group via the production path (Alice adds Bob, then
  adds Fred; Bob processes the add-Fred commit), then has Fred
  encrypt an application message and asserts both Alice and Bob can
  decrypt it. Without the fix this throws NPE / stack-overflows at
  `encrypt()`.

https://claude.ai/code/session_014zfdNeeKAfU1zGGyFw4bUL
This commit is contained in:
Claude
2026-04-21 01:49:05 +00:00
parent b039543807
commit 621fe9c8c5
4 changed files with 112 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt
+41 -34
View File
@@ -47,9 +47,6 @@ class SecretTree(
private val encryptionSecret: ByteArray,
private val leafCount: Int,
) {
/** Cached tree node secrets, computed lazily */
private val treeSecrets = mutableMapOf<Int, ByteArray>()
/** Per-sender ratchet state: (handshake generation, handshake secret, app generation, app secret) */
private val senderState = mutableMapOf<Int, SenderRatchetState>()
@@ -72,11 +69,6 @@ class SecretTree(
const val MAX_CONSUMED_GENERATIONS_PER_SENDER = 1000
}
init {
// Seed the root
treeSecrets[BinaryTree.root(leafCount)] = encryptionSecret
}
/**
* Get the next (key, nonce) for application messages from a given sender.
*/
@@ -252,36 +244,51 @@ class SecretTree(
}
/**
* Derive the leaf secret from the encryption secret using the tree structure.
* Derive the leaf secret from the encryption secret by walking DOWN the
* binary tree from the root to the target leaf (RFC 9420 §9).
*
* At each step we pick left or right based on which subtree contains the
* target. In an MLS left-balanced tree the left-subtree node indices are
* always strictly less than the current node, and the right-subtree
* indices strictly greater — so the direction is simply
* `targetNode < currentNode`.
*
* This also correctly handles non-power-of-2 leaf counts (3, 5, 6, 7, 9
* …) where the rightmost leaves live beneath "virtual" intermediate
* nodes that are beyond `nodeCount`. Walking down still succeeds because
* [BinaryTree.left] / [BinaryTree.right] give the correct descendants
* even for virtual nodes, and the target leaf is reachable via a chain
* of left/right steps that mixes real and virtual intermediates.
*
* The previous implementation derived the target's secret from
* `BinaryTree.parent(target)` which, for leaves on the right edge of a
* non-full tree, returned a high ancestor (often the root) that is NOT
* the target's direct parent. Its `left()` and `right()` then pointed
* at different nodes entirely, the target stayed un-cached, and the
* follow-up `return treeSecrets[nodeIndex]!!` either threw NPE (on
* JVM) or — when repeatedly re-entered — recursed into
* [getNodeSecret] until the stack was exhausted (on ART).
*/
private fun getLeafSecret(leafIndex: Int): ByteArray {
val nodeIndex = BinaryTree.leafToNode(leafIndex)
return getNodeSecret(nodeIndex)
}
val targetNode = BinaryTree.leafToNode(leafIndex)
val rootIdx = BinaryTree.root(leafCount)
var currentSecret = encryptionSecret
var currentNode = rootIdx
/**
* Recursively derive a node's secret from its parent in the secret tree.
*/
private fun getNodeSecret(nodeIndex: Int): ByteArray {
treeSecrets[nodeIndex]?.let { return it }
while (currentNode != targetNode) {
val goLeft = targetNode < currentNode
val label = if (goLeft) "left" else "right"
currentSecret =
MlsCryptoProvider.expandWithLabel(
currentSecret,
"tree",
label.encodeToByteArray(),
MlsCryptoProvider.HASH_OUTPUT_LENGTH,
)
currentNode = if (goLeft) BinaryTree.left(currentNode) else BinaryTree.right(currentNode)
}
val parentIdx = BinaryTree.parent(nodeIndex, BinaryTree.nodeCount(leafCount))
val parentSecret = getNodeSecret(parentIdx)
// Derive left and right children secrets
val leftIdx = BinaryTree.left(parentIdx)
val rightIdx = BinaryTree.right(parentIdx)
val leftSecret = MlsCryptoProvider.expandWithLabel(parentSecret, "tree", "left".encodeToByteArray(), MlsCryptoProvider.HASH_OUTPUT_LENGTH)
val rightSecret = MlsCryptoProvider.expandWithLabel(parentSecret, "tree", "right".encodeToByteArray(), MlsCryptoProvider.HASH_OUTPUT_LENGTH)
treeSecrets[leftIdx] = leftSecret
treeSecrets[rightIdx] = rightSecret
// Clear parent secret for forward secrecy
treeSecrets.remove(parentIdx)
return treeSecrets[nodeIndex]!!
return currentSecret
}
}
quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt
+61
View File
@@ -428,6 +428,67 @@ class MarmotPipelineTest {
}
}
@Test
fun testFredEncryptsAfterJoiningThreeMemberGroup() {
// Reproduces the production StackOverflowError: the last-joined member
// (Fred at leafIndex=2 in a 3-member group) attempts to encrypt his
// first message and the SecretTree.getNodeSecret recursion never
// terminates.
runBlocking {
val aliceMgr = createGroupManager()
val bobMgr = createGroupManager()
val fredMgr = createGroupManager()
val aliceIdBytes = ByteArray(32) { 0xA1.toByte() }
aliceMgr.createGroup(groupId, aliceIdBytes)
aliceMgr.updateGroupExtensions(
nostrGroupId = groupId,
extensions =
listOf(
com.vitorpamplona.quartz.marmot.mip01Groups
.MarmotGroupData(
nostrGroupId = groupId,
adminPubkeys = listOf(aliceIdBytes.toHexKey()),
).toExtension(),
),
)
val aliceGroup = aliceMgr.getGroup(groupId)!!
val bobBundle = aliceGroup.createKeyPackage(ByteArray(32) { 0xB2.toByte() }, ByteArray(0))
val fredBundle = aliceGroup.createKeyPackage(ByteArray(32) { 0xF3.toByte() }, ByteArray(0))
// Alice adds Bob.
val addBob = aliceMgr.addMember(groupId, bobBundle.keyPackage.toTlsBytes())
bobMgr.processWelcome(addBob.welcomeBytes!!, bobBundle)
// Alice adds Fred (he joins at leafIndex=2, in a 3-leaf tree).
val addFred = aliceMgr.addMember(groupId, fredBundle.keyPackage.toTlsBytes())
fredMgr.processWelcome(addFred.welcomeBytes!!, fredBundle)
// Bob (existing member at the pre-add-Fred epoch) receives and
// applies Alice's add-Fred commit to reach the same epoch as
// Alice + Fred. Without this, Bob can't decrypt Fred's subsequent
// application messages.
bobMgr.processCommit(
nostrGroupId = groupId,
commitBytes = addFred.commitBytes,
senderLeafIndex = aliceMgr.getGroup(groupId)!!.leafIndex,
confirmationTag = ByteArray(0),
)
// Sanity: Fred is at leafIndex=2, leafCount=3.
assertEquals(2, fredMgr.getGroup(groupId)!!.leafIndex)
// This is where production crashed with StackOverflowError.
val fredMsg = "hi from fred"
val fredCiphertext = fredMgr.encrypt(groupId, fredMsg.encodeToByteArray())
// And Alice & Bob must be able to decrypt it.
val aliceDecrypted = aliceMgr.decrypt(groupId, fredCiphertext)
assertEquals(fredMsg, aliceDecrypted.content.decodeToString())
val bobDecrypted = bobMgr.decrypt(groupId, fredCiphertext)
assertEquals(fredMsg, bobDecrypted.content.decodeToString())
}
}
@Test
fun testAddMemberExposesPreCommitExporterSecret() {
// Regression: before the fix, MarmotManager.addMember outer-encrypted
quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupEdgeCaseTest.kt
+5 -4
View File
@@ -225,9 +225,11 @@ class MlsGroupEdgeCaseTest {
// 6. Multiple epochs of encrypt/decrypt
// -----------------------------------------------------------------------
// BUG: processCommit key derivation diverges — commit_secret decryption from
// UpdatePath does not correctly derive matching epoch secrets between commit()
// and processCommit(). See MlsGroupLifecycleTest.testThreeMemberGroup_SequentialAdditions.
// BUG: same empty-commit (no proposals, pure UpdatePath) divergence as
// MlsGroupLifecycleTest.testEmptyCommit_AdvancesEpoch. The
// SecretTree.getNodeSecret non-full-tree fix doesn't address this — after
// 5 empty commits Alice and Bob's ratchet secrets diverge and AEAD
// decryption fails with "Tag mismatch". Tracked separately.
@Ignore
@Test
fun testMultipleEpochTransitions_EncryptDecryptStillWorks() {
@@ -242,7 +244,6 @@ class MlsGroupEdgeCaseTest {
bob.processCommit(commitResult.commitBytes, alice.leafIndex, ByteArray(0))
}
assertEquals(7L, alice.epoch) // epoch 0 + addMember(1) + 5 commits = 6... wait
// epoch 0 (create) -> epoch 1 (add bob) -> 5 empty commits = epoch 6
assertEquals(6L, alice.epoch)
assertEquals(alice.epoch, bob.epoch)
quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupLifecycleTest.kt
+5 -7
View File
@@ -176,7 +176,6 @@ class MlsGroupLifecycleTest {
// because the commit_secret decryption from the UpdatePath does not
// correctly walk the ratchet tree to find the common ancestor's path secret.
// This causes AEAD decryption failures on cross-member messages.
@Ignore
@Test
fun testThreeMemberGroup_SequentialAdditions() {
// Alice creates the group
@@ -243,7 +242,6 @@ class MlsGroupLifecycleTest {
// -----------------------------------------------------------------------
// BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
@Ignore
@Test
fun testExternalJoin_ZaraJoinsViaGroupInfo() {
val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -266,7 +264,6 @@ class MlsGroupLifecycleTest {
}
// BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
@Ignore
@Test
fun testExternalJoin_ExporterSecretsAgree() {
val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -312,7 +309,6 @@ class MlsGroupLifecycleTest {
// -----------------------------------------------------------------------
// BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
@Ignore
@Test
fun testSigningKeyRotation_EpochAdvances() {
val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -342,7 +338,6 @@ class MlsGroupLifecycleTest {
}
// BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
@Ignore
@Test
fun testEncryptDecryptAfterSigningKeyRotation() {
val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -412,7 +407,6 @@ class MlsGroupLifecycleTest {
// -----------------------------------------------------------------------
// BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
@Ignore
@Test
fun testPskProposal_EpochAdvancesWithPsk() {
val alice = MlsGroup.create("alice".encodeToByteArray())
@@ -472,7 +466,11 @@ class MlsGroupLifecycleTest {
// 12. Empty commit (no proposals, just UpdatePath for forward secrecy)
// -----------------------------------------------------------------------
// BUG: processCommit key derivation diverges — see testThreeMemberGroup_SequentialAdditions
// BUG: empty-commit (no proposals, pure UpdatePath) diverges Alice's and
// Bob's exporter secrets after processCommit. Unrelated to the SecretTree
// non-full-tree fix — the other @Ignored "processCommit diverges" tests in
// this file and MlsGroupEdgeCaseTest now pass, but this one still fails.
// Tracked separately.
@Ignore
@Test
fun testEmptyCommit_AdvancesEpoch() {