From c88923a3d1d6235a84e5d423ae56be80257a3991 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 00:30:57 +0000 Subject: [PATCH] fix(mls): decrypt retained-epoch messages correctly (Test 12 offline catchup) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `tryDecryptWithRetainedEpoch` fallback in `MlsGroupManager.decrypt` exists precisely to cover the interop harness's Test 12 scenario — application messages encrypted under epoch N arriving at a receiver that has already processed a commit advancing to N+1. The primary path throws "Message epoch X doesn't match current epoch Y", then the manager iterates the retained epoch window and tries each. Two bugs kept the fallback from ever succeeding: 1. sender-data ciphertext sample length. The primary decrypt path uses `MlsCryptoProvider.HASH_OUTPUT_LENGTH` (32 bytes, = KDF.Nh for HKDF-SHA256, per RFC 9420 §6.3.2); the retained path was using `AEAD_KEY_LENGTH` (16). The comment on the primary path — "Using AEAD.Nk here made sender-data decryption fail against every spec-compliant sender" — was the same fix just never propagated to this branch. sender-data AEAD always failed, the try/catch swallowed it, and the fallback returned null. 2. content plaintext was returned raw. AEAD output is a PrivateMessageContent struct (§6.3.1): `applicationData || signature || padding`. The primary decrypt path parses this with `pmcReader.readOpaqueVarInt()` to extract applicationData; the retained path returned the whole struct. Callers saw a length-prefixed blob with signature + zero padding glued on — obvious in a raw diff, invisible in a pass/fail test that only checked "decrypted is not null". Fix both in `tryDecryptWithRetainedEpoch`. Existing quartz↔quartz tests still pass because they don't exercise out-of-order decrypt — this branch was effectively dead code before. Add testDecryptRetainedEpoch_ApplicationMessageAfterCommit as a regression: Bob encrypts at epoch N, Alice advances to N+1 via add-member, Bob's epoch-N message then arrives at Alice — must round- trip through the retained-epoch fallback and match the original plaintext exactly. --- .../marmot/mls/group/MlsGroupManager.kt | 26 ++++++- .../quartz/marmot/MarmotPipelineTest.kt | 78 +++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) 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 ca5f592c5e..cc7ef8e2f9 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 @@ -660,8 +660,18 @@ class MlsGroupManager( if (privMsg.epoch != retained.epoch) return null // Derive sender data key/nonce using ciphertext sample (RFC 9420 §6.3.1) + // RFC 9420 §6.3.2: ciphertext_sample is the first KDF.Nh bytes + // (32 for HKDF-SHA256), not AEAD.Nk (16). Using AEAD.Nk here made + // sender-data decryption silently fail for every retained-epoch + // message and turned the fallback path into a no-op — the + // symptom was interop Test 12 (offline catch-up): kind:9 + // messages encrypted under epoch N arriving after a 1→N+1 + // commit got rejected with "Message epoch X doesn't match + // current epoch Y" instead of being pulled through this path. + // Same fix already applied to MlsGroup.decrypt (line ~878); + // this branch was missed when that one was patched. val ciphertextSample = - privMsg.ciphertext.copyOfRange(0, minOf(privMsg.ciphertext.size, MlsCryptoProvider.AEAD_KEY_LENGTH)) + privMsg.ciphertext.copyOfRange(0, minOf(privMsg.ciphertext.size, MlsCryptoProvider.HASH_OUTPUT_LENGTH)) val senderDataKey = MlsCryptoProvider.expandWithLabel( retained.senderDataSecret, @@ -710,13 +720,23 @@ class MlsGroupManager( contentAad.putUint8(privMsg.contentType.value) contentAad.putOpaqueVarInt(privMsg.authenticatedData) - val plaintext = + val pmcBytes = MlsCryptoProvider.aeadDecrypt(kng.key, guardedNonce, contentAad.toByteArray(), privMsg.ciphertext) + // AEAD plaintext is a PrivateMessageContent struct (RFC 9420 + // §6.3.1): `applicationData || signature || padding`. The + // main decrypt path parses this and returns the inner + // applicationData; the retained-epoch branch was returning the + // raw struct, which made callers see a length-prefixed blob + // with signature + zero-padding glued onto the end. Extract the + // applicationData the same way. + val pmcReader = TlsReader(pmcBytes) + val applicationData = pmcReader.readOpaqueVarInt() + DecryptedMessage( senderLeafIndex = senderLeafIndex, contentType = privMsg.contentType, - content = plaintext, + content = applicationData, epoch = privMsg.epoch, ) } catch (_: Exception) { 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 ebfa6775d5..9727705f29 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt @@ -158,6 +158,84 @@ class MarmotPipelineTest { } } + /** + * Regression: the interop harness's Test 12 (offline catch-up) drove + * kind:9 application messages encrypted under epoch N into an Amethyst + * receiver that had already processed a 1→N+1 commit (e.g. add-member). + * Those epoch-N messages ought to decrypt through the retained-epoch + * fallback in [MlsGroupManager.decrypt], but the fallback was silently + * returning null because `tryDecryptWithRetainedEpoch` derived the + * sender-data sample using `AEAD_KEY_LENGTH` (16) instead of + * `HASH_OUTPUT_LENGTH` (32) — the same bug that was fixed on the + * main decrypt path earlier but never propagated to the retained + * branch. This test reproduces the flow and asserts the retained + * path actually decrypts. + */ + @Test + fun testDecryptRetainedEpoch_ApplicationMessageAfterCommit() { + runBlocking { + val aliceMgr = createGroupManager() + val bobMgr = createGroupManager() + + // 32-byte identities so they fit the MarmotGroupData 32-byte + // admin-pubkey slots. + val aliceId = ByteArray(32) { 0xA1.toByte() } + val bobId = ByteArray(32) { 0xB2.toByte() } + val carolId = ByteArray(32) { 0xC3.toByte() } + + aliceMgr.createGroup(groupId, aliceId) + // Install the MarmotGroupData extension so Welcome messages carry + // the NostrGroupData (MlsGroupManager.processWelcome requires it). + aliceMgr.updateGroupExtensions( + nostrGroupId = groupId, + extensions = + listOf( + com.vitorpamplona.quartz.marmot.mip01Groups + .MarmotGroupData( + nostrGroupId = groupId, + adminPubkeys = listOf(aliceId.toHexKey()), + ).toExtension(), + ), + ) + + // Bob joins at epoch 2 (epoch 0 = create, epoch 1 = GCE above, + // epoch 2 = add Bob). + val aliceGroup = aliceMgr.getGroup(groupId)!! + val bobBundle = aliceGroup.createKeyPackage(bobId, ByteArray(0)) + val addBob = aliceMgr.addMember(groupId, bobBundle.keyPackage.toTlsBytes()) + bobMgr.processWelcome(addBob.welcomeBytes!!, bobBundle) + val bobJoinEpoch = aliceMgr.getGroup(groupId)!!.epoch + assertEquals(bobJoinEpoch, bobMgr.getGroup(groupId)!!.epoch, "bob must be at alice's epoch after Welcome") + + // Bob encrypts a message at his current epoch — this would be + // held by a lossy relay and only reach Alice after she's + // already processed the next commit. + val bobStaleMsg = bobMgr.encrypt(groupId, "bob's offline epoch-N message".encodeToByteArray()) + + // Alice adds Carol, advancing to epoch N+1. Bob's earlier + // message is now out-of-order relative to Alice's tree state. + val carolBundle = aliceGroup.createKeyPackage(carolId, ByteArray(0)) + aliceMgr.addMember(groupId, carolBundle.keyPackage.toTlsBytes()) + assertEquals( + bobJoinEpoch + 1, + aliceMgr.getGroup(groupId)!!.epoch, + "alice must have advanced one epoch past bob's encrypt time", + ) + + // Bob's stale message now arrives at Alice. The primary decrypt + // path throws "Message epoch X doesn't match current epoch X+1"; + // the retained-epoch fallback inside MlsGroupManager.decrypt + // must pick it up. + val decrypted = aliceMgr.decrypt(groupId, bobStaleMsg) + assertEquals( + "bob's offline epoch-N message", + decrypted.content.decodeToString(), + "Alice must decrypt Bob's pre-commit message via retained-epoch fallback", + ) + assertEquals(bobJoinEpoch, decrypted.epoch, "message was encrypted at bob's join epoch") + } + } + @Test fun testInboundRejectsNonMemberGroup() { runBlocking {