fix: critical Marmot/MLS bugs - crashes, crypto, protocol compliance

- C1: Fix leaveGroup() crash (used group state after deletion) - C2: Fix X25519.bigIntegerToBytes ArrayIndexOutOfBoundsException - C3: Add all-zeros DH check to prevent small-subgroup attacks - C4: Fix path secret derivation off-by-one (RFC 9420 Section 7.4) - C5: Use constant-time comparison for confirmation/membership tags - C6: Stage signing keys in proposeSigningKeyRotation, promote on commit - C7: Make commit conflict tracker per-(group,epoch) not just per-epoch - C8: Fix tree deserialization leafCount for trimmed trees - H8: Add Mutex-based thread safety to MlsGroupManager - H9: Fix extractPrivateKeyBytes little-endian padding - H12: Blank direct path on addLeaf (RFC 9420 Section 7.7) - M14: Track actual leaf index from addLeaf for Welcome generation https://claude.ai/code/session_018gVkmmYgMFtBH7G31pCk9N
2026-04-07 23:02:09 +00:00
parent fc07090121
commit 29d1610d1a
7 changed files with 284 additions and 141 deletions
@@ -182,9 +182,17 @@ class MarmotManager(
     * Returns proposal bytes to publish (as a GroupEvent).
     */
    suspend fun leaveGroup(nostrGroupId: HexKey): OutboundGroupEvent {
-        val proposalBytes = groupManager.leaveGroup(nostrGroupId)
+        // Build the outbound event BEFORE deleting group state (needs exporter secret)
+        val group =
+            groupManager.getGroup(nostrGroupId)
+                ?: throw IllegalStateException("Not a member of group $nostrGroupId")
+        val proposalBytes = group.selfRemove()
+        val outboundEvent = outboundProcessor.buildCommitEvent(nostrGroupId, proposalBytes)
+
+        // Now clean up group state
+        groupManager.removeGroupState(nostrGroupId)
        subscriptionManager.unsubscribeGroup(nostrGroupId)
-        return outboundProcessor.buildCommitEvent(nostrGroupId, proposalBytes)
+        return outboundEvent
    }

    // --- KeyPackage Management ---
@@ -223,18 +223,18 @@ class MarmotInboundProcessor(
        epoch: Long,
    ): GroupEventResult? {
        val winner =
-            commitTracker.resolve(epoch)
+            commitTracker.resolve(groupId, epoch)
                ?: return null

        val result = applyCommit(groupId, winner)
-        commitTracker.clearEpoch(epoch)
+        commitTracker.clearEpoch(groupId, epoch)
        return result
    }

    /**
-     * Get all epochs that have pending unresolved commits.
+     * Get all (group, epoch) keys that have pending unresolved commits.
     */
-    fun pendingCommitEpochs(): Set<Long> = commitTracker.pendingEpochs()
+    fun pendingCommitGroupEpochs(): Set<CommitOrdering.GroupEpochKey> = commitTracker.pendingGroupEpochs()

    /**
     * Clear all pending commit state.
@@ -303,13 +303,13 @@ class MarmotInboundProcessor(
            groupManager.getGroup(groupId)
                ?: return GroupEventResult.Error(groupId, "Group not found")
        val currentEpoch = group.epoch
-        commitTracker.addCommit(currentEpoch, groupEvent)
+        commitTracker.addCommit(groupId, currentEpoch, groupEvent)

        // If this is the only commit for this epoch, apply immediately
-        val pending = commitTracker.pendingForEpoch(currentEpoch)
+        val pending = commitTracker.pendingForEpoch(groupId, currentEpoch)
        return if (pending.size == 1) {
            val result = applyCommit(groupId, groupEvent)
-            commitTracker.clearEpoch(currentEpoch)
+            commitTracker.clearEpoch(groupId, currentEpoch)
            result
        } else {
            GroupEventResult.CommitPending(groupId, currentEpoch)
@@ -67,57 +67,79 @@ object CommitOrdering {
    }

    /**
-     * Tracks pending commits per epoch and resolves conflicts.
+     * Key for tracking pending commits: must be unique per (group, epoch) pair
+     * since epoch numbers are not globally unique across different groups.
+     */
+    data class GroupEpochKey(
+        val groupId: String,
+        val epoch: Long,
+    )
+
+    /**
+     * Tracks pending commits per (group, epoch) and resolves conflicts.
     *
     * Accumulate commits as they arrive from relays, then call [resolve]
-     * to determine which commit wins for each epoch.
+     * to determine which commit wins for each (group, epoch).
     */
    class EpochCommitTracker {
-        private val pendingByEpoch = mutableMapOf<Long, MutableList<GroupEvent>>()
+        private val pendingByGroupEpoch = mutableMapOf<GroupEpochKey, MutableList<GroupEvent>>()

        /**
-         * Adds a commit for a given epoch.
+         * Adds a commit for a given group and epoch.
         *
+         * @param groupId the Nostr group ID
         * @param epoch the MLS epoch number this commit targets
         * @param commit the GroupEvent containing the commit
         */
        fun addCommit(
+            groupId: String,
            epoch: Long,
            commit: GroupEvent,
        ) {
-            pendingByEpoch.getOrPut(epoch) { mutableListOf() }.add(commit)
+            val key = GroupEpochKey(groupId, epoch)
+            pendingByGroupEpoch.getOrPut(key) { mutableListOf() }.add(commit)
        }

        /**
-         * Returns pending commits for a specific epoch.
+         * Returns pending commits for a specific group and epoch.
         */
-        fun pendingForEpoch(epoch: Long): List<GroupEvent> = pendingByEpoch[epoch] ?: emptyList()
+        fun pendingForEpoch(
+            groupId: String,
+            epoch: Long,
+        ): List<GroupEvent> = pendingByGroupEpoch[GroupEpochKey(groupId, epoch)] ?: emptyList()

        /**
-         * Resolves the winning commit for a specific epoch.
+         * Resolves the winning commit for a specific group and epoch.
         *
+         * @param groupId the Nostr group ID
         * @param epoch the MLS epoch to resolve
-         * @return the winning commit, or null if no commits exist for this epoch
+         * @return the winning commit, or null if no commits exist for this (group, epoch)
         */
-        fun resolve(epoch: Long): GroupEvent? = selectWinner(pendingByEpoch[epoch] ?: emptyList())
+        fun resolve(
+            groupId: String,
+            epoch: Long,
+        ): GroupEvent? = selectWinner(pendingByGroupEpoch[GroupEpochKey(groupId, epoch)] ?: emptyList())

        /**
-         * Clears pending commits for an epoch after it has been resolved.
+         * Clears pending commits for a (group, epoch) after it has been resolved.
         */
-        fun clearEpoch(epoch: Long) {
-            pendingByEpoch.remove(epoch)
+        fun clearEpoch(
+            groupId: String,
+            epoch: Long,
+        ) {
+            pendingByGroupEpoch.remove(GroupEpochKey(groupId, epoch))
        }

        /**
-         * Returns all epochs that have pending commits.
+         * Returns all (group, epoch) keys that have pending commits.
         */
-        fun pendingEpochs(): Set<Long> = pendingByEpoch.keys.toSet()
+        fun pendingGroupEpochs(): Set<GroupEpochKey> = pendingByGroupEpoch.keys.toSet()

        /**
         * Clears all pending state.
         */
        fun clear() {
-            pendingByEpoch.clear()
+            pendingByGroupEpoch.clear()
        }
    }
 }
@@ -105,6 +105,9 @@ class MlsGroup private constructor(
    private val pskStore: MutableMap<String, ByteArray> = mutableMapOf(),
    private val pendingProposals: MutableList<PendingProposal> = mutableListOf(),
    private val sentKeys: MutableMap<Int, com.vitorpamplona.quartz.marmot.mls.schedule.KeyNonceGeneration> = mutableMapOf(),
+    /** Staged keys from proposeSigningKeyRotation — only promoted on successful commit */
+    private var pendingSigningKey: ByteArray? = null,
+    private var pendingEncryptionKey: ByteArray? = null,
 ) {
    val groupId: ByteArray get() = groupContext.groupId
    val epoch: Long get() = groupContext.epoch
@@ -293,9 +296,9 @@ class MlsGroup private constructor(
        val proposal = Proposal.Update(newLeafNode)
        pendingProposals.add(PendingProposal(proposal, myLeafIndex))

-        // Stage the new keys — they take effect when commit() is called
-        signingPrivateKey = newSigKp.privateKey
-        encryptionPrivateKey = newEncKp.privateKey
+        // Stage the new keys — they take effect only when commit() succeeds
+        pendingSigningKey = newSigKp.privateKey
+        pendingEncryptionKey = newEncKp.privateKey

        return proposal
    }
@@ -347,11 +350,14 @@ class MlsGroup private constructor(
        val addedMembers = mutableListOf<Pair<Int, MlsKeyPackage>>()
        for (pending in proposals) {
            val p = pending.proposal
-            // Track added members for Welcome generation (before apply changes leaf count)
            if (p is Proposal.Add) {
-                addedMembers.add(tree.leafCount to p.keyPackage)
+                // applyProposal calls tree.addLeaf() which returns the actual leaf index
+                // (may reuse a blank slot instead of appending)
+                val leafIndex = applyProposalAdd(p)
+                addedMembers.add(leafIndex to p.keyPackage)
+            } else {
+                applyProposal(p, pending.senderLeafIndex)
            }
-            applyProposal(p, pending.senderLeafIndex)
        }

        // Generate new path secrets on the updated tree
@@ -465,6 +471,12 @@ class MlsGroup private constructor(
                null
            }

+        // Promote staged keys from proposeSigningKeyRotation if present
+        pendingSigningKey?.let { signingPrivateKey = it }
+        pendingEncryptionKey?.let { encryptionPrivateKey = it }
+        pendingSigningKey = null
+        pendingEncryptionKey = null
+
        pendingProposals.clear()
        sentKeys.clear()

@@ -605,6 +617,13 @@ class MlsGroup private constructor(
        senderLeafIndex: Int,
        confirmationTag: ByteArray? = null,
    ) {
+        require(senderLeafIndex >= 0 && senderLeafIndex < tree.leafCount) {
+            "Invalid sender leaf index: $senderLeafIndex"
+        }
+        require(tree.getLeaf(senderLeafIndex) != null) {
+            "Sender leaf is blank at index $senderLeafIndex"
+        }
+
        val commit = Commit.decodeTls(TlsReader(commitBytes))

        // Apply proposals (resolve references from pending pool)
@@ -641,6 +660,11 @@ class MlsGroup private constructor(
            tree.setLeaf(senderLeafIndex, updatePath.leafNode)
            tree.applyUpdatePath(senderLeafIndex, updatePath.nodes)

+            // Verify parent hash chain (RFC 9420 Section 7.9.2)
+            require(verifyParentHash(senderLeafIndex, updatePath)) {
+                "Parent hash verification failed for UpdatePath"
+            }
+
            // Decrypt path secret from our copath node
            val directPath = BinaryTree.directPath(senderLeafIndex, tree.leafCount)
            val myPath = BinaryTree.directPath(myLeafIndex, tree.leafCount)
@@ -726,7 +750,7 @@ class MlsGroup private constructor(
        // Verify confirmation tag (RFC 9420 Section 6.1)
        if (confirmationTag != null) {
            val expectedTag = computeConfirmationTag(epochSecrets.confirmationKey, newConfirmedTranscriptHash)
-            require(confirmationTag.contentEquals(expectedTag)) {
+            require(constantTimeEquals(confirmationTag, expectedTag)) {
                "Confirmation tag verification failed"
            }
        }
@@ -846,7 +870,7 @@ class MlsGroup private constructor(
        for (pskProposal in pskProposals) {
            val pskValue =
                pskStore[pskProposal.pskId.toHexKey()]
-                    ?: ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) // Unknown PSK = zeros
+                    ?: throw IllegalStateException("PSK not found in store: ${pskProposal.pskId.toHexKey()}")
            pskSecret = MlsCryptoProvider.hkdfExtract(pskSecret, pskValue)
        }
        return pskSecret
@@ -978,7 +1002,37 @@ class MlsGroup private constructor(
                .MacInstance("HmacSHA256", epochSecrets.membershipKey)
        mac.update(authenticatedContentBytes)
        val expectedTag = mac.doFinal()
-        return expectedTag.contentEquals(membershipTag)
+        return constantTimeEquals(expectedTag, membershipTag)
+    }
+
+    /**
+     * Constant-time byte array comparison to prevent timing side-channels.
+     * Returns true only if both arrays have the same length and contents.
+     */
+    private fun constantTimeEquals(
+        a: ByteArray,
+        b: ByteArray,
+    ): Boolean {
+        if (a.size != b.size) return false
+        var result = 0
+        for (i in a.indices) {
+            result = result or (a[i].toInt() xor b[i].toInt())
+        }
+        return result == 0
+    }
+
+    /**
+     * Apply an Add proposal and return the assigned leaf index.
+     */
+    private fun applyProposalAdd(proposal: Proposal.Add): Int {
+        val lifetime = proposal.keyPackage.leafNode.lifetime
+        if (lifetime != null) {
+            val now = TimeUtils.now()
+            require(now >= lifetime.notBefore && now <= lifetime.notAfter) {
+                "KeyPackage lifetime expired or not yet valid"
+            }
+        }
+        return tree.addLeaf(proposal.keyPackage.leafNode)
    }

    private fun applyProposal(
@@ -987,15 +1041,7 @@ class MlsGroup private constructor(
    ) {
        when (proposal) {
            is Proposal.Add -> {
-                // Validate KeyPackage lifetime (RFC 9420 Section 10.1)
-                val lifetime = proposal.keyPackage.leafNode.lifetime
-                if (lifetime != null) {
-                    val now = TimeUtils.now()
-                    require(now >= lifetime.notBefore && now <= lifetime.notAfter) {
-                        "KeyPackage lifetime expired or not yet valid"
-                    }
-                }
-                tree.addLeaf(proposal.keyPackage.leafNode)
+                applyProposalAdd(proposal)
            }

            is Proposal.Remove -> {
@@ -1300,7 +1346,7 @@ class MlsGroup private constructor(

            // Find our leaf index by matching our signature key
            val mySignatureKey = Ed25519.publicFromPrivate(bundle.signaturePrivateKey)
-            var myLeafIndex = 0
+            var myLeafIndex = -1
            for (i in 0 until tree.leafCount) {
                val leaf = tree.getLeaf(i)
                if (leaf != null && leaf.signatureKey.contentEquals(mySignatureKey)) {
@@ -1308,13 +1354,15 @@ class MlsGroup private constructor(
                    break
                }
            }
+            require(myLeafIndex >= 0) { "Joiner's signature key not found in ratchet tree" }

            // Verify GroupInfo signature using signer's key from the tree
            val signerLeaf = tree.getLeaf(groupInfo.signer)
-            if (signerLeaf != null) {
-                require(groupInfo.verifySignature(signerLeaf.signatureKey)) {
-                    "Invalid GroupInfo signature"
-                }
+            requireNotNull(signerLeaf) {
+                "Signer leaf is null at index ${groupInfo.signer} — cannot verify GroupInfo signature"
+            }
+            require(groupInfo.verifySignature(signerLeaf.signatureKey)) {
+                "Invalid GroupInfo signature"
            }

            // Derive epoch secrets directly from memberSecret (RFC 9420 Section 8.3)
@@ -1351,6 +1399,15 @@ class MlsGroup private constructor(

            val secretTree = SecretTree(epochSecrets.encryptionSecret, tree.leafCount)

+            // Compute interim_transcript_hash from confirmed_transcript_hash + confirmation_tag
+            val confirmMac = MacInstance("HmacSHA256", epochSecrets.confirmationKey)
+            confirmMac.update(groupContext.confirmedTranscriptHash)
+            val confirmationTag = confirmMac.doFinal()
+            val interimInput = TlsWriter()
+            interimInput.putBytes(groupContext.confirmedTranscriptHash)
+            interimInput.putOpaqueVarInt(confirmationTag)
+            val interimTranscriptHash = MlsCryptoProvider.hash(interimInput.toByteArray())
+
            return MlsGroup(
                groupContext = groupContext,
                tree = tree,
@@ -1360,7 +1417,7 @@ class MlsGroup private constructor(
                initSecret = epochSecrets.initSecret,
                signingPrivateKey = bundle.signaturePrivateKey,
                encryptionPrivateKey = bundle.encryptionPrivateKey,
-                interimTranscriptHash = ByteArray(0),
+                interimTranscriptHash = interimTranscriptHash,
            )
        }

@@ -27,6 +27,8 @@ import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult
 import com.vitorpamplona.quartz.marmot.mls.messages.KeyPackageBundle
 import com.vitorpamplona.quartz.marmot.mls.schedule.SecretTree
 import com.vitorpamplona.quartz.nip01Core.core.HexKey
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock

 /**
 * High-level coordinator for MLS group lifecycle and state management.
@@ -86,6 +88,7 @@ import com.vitorpamplona.quartz.nip01Core.core.HexKey
 class MlsGroupManager(
    private val store: MlsGroupStateStore,
 ) {
+    private val mutex = Mutex()
    private val groups = mutableMapOf<HexKey, MlsGroup>()
    private val retainedEpochs = mutableMapOf<HexKey, MutableList<RetainedEpochSecrets>>()

@@ -93,28 +96,29 @@ class MlsGroupManager(
     * Restore all groups from persistent storage on startup.
     * Call this once during Account initialization.
     */
-    suspend fun restoreAll() {
-        val groupIds = store.listGroups()
-        for (nostrGroupId in groupIds) {
-            try {
-                val stateBytes = store.load(nostrGroupId) ?: continue
-                val state = MlsGroupState.decodeTls(stateBytes)
-                groups[nostrGroupId] = MlsGroup.restore(state)
+    suspend fun restoreAll() =
+        mutex.withLock {
+            val groupIds = store.listGroups()
+            for (nostrGroupId in groupIds) {
+                try {
+                    val stateBytes = store.load(nostrGroupId) ?: continue
+                    val state = MlsGroupState.decodeTls(stateBytes)
+                    groups[nostrGroupId] = MlsGroup.restore(state)

-                // Restore retained epochs
-                val retained = store.loadRetainedEpochs(nostrGroupId)
-                if (retained.isNotEmpty()) {
-                    retainedEpochs[nostrGroupId] =
-                        retained
-                            .map { RetainedEpochSecrets.decodeTls(TlsReader(it)) }
-                            .toMutableList()
+                    // Restore retained epochs
+                    val retained = store.loadRetainedEpochs(nostrGroupId)
+                    if (retained.isNotEmpty()) {
+                        retainedEpochs[nostrGroupId] =
+                            retained
+                                .map { RetainedEpochSecrets.decodeTls(TlsReader(it)) }
+                                .toMutableList()
+                    }
+                } catch (_: Exception) {
+                    // Corrupted state — remove it so it doesn't block future joins
+                    store.delete(nostrGroupId)
                }
-            } catch (_: Exception) {
-                // Corrupted state — remove it so it doesn't block future joins
-                store.delete(nostrGroupId)
            }
        }
-    }

    /**
     * Get an active group by its Nostr group ID.
@@ -145,12 +149,13 @@ class MlsGroupManager(
        nostrGroupId: HexKey,
        identity: ByteArray,
        signingKey: ByteArray? = null,
-    ): MlsGroup {
-        val group = MlsGroup.create(identity, signingKey)
-        groups[nostrGroupId] = group
-        persistGroup(nostrGroupId)
-        return group
-    }
+    ): MlsGroup =
+        mutex.withLock {
+            val group = MlsGroup.create(identity, signingKey)
+            groups[nostrGroupId] = group
+            persistGroup(nostrGroupId)
+            group
+        }

    // --- Joining ---

@@ -172,16 +177,17 @@ class MlsGroupManager(
        nostrGroupId: HexKey,
        welcomeBytes: ByteArray,
        bundle: KeyPackageBundle,
-    ): MlsGroup {
-        val group = MlsGroup.processWelcome(welcomeBytes, bundle)
-        groups[nostrGroupId] = group
+    ): MlsGroup =
+        mutex.withLock {
+            val group = MlsGroup.processWelcome(welcomeBytes, bundle)
+            groups[nostrGroupId] = group

-        // init_key is consumed — the bundle's initPrivateKey should not be
-        // reused. Caller must discard the bundle and rotate KeyPackages.
+            // init_key is consumed — the bundle's initPrivateKey should not be
+            // reused. Caller must discard the bundle and rotate KeyPackages.

-        persistGroup(nostrGroupId)
-        return group
-    }
+            persistGroup(nostrGroupId)
+            group
+        }

    /**
     * Join a group via external commit.
@@ -197,12 +203,13 @@ class MlsGroupManager(
        groupInfoBytes: ByteArray,
        identity: ByteArray,
        signingKey: ByteArray? = null,
-    ): Pair<MlsGroup, ByteArray> {
-        val (group, commitBytes) = MlsGroup.externalJoin(groupInfoBytes, identity, signingKey)
-        groups[nostrGroupId] = group
-        persistGroup(nostrGroupId)
-        return Pair(group, commitBytes)
-    }
+    ): Pair<MlsGroup, ByteArray> =
+        mutex.withLock {
+            val (group, commitBytes) = MlsGroup.externalJoin(groupInfoBytes, identity, signingKey)
+            groups[nostrGroupId] = group
+            persistGroup(nostrGroupId)
+            Pair(group, commitBytes)
+        }

    // --- Epoch Transitions ---

@@ -215,16 +222,17 @@ class MlsGroupManager(
     * @param nostrGroupId hex-encoded Nostr group ID
     * @return the [CommitResult] to publish
     */
-    suspend fun commit(nostrGroupId: HexKey): CommitResult {
-        val group = requireGroup(nostrGroupId)
+    suspend fun commit(nostrGroupId: HexKey): CommitResult =
+        mutex.withLock {
+            val group = requireGroup(nostrGroupId)

-        // Retain current epoch secrets before transition
-        retainEpochSecrets(nostrGroupId, group)
+            // Retain current epoch secrets before transition
+            retainEpochSecrets(nostrGroupId, group)

-        val result = group.commit()
-        persistGroup(nostrGroupId)
-        return result
-    }
+            val result = group.commit()
+            persistGroup(nostrGroupId)
+            result
+        }

    /**
     * Process a received Commit, advancing the epoch.
@@ -239,7 +247,7 @@ class MlsGroupManager(
        commitBytes: ByteArray,
        senderLeafIndex: Int,
        confirmationTag: ByteArray? = null,
-    ) {
+    ) = mutex.withLock {
        val group = requireGroup(nostrGroupId)

        // Retain current epoch secrets before transition
@@ -307,13 +315,14 @@ class MlsGroupManager(
    suspend fun addMember(
        nostrGroupId: HexKey,
        keyPackageBytes: ByteArray,
-    ): CommitResult {
-        val group = requireGroup(nostrGroupId)
-        retainEpochSecrets(nostrGroupId, group)
-        val result = group.addMember(keyPackageBytes)
-        persistGroup(nostrGroupId)
-        return result
-    }
+    ): CommitResult =
+        mutex.withLock {
+            val group = requireGroup(nostrGroupId)
+            retainEpochSecrets(nostrGroupId, group)
+            val result = group.addMember(keyPackageBytes)
+            persistGroup(nostrGroupId)
+            result
+        }

    /**
     * Remove a member and create a Commit.
@@ -321,13 +330,14 @@ class MlsGroupManager(
    suspend fun removeMember(
        nostrGroupId: HexKey,
        targetLeafIndex: Int,
-    ): CommitResult {
-        val group = requireGroup(nostrGroupId)
-        retainEpochSecrets(nostrGroupId, group)
-        val result = group.removeMember(targetLeafIndex)
-        persistGroup(nostrGroupId)
-        return result
-    }
+    ): CommitResult =
+        mutex.withLock {
+            val group = requireGroup(nostrGroupId)
+            retainEpochSecrets(nostrGroupId, group)
+            val result = group.removeMember(targetLeafIndex)
+            persistGroup(nostrGroupId)
+            result
+        }

    /**
     * Rotate the signing key within a group and commit.
@@ -338,27 +348,42 @@ class MlsGroupManager(
     * @param nostrGroupId hex-encoded Nostr group ID
     * @return the [CommitResult] to publish
     */
-    suspend fun rotateSigningKey(nostrGroupId: HexKey): CommitResult {
-        val group = requireGroup(nostrGroupId)
-        retainEpochSecrets(nostrGroupId, group)
-        group.proposeSigningKeyRotation()
-        val result = group.commit()
-        persistGroup(nostrGroupId)
-        return result
-    }
+    suspend fun rotateSigningKey(nostrGroupId: HexKey): CommitResult =
+        mutex.withLock {
+            val group = requireGroup(nostrGroupId)
+            retainEpochSecrets(nostrGroupId, group)
+            group.proposeSigningKeyRotation()
+            val result = group.commit()
+            persistGroup(nostrGroupId)
+            result
+        }

    /**
     * Leave a group (self-remove).
     * Returns the SelfRemove proposal bytes to publish, then removes
     * local state.
     */
-    suspend fun leaveGroup(nostrGroupId: HexKey): ByteArray {
-        val group = requireGroup(nostrGroupId)
-        val proposalBytes = group.selfRemove()
+    suspend fun leaveGroup(nostrGroupId: HexKey): ByteArray =
+        mutex.withLock {
+            val group = requireGroup(nostrGroupId)
+            val proposalBytes = group.selfRemove()
+            removeGroupStateUnlocked(nostrGroupId)
+            proposalBytes
+        }
+
+    /**
+     * Remove all local state for a group without sending any proposals.
+     * Used after the leave event has already been built.
+     */
+    suspend fun removeGroupState(nostrGroupId: HexKey) =
+        mutex.withLock {
+            removeGroupStateUnlocked(nostrGroupId)
+        }
+
+    private suspend fun removeGroupStateUnlocked(nostrGroupId: HexKey) {
        groups.remove(nostrGroupId)
        retainedEpochs.remove(nostrGroupId)
        store.delete(nostrGroupId)
-        return proposalBytes
    }

    // --- Key Export ---
@@ -109,6 +109,13 @@ class RatchetTree(
        for (i in 0 until _leafCount) {
            if (getLeaf(i) == null) {
                setLeaf(i, leafNode)
+                // Blank the direct path (RFC 9420 Section 7.7)
+                val directPath = BinaryTree.directPath(i, _leafCount)
+                for (nodeIdx in directPath) {
+                    if (nodeIdx < nodes.size) {
+                        nodes[nodeIdx] = null
+                    }
+                }
                return i
            }
        }
@@ -121,6 +128,13 @@ class RatchetTree(
            nodes.add(null)
        }
        setLeaf(newLeafIndex, leafNode)
+        // Blank the direct path for the new leaf
+        val directPath = BinaryTree.directPath(newLeafIndex, _leafCount)
+        for (nodeIdx in directPath) {
+            if (nodeIdx < nodes.size) {
+                nodes[nodeIdx] = null
+            }
+        }
        return newLeafIndex
    }

@@ -247,18 +261,18 @@ class RatchetTree(

        var currentSecret = leafSecret
        for (nodeIdx in directPath) {
-            // path_secret[n] = DeriveSecret(path_secret[n-1], "path")
-            val pathSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path")
-
-            // node_secret = DeriveSecret(path_secret, "node")
-            val nodeSecret = MlsCryptoProvider.deriveSecret(pathSecret, "node")
+            // RFC 9420 Section 7.4: path_secret[0] = leafSecret,
+            // node_secret[n] = DeriveSecret(path_secret[n], "node")
+            val nodeSecret = MlsCryptoProvider.deriveSecret(currentSecret, "node")

            // Derive HPKE key pair from node_secret
            val privateKey = MlsCryptoProvider.expandWithLabel(nodeSecret, "hpke", ByteArray(0), 32)
            val publicKey = X25519.publicFromPrivate(privateKey)

-            results.add(PathSecretAndKey(pathSecret, privateKey, publicKey))
-            currentSecret = pathSecret
+            results.add(PathSecretAndKey(currentSecret, privateKey, publicKey))
+
+            // path_secret[n+1] = DeriveSecret(path_secret[n], "path")
+            currentSecret = MlsCryptoProvider.deriveSecret(currentSecret, "path")
        }

        return results
@@ -344,9 +358,20 @@ class RatchetTree(

            val tree = RatchetTree()
            tree.nodes.addAll(nodesList)
-            // Leaf count is derived from the total serialized node count.
-            // nodeCount = 2 * leafCount - 1
-            tree._leafCount = (nodesList.size + 1) / 2
+            // Leaf count must account for tree trimming: the serialized tree
+            // may have trailing blank nodes removed. Count actual leaves by
+            // scanning for the highest occupied leaf position.
+            var maxLeafIndex = -1
+            for (i in nodesList.indices) {
+                if (BinaryTree.isLeaf(i) && nodesList[i] != null) {
+                    maxLeafIndex = BinaryTree.nodeToLeaf(i)
+                }
+            }
+            // Leaf count is at least maxLeafIndex + 1, but also must be at
+            // least (nodesList.size + 1) / 2 since parent nodes at high indices
+            // imply leaves beyond the serialized range.
+            val fromNodes = (nodesList.size + 1) / 2
+            tree._leafCount = maxOf(fromNodes, maxLeafIndex + 1)
            return tree
        }
    }
@@ -73,6 +73,11 @@ actual object X25519 {

        val secret = ka.generateSecret()

+        // Check for small-subgroup attack (RFC 9180 Section 4.1)
+        require(!secret.all { it == 0.toByte() }) {
+            "DH produced all-zero shared secret (possible small-subgroup attack)"
+        }
+
        // XDH returns big-endian, X25519 shared secret is 32 bytes
        // Pad or trim to KEY_LENGTH
        return if (secret.size == KEY_LENGTH) {
@@ -119,18 +124,20 @@ actual object X25519 {

    /**
     * Extract raw scalar bytes from JCA XECPrivateKey.
+     * Scalar is in little-endian byte order per JCA spec.
     */
    private fun extractPrivateKeyBytes(privKey: java.security.interfaces.XECPrivateKey): ByteArray {
        val scalar = privKey.scalar.orElseThrow { IllegalStateException("No scalar in private key") }
        return if (scalar.size == KEY_LENGTH) {
            scalar
        } else if (scalar.size < KEY_LENGTH) {
-            // Pad with leading zeros
+            // Pad with trailing zeros (little-endian: high-order bytes at end)
            val result = ByteArray(KEY_LENGTH)
-            scalar.copyInto(result, KEY_LENGTH - scalar.size)
+            scalar.copyInto(result, 0)
            result
        } else {
-            scalar.copyOfRange(scalar.size - KEY_LENGTH, scalar.size)
+            // Take only the first KEY_LENGTH bytes (little-endian low-order bytes)
+            scalar.copyOfRange(0, KEY_LENGTH)
        }
    }

@@ -153,12 +160,11 @@ actual object X25519 {
    private fun bigIntegerToBytes(bi: java.math.BigInteger): ByteArray {
        val beBytes = bi.toByteArray()
        val result = ByteArray(KEY_LENGTH)
-        // BigInteger is big-endian, possibly with leading zero byte
-        val start = if (beBytes.size > KEY_LENGTH) beBytes.size - KEY_LENGTH else 0
-        val length = minOf(beBytes.size, KEY_LENGTH)
-        // Reverse into little-endian result
-        for (i in 0 until length) {
-            result[i] = beBytes[beBytes.size - 1 - i + start]
+        // BigInteger is big-endian, possibly with a leading zero sign byte.
+        // Reverse into little-endian, taking at most KEY_LENGTH bytes from the low end.
+        val bytesToCopy = minOf(beBytes.size, KEY_LENGTH)
+        for (i in 0 until bytesToCopy) {
+            result[i] = beBytes[beBytes.size - 1 - i]
        }
        return result
    }