From eee8b66b5322cb3353a9eabb23fbcf0d648beacb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Apr 2026 07:25:34 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20round=202=20MLS=20security=20fixes=20?= =?UTF-8?q?=E2=80=94=20resolve=20critical=20key=20schedule=20and=20validat?= =?UTF-8?q?ion=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: - Fix PSK/ExternalInit proposals by Reference dropped from key schedule: processCommit now collects ALL resolved proposals (inline + by-reference) into resolvedProposals list used for PSK and ExternalInit computation - Fix decrypt() missing blank-leaf membership check: validate sender leaf is non-null (occupied) before proceeding with decryption High fixes: - Fix MlsGroupManager.decrypt() now mutex-protected to prevent concurrent SecretTree ratchet corruption and potential nonce reuse Medium fixes: - Fix externalJoin: verify GroupInfo signature before trusting tree/keys - Fix parentHash verification: COMMIT leaf nodes must have non-empty parentHash (no longer silently skipped) - Fix proposal application order: Updates/Removes applied before Adds per RFC 9420 §12.4.2 (frees blank slots before reuse) - Add encryption key uniqueness check in RatchetTree.addLeaf() per §7.3 - Add LeafNode capabilities validation: verify version and ciphersuite support in applyProposalAdd per §12.1.1 - Remove redundant confirmation tag recomputation in processCommit https://claude.ai/code/session_017SjKXS4Vpu4xRg9zHTgpmC --- .../quartz/marmot/mls/group/MlsGroup.kt | 85 +++++++++++++------ .../marmot/mls/group/MlsGroupManager.kt | 29 ++++--- .../quartz/marmot/mls/tree/RatchetTree.kt | 17 ++++ 3 files changed, 90 insertions(+), 41 deletions(-) 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 f62a371c6..05d2bc591 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 @@ -356,20 +356,23 @@ class MlsGroup private constructor( // Check if we need an UpdatePath (required unless only SelfRemove) val needsPath = proposals.any { it.proposal !is Proposal.SelfRemove } - // Apply proposals to tree FIRST (RFC 9420 Section 12.4.1) - // The UpdatePath is computed after proposals are applied. + // Apply proposals to tree FIRST (RFC 9420 Section 12.4.2) + // Order: Updates/Removes first, then Adds (so blank slots are freed before reuse) val addedMembers = mutableListOf>() + val addProposals = mutableListOf() for (pending in proposals) { - val p = pending.proposal - if (p is Proposal.Add) { - // 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) + if (pending.proposal is Proposal.Add) { + addProposals.add(pending) } else { - applyProposal(p, pending.senderLeafIndex) + applyProposal(pending.proposal, pending.senderLeafIndex) } } + // Apply Adds after Removes/Updates + for (pending in addProposals) { + val p = pending.proposal as Proposal.Add + val leafIndex = applyProposalAdd(p) + addedMembers.add(leafIndex to p.keyPackage) + } // Generate new path secrets on the updated tree val leafSecret = MlsCryptoProvider.randomBytes(MlsCryptoProvider.HASH_OUTPUT_LENGTH) @@ -623,10 +626,13 @@ class MlsGroup private constructor( val generation = senderReader.readUint32().toInt() val reuseGuard = senderReader.readBytes(REUSE_GUARD_LENGTH) - // Validate sender leaf index (M12) + // Validate sender leaf index and membership (RFC 9420 §6.3.1) require(senderLeafIndex in 0 until tree.leafCount) { "Sender leaf index $senderLeafIndex out of range [0, ${tree.leafCount})" } + require(tree.getLeaf(senderLeafIndex) != null) { + "Sender leaf is blank at index $senderLeafIndex (not a group member)" + } // Get the key/nonce for this sender+generation // If we sent this message ourselves, use the cached key to avoid ratchet conflict @@ -692,10 +698,13 @@ class MlsGroup private constructor( } // Apply proposals (resolve references from pending pool) + // Collect all resolved proposals for key schedule computation + val resolvedProposals = mutableListOf() for (proposalOrRef in commit.proposals) { when (proposalOrRef) { is ProposalOrRef.Inline -> { applyProposal(proposalOrRef.proposal, senderLeafIndex) + resolvedProposals.add(proposalOrRef.proposal) } is ProposalOrRef.Reference -> { @@ -711,6 +720,7 @@ class MlsGroup private constructor( "Commit references unknown proposal (ref not found in pending proposals)" } applyProposal(resolved.proposal, resolved.senderLeafIndex) + resolvedProposals.add(resolved.proposal) } } } @@ -794,19 +804,12 @@ class MlsGroup private constructor( confirmedTranscriptHash = newConfirmedTranscriptHash, ) - // Compute PSK secret from any PSK proposals in the commit - val commitPskProposals = - commit.proposals.mapNotNull { - when (it) { - is ProposalOrRef.Inline -> it.proposal - is ProposalOrRef.Reference -> null - } - } - val pskSecret = computePskSecret(commitPskProposals) + // Compute PSK secret from all resolved proposals (both inline and by-reference) + val pskSecret = computePskSecret(resolvedProposals) // Check for ExternalInit proposal — overrides init_secret (RFC 9420 Section 8.3) val externalInitProposal = - commitPskProposals.filterIsInstance().firstOrNull() + resolvedProposals.filterIsInstance().firstOrNull() val effectiveInitSecret = if (externalInitProposal != null) { deriveExternalInitSecret(externalInitProposal.kemOutput) @@ -825,11 +828,10 @@ class MlsGroup private constructor( "Confirmation tag verification failed" } - // Update interim_transcript_hash for next epoch - val computedTag = computeConfirmationTag(epochSecrets.confirmationKey, newConfirmedTranscriptHash) + // Update interim_transcript_hash for next epoch (reuse verified expectedTag) val interimInput = TlsWriter() interimInput.putBytes(newConfirmedTranscriptHash) - interimInput.putOpaqueVarInt(computedTag) + interimInput.putOpaqueVarInt(expectedTag) interimTranscriptHash = MlsCryptoProvider.hash(interimInput.toByteArray()) pendingProposals.clear() @@ -996,8 +998,15 @@ class MlsGroup private constructor( val directPath = BinaryTree.directPath(senderLeafIndex, tree.leafCount) if (directPath.isEmpty()) return true - val leafParentHash = updatePath.leafNode.parentHash ?: return true - if (leafParentHash.isEmpty()) return true + // COMMIT leaf nodes MUST have a non-empty parentHash (RFC 9420 §7.9.2) + val leafParentHash = updatePath.leafNode.parentHash + if (updatePath.leafNode.leafNodeSource == LeafNodeSource.COMMIT) { + requireNotNull(leafParentHash) { "Commit LeafNode missing parentHash" } + require(leafParentHash.isNotEmpty()) { "Commit LeafNode has empty parentHash" } + } else { + // Non-commit leaf nodes may not have parentHash + if (leafParentHash == null || leafParentHash.isEmpty()) return true + } // Walk up the direct path, verifying each parent_hash link var expectedParentHash = leafParentHash @@ -1129,14 +1138,27 @@ class MlsGroup private constructor( * Apply an Add proposal and return the assigned leaf index. */ private fun applyProposalAdd(proposal: Proposal.Add): Int { - val lifetime = proposal.keyPackage.leafNode.lifetime + val leafNode = proposal.keyPackage.leafNode + + // Validate lifetime + val lifetime = 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) + + // Validate capabilities (RFC 9420 §12.1.1) + val caps = leafNode.capabilities + require(1 in caps.versions) { + "KeyPackage does not support MLS protocol version 1" + } + require(1 in caps.ciphersuites) { + "KeyPackage does not support ciphersuite 0x0001" + } + + return tree.addLeaf(leafNode) } private fun applyProposal( @@ -1588,6 +1610,15 @@ class MlsGroup private constructor( ?: throw IllegalArgumentException("GroupInfo missing ratchet_tree extension") val tree = RatchetTree.decodeTls(TlsReader(ratchetTreeExt.extensionData)) + // Verify GroupInfo signature (RFC 9420 Section 12.4.3.1) + val signerLeaf = tree.getLeaf(groupInfo.signer) + requireNotNull(signerLeaf) { + "Signer leaf is null at index ${groupInfo.signer} — cannot verify GroupInfo signature" + } + require(groupInfo.verifySignature(signerLeaf.signatureKey)) { + "Invalid GroupInfo signature in externalJoin" + } + // Extract external_pub from extensions val externalPubExt = groupInfo.extensions.find { it.extensionType == EXTERNAL_PUB_EXTENSION_TYPE } 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 0bd91a870..952e633ed 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 @@ -292,24 +292,25 @@ class MlsGroupManager( suspend fun decrypt( nostrGroupId: HexKey, messageBytes: ByteArray, - ): DecryptedMessage { - val group = requireGroup(nostrGroupId) + ): DecryptedMessage = + mutex.withLock { + val group = requireGroup(nostrGroupId) - // Try current epoch - val current = group.decryptOrNull(messageBytes) - if (current != null) return current + // Try current epoch + val current = group.decryptOrNull(messageBytes) + if (current != null) return@withLock current - // Try retained epochs - val retained = retainedEpochs[nostrGroupId] ?: emptyList() - for (epochSecrets in retained) { - val result = tryDecryptWithRetainedEpoch(messageBytes, epochSecrets) - if (result != null) return result + // Try retained epochs + val retained = retainedEpochs[nostrGroupId] ?: emptyList() + for (epochSecrets in retained) { + val result = tryDecryptWithRetainedEpoch(messageBytes, epochSecrets) + if (result != null) return@withLock result + } + + // No epoch could decrypt — rethrow from current epoch for diagnostics + group.decrypt(messageBytes) } - // No epoch could decrypt — rethrow from current epoch for diagnostics - return group.decrypt(messageBytes) - } - /** * Decrypt with null return on failure. */ 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 b3085b816..729186470 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 @@ -100,11 +100,28 @@ class RatchetTree( return (node as? TreeNode.Leaf)?.leafNode } + /** + * Check that the given encryption key is not already used by another leaf (RFC 9420 §7.3). + */ + private fun requireUniqueEncryptionKey( + leafNode: LeafNode, + excludeLeafIndex: Int = -1, + ) { + for (i in 0 until _leafCount) { + if (i == excludeLeafIndex) continue + val existing = getLeaf(i) ?: continue + require(!existing.encryptionKey.contentEquals(leafNode.encryptionKey)) { + "Duplicate encryption key: leaf $i already uses this key" + } + } + } + /** * Add a new leaf to the tree. Returns the leaf index. * First tries to reuse a blank leaf slot, otherwise appends. */ fun addLeaf(leafNode: LeafNode): Int { + requireUniqueEncryptionKey(leafNode) // Find first blank leaf for (i in 0 until _leafCount) { if (getLeaf(i) == null) {