From 29d1610d1a76a15d12baf2ca4641e7fc6e99971b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Apr 2026 23:02:09 +0000 Subject: [PATCH] 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 --- .../amethyst/commons/marmot/MarmotManager.kt | 12 +- .../quartz/marmot/MarmotInboundProcessor.kt | 14 +- .../mip03GroupMessages/CommitOrdering.kt | 54 ++++-- .../quartz/marmot/mls/group/MlsGroup.kt | 105 ++++++++--- .../marmot/mls/group/MlsGroupManager.kt | 171 ++++++++++-------- .../quartz/marmot/mls/tree/RatchetTree.kt | 45 ++++- .../marmot/mls/crypto/X25519.jvmAndroid.kt | 24 ++- 7 files changed, 284 insertions(+), 141 deletions(-) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt index d9d8cfc35..3719d92f8 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt @@ -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 --- diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt index 7c4d68f4c..8049e1321 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt @@ -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 = commitTracker.pendingEpochs() + fun pendingCommitGroupEpochs(): Set = 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) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip03GroupMessages/CommitOrdering.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip03GroupMessages/CommitOrdering.kt index 614979a78..6fd12edcb 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip03GroupMessages/CommitOrdering.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip03GroupMessages/CommitOrdering.kt @@ -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>() + private val pendingByGroupEpoch = mutableMapOf>() /** - * 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 = pendingByEpoch[epoch] ?: emptyList() + fun pendingForEpoch( + groupId: String, + epoch: Long, + ): List = 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 = pendingByEpoch.keys.toSet() + fun pendingGroupEpochs(): Set = pendingByGroupEpoch.keys.toSet() /** * Clears all pending state. */ fun clear() { - pendingByEpoch.clear() + pendingByGroupEpoch.clear() } } } 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 d44b4a993..41b8f8998 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 @@ -105,6 +105,9 @@ class MlsGroup private constructor( private val pskStore: MutableMap = mutableMapOf(), private val pendingProposals: MutableList = mutableListOf(), private val sentKeys: MutableMap = 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>() 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, ) } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt index b7fede6f0..3ae6abb5b 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt @@ -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() private val retainedEpochs = mutableMapOf>() @@ -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 { - val (group, commitBytes) = MlsGroup.externalJoin(groupInfoBytes, identity, signingKey) - groups[nostrGroupId] = group - persistGroup(nostrGroupId) - return Pair(group, commitBytes) - } + ): Pair = + 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 --- diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt index e25272300..b3085b816 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/RatchetTree.kt @@ -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 } } diff --git a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.jvmAndroid.kt b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.jvmAndroid.kt index c784cf7ae..3d0265498 100644 --- a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.jvmAndroid.kt +++ b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.jvmAndroid.kt @@ -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 }