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 dc1dc9209..412133729 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt @@ -577,15 +577,29 @@ class MarmotInboundProcessor( } else -> { - groupManager.processCommit( - nostrGroupId = groupId, - commitBytes = pubMsg.content, - senderLeafIndex = pubMsg.sender.leafIndex, - confirmationTag = tag, - signature = pubMsg.signature, - ) + // RFC 9420 §6.2 — reject PublicMessage commits + // whose membership_tag doesn't match what the + // current epoch's membership_key would produce. + // Without this an outsider with the outer + // exporter secret could inject arbitrary commit + // bytes and advance the group past them. val group = groupManager.getGroup(groupId) - GroupEventResult.CommitProcessed(groupId, group?.epoch ?: 0) + if (group != null && !group.verifyPublicMessageCommitMembershipTag(pubMsg)) { + GroupEventResult.Error( + groupId, + "Invalid membership_tag on PublicMessage commit", + ) + } else { + groupManager.processCommit( + nostrGroupId = groupId, + commitBytes = pubMsg.content, + senderLeafIndex = pubMsg.sender.leafIndex, + confirmationTag = tag, + signature = pubMsg.signature, + ) + val post = groupManager.getGroup(groupId) + GroupEventResult.CommitProcessed(groupId, post?.epoch ?: 0) + } } } } 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 f772f680f..91b467d32 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 @@ -1042,6 +1042,47 @@ class MlsGroup private constructor( confirmationTag: ByteArray, signature: ByteArray = ByteArray(0), wireFormat: WireFormat = WireFormat.PUBLIC_MESSAGE, + ) { + // Snapshot all mutable state BEFORE applying any proposals or + // UpdatePath mutations. If any step throws (bad signature, parent + // hash mismatch, decrypt failure, confirmation-tag divergence), we + // restore the snapshot so callers observe the group in the same + // state as if processCommit had never run. Without this rollback + // a partial mutation leaves the tree one epoch ahead of + // `groupContext.epoch` and every subsequent decrypt fails with + // `Message epoch X doesn't match current epoch Y`. + val treeSnapshot = tree.snapshot() + val ctxSnapshot = groupContext + val secretsSnapshot = epochSecrets + val initSnapshot = initSecret + val secretTreeSnapshot = secretTree + val interimSnapshot = interimTranscriptHash + val pendingSnapshot = pendingProposals.toList() + val sentKeysSnapshot = sentKeys.toMap() + + try { + processCommitInner(commitBytes, senderLeafIndex, confirmationTag, signature, wireFormat) + } catch (t: Throwable) { + tree.restoreFrom(treeSnapshot) + groupContext = ctxSnapshot + epochSecrets = secretsSnapshot + initSecret = initSnapshot + secretTree = secretTreeSnapshot + interimTranscriptHash = interimSnapshot + pendingProposals.clear() + pendingProposals.addAll(pendingSnapshot) + sentKeys.clear() + sentKeys.putAll(sentKeysSnapshot) + throw t + } + } + + private fun processCommitInner( + commitBytes: ByteArray, + senderLeafIndex: Int, + confirmationTag: ByteArray, + signature: ByteArray, + wireFormat: WireFormat, ) { val commit = Commit.decodeTls(TlsReader(commitBytes)) @@ -1065,6 +1106,33 @@ class MlsGroup private constructor( } } + // Verify the FramedContentTBS signature (RFC 9420 §6.1) against + // the sender's pre-commit leaf signature_key. Any non-external + // commit MUST carry a non-empty signature from the sender's leaf + // — without this check anyone with the outer exporter key (in a + // compromised relay scenario) could forge commits. + if (!isExternalCommit) { + require(signature.isNotEmpty()) { + "FramedContentTBS signature missing on commit from leaf $senderLeafIndex" + } + val senderLeaf = + requireNotNull(tree.getLeaf(senderLeafIndex)) { + "Sender leaf is blank at index $senderLeafIndex" + } + val tbs = + buildCommitFramedContentTbs( + groupId = groupContext.groupId, + epoch = groupContext.epoch, + senderLeafIndex = senderLeafIndex, + commitBytes = commitBytes, + groupContextBytes = groupContext.toTlsBytes(), + wireFormat = wireFormat, + ) + require(MlsCryptoProvider.verifyWithLabel(senderLeaf.signatureKey, "FramedContentTBS", tbs, signature)) { + "Invalid FramedContentTBS signature on commit from leaf $senderLeafIndex" + } + } + // Apply proposals (resolve references from pending pool). // Matches the committer's order: apply non-Add proposals first, then Adds, // so leaves freed by Remove are available for Add reuse (RFC 9420 §12.4.2). @@ -1121,6 +1189,17 @@ class MlsGroup private constructor( require(verifyLeafNodeSignature(updatePath.leafNode, groupId, senderLeafIndex)) { "Invalid LeafNode signature in UpdatePath" } + // RFC 9420 §8.4: LeafNode lifetime bounds must be current. + // An expired leaf is a revoked key — accepting it here would + // let a peer replay old UpdatePath material past the window + // the signer authorized. + val lifetime = updatePath.leafNode.lifetime + if (lifetime != null) { + val now = TimeUtils.now() + require(now >= lifetime.notBefore && now <= lifetime.notAfter) { + "LeafNode lifetime expired or not yet valid in UpdatePath" + } + } // For external commits the sender's leaf is not yet in the tree. // Grow the tree with a blank slot so directPath / sibling-hash @@ -1253,17 +1332,17 @@ class MlsGroup private constructor( initSecret = epochSecrets.initSecret secretTree = SecretTree(epochSecrets.encryptionSecret, tree.leafCount) - // Verify confirmation tag (RFC 9420 Section 6.1). In real message - // flows the tag is carried on the wrapping PublicMessage and must be - // verified. Callers that process a raw commit payload and have - // verified the tag externally (or for test harnesses that work with - // `Commit.toTlsBytes()` directly) pass `ByteArray(0)`; in that mode - // we skip the comparison. Any non-empty tag is still checked. + // Verify confirmation tag (RFC 9420 Section 6.1). Every commit on + // the wire MUST carry a confirmation_tag that matches what the + // receiver derives from the post-commit confirmation_key — this + // is the last line of defense against a committer with the + // correct signing key but a forged tree state. val expectedTag = computeConfirmationTag(epochSecrets.confirmationKey, newConfirmedTranscriptHash) - if (confirmationTag.isNotEmpty()) { - require(constantTimeEquals(confirmationTag, expectedTag)) { - "Confirmation tag verification failed" - } + require(confirmationTag.isNotEmpty()) { + "Confirmation tag missing on commit from leaf $senderLeafIndex" + } + require(constantTimeEquals(confirmationTag, expectedTag)) { + "Confirmation tag verification failed" } // Update interim_transcript_hash for next epoch (reuse verified expectedTag) @@ -1578,6 +1657,33 @@ class MlsGroup private constructor( return constantTimeEquals(expectedTag, membershipTag) } + /** + * Verify RFC 9420 §6.2 membership_tag on an inbound PublicMessage Commit. + * The tag binds the whole `(TBS || FramedContentAuthData)` payload to + * the sender's epoch — if it's missing or wrong, the sender either + * isn't a member or the message was tampered with. Returns false for + * either case; callers should reject the commit before advancing state. + */ + fun verifyPublicMessageCommitMembershipTag(pubMsg: PublicMessage): Boolean { + val membershipTag = pubMsg.membershipTag ?: return false + if (membershipTag.isEmpty()) return false + val confirmationTag = pubMsg.confirmationTag ?: return false + val tbs = + buildCommitFramedContentTbs( + groupId = pubMsg.groupId, + epoch = pubMsg.epoch, + senderLeafIndex = pubMsg.sender.leafIndex, + commitBytes = pubMsg.content, + groupContextBytes = groupContext.toTlsBytes(), + wireFormat = WireFormat.PUBLIC_MESSAGE, + ) + val tbmWriter = TlsWriter() + tbmWriter.putBytes(tbs) + tbmWriter.putOpaqueVarInt(pubMsg.signature) + tbmWriter.putOpaqueVarInt(confirmationTag) + return verifyMembershipTag(tbmWriter.toByteArray(), membershipTag) + } + /** * Build PrivateContentAAD (RFC 9420 §6.3.2): * ``` @@ -1918,10 +2024,16 @@ class MlsGroup private constructor( private const val RATCHET_TREE_EXTENSION_TYPE = 0x0002 /** - * Build the FramedContentTBS bytes for a member-sender commit over - * PublicMessage wire format (RFC 9420 §6.1). The signature over this - * value is the `FramedContentAuthData.signature` that rides both in - * the on-the-wire PublicMessage AND in the ConfirmedTranscriptHashInput. + * Build the FramedContentTBS bytes for a member-sender commit + * (RFC 9420 §6.1). The signature over this value is the + * `FramedContentAuthData.signature` that rides both in the + * on-the-wire PublicMessage/PrivateMessage AND in the + * ConfirmedTranscriptHashInput. + * + * The wire_format argument MUST match the envelope actually used; + * mixing PUBLIC_MESSAGE here with a PRIVATE_MESSAGE envelope (or + * vice versa) produces a signature that receivers can't verify + * because they recompute the TBS with the real wire_format byte. */ internal fun buildCommitFramedContentTbs( groupId: ByteArray, @@ -1929,10 +2041,11 @@ class MlsGroup private constructor( senderLeafIndex: Int, commitBytes: ByteArray, groupContextBytes: ByteArray, + wireFormat: WireFormat = WireFormat.PUBLIC_MESSAGE, ): ByteArray { val writer = TlsWriter() writer.putUint16(MlsMessage.MLS_VERSION_10) - writer.putUint16(WireFormat.PUBLIC_MESSAGE.value) + writer.putUint16(wireFormat.value) writer.putOpaqueVarInt(groupId) writer.putUint64(epoch) encodeSender(writer, Sender(SenderType.MEMBER, senderLeafIndex)) @@ -2674,12 +2787,6 @@ class MlsGroup private constructor( return commit() } - /** - * Remove self from the group. - * - * Per MIP-01/MIP-03, admins must self-demote first; this helper rejects - * calls from a member currently listed in `admin_pubkeys`. - */ /** * Build a standalone SelfRemove proposal framed as a PublicMessage MLS * message (RFC 9420 §6.2 + draft-ietf-mls-extensions). 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 10646bd49..39f70a786 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 @@ -185,6 +185,27 @@ class RatchetTree( } } + /** + * Snapshot the mutable tree state so callers can roll back after a + * failed commit application. `TreeNode`, `LeafNode`, and `ParentNode` + * are all immutable data classes — copying the `nodes` list is enough + * to isolate future edits. + */ + fun snapshot(): Snapshot = Snapshot(nodes.toList(), _leafCount) + + /** Restore the mutable tree state produced by an earlier [snapshot]. */ + fun restoreFrom(snapshot: Snapshot) { + nodes.clear() + nodes.addAll(snapshot.nodes) + _leafCount = snapshot.leafCount + } + + /** Opaque capture of the ratchet tree's mutable state. */ + class Snapshot internal constructor( + internal val nodes: List, + internal val leafCount: Int, + ) + /** * Compute the tree hash for this ratchet tree (RFC 9420 Section 7.9). * Used in GroupContext to bind the group state to the tree.