diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt index 3766a396f..5c3c28e66 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/schedule/SecretTree.kt @@ -47,9 +47,6 @@ class SecretTree( private val encryptionSecret: ByteArray, private val leafCount: Int, ) { - /** Cached tree node secrets, computed lazily */ - private val treeSecrets = mutableMapOf() - /** Per-sender ratchet state: (handshake generation, handshake secret, app generation, app secret) */ private val senderState = mutableMapOf() @@ -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 } } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt index 1aae80232..c74b1e6f0 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt @@ -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 diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupEdgeCaseTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupEdgeCaseTest.kt index 4647c3960..d77ff1e89 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupEdgeCaseTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupEdgeCaseTest.kt @@ -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) diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupLifecycleTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupLifecycleTest.kt index c16a0256e..3a829233d 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupLifecycleTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/mls/MlsGroupLifecycleTest.kt @@ -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() {