Address high-severity MLS/nostr review findings
Verify inbound nostr signatures (C1): NostrEvent.fromEvent now calls quartz Event.verify() (id-hash integrity + Schnorr) and drops the event on failure. fromEvent is the single inbound parse choke point, so forged events from a malicious/compromised relay can no longer reach the DB (profile overwrites, fake notes/reactions, etc.). Persist + retain PrivateMessage commits (H2): applyCommit's PRIVATE_MESSAGE branch captured no epoch secrets and never persisted the advance, so decrypt()'s inline commit was lost on reload and prior-epoch messages became undecryptable. Capture retainedSecrets before decrypt and pushRetainedEpoch + persistGroup on a successful advance, matching decryptMessageBytes/processCommit. Fix membership/admin reconciliation inversions (H3/M1/M2) in processGroupMembershipChanges: new-member filter now negates correctly so new joiners are persisted; the new-member admin flag uses == true instead of == null (previously marked everyone admin when adminPubkeys was absent, nobody when present); removedAdmins uses != true so admins actually dropped from the list are demoted instead of stripping current admins. Document retained-epoch impersonation (C2) and plaintext-at-rest (H1): these can't be fixed in-app (RetainedEpochSecrets carries no tree/leaf sig keys/groupContext to verify a past-epoch signature; encryption-at-rest needs a platform keystore). Add prominent SECURITY warnings/TODOs at tryDecryptWithRetainedEpoch, the ncryptsec* key-package fields, and the mlsGroupState persistence, with the required fix in each case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
07e1db950a
commit
12189dcd75
3 files changed
+80
-7
No files matched your search
@@ -10,6 +10,7 @@ import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEvent
|
|||||||
import com.vitorpamplona.quartz.nip01Core.core.Event
|
import com.vitorpamplona.quartz.nip01Core.core.Event
|
||||||
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
||||||
import com.vitorpamplona.quartz.nip01Core.core.Kind
|
import com.vitorpamplona.quartz.nip01Core.core.Kind
|
||||||
|
import com.vitorpamplona.quartz.nip01Core.crypto.verify
|
||||||
import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent
|
import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent
|
||||||
import com.vitorpamplona.quartz.nip01Core.tags.events.firstTaggedEvent
|
import com.vitorpamplona.quartz.nip01Core.tags.events.firstTaggedEvent
|
||||||
import com.vitorpamplona.quartz.nip01Core.tags.events.taggedEvents
|
import com.vitorpamplona.quartz.nip01Core.tags.events.taggedEvents
|
||||||
@@ -491,6 +492,14 @@ data class NostrEvent(
|
|||||||
companion object {
|
companion object {
|
||||||
|
|
||||||
fun fromEvent(event: Event, synchronizeNostrEventRequest: SynchronizeNostrEventRequest? = null): NostrEvent? = try {
|
fun fromEvent(event: Event, synchronizeNostrEventRequest: SynchronizeNostrEventRequest? = null): NostrEvent? = try {
|
||||||
|
// Relays are untrusted. Reject any event whose id doesn't match the
|
||||||
|
// canonical NIP-01 hash or whose Schnorr signature is invalid, so a
|
||||||
|
// malicious/compromised relay cannot inject forged events attributed
|
||||||
|
// to another pubkey (profile overwrites, fake notes/reactions, etc.).
|
||||||
|
// verify() == verifyId() && verifySignature().
|
||||||
|
if (!event.verify()) {
|
||||||
|
return null
|
||||||
|
}
|
||||||
// TODO: Check for unsupported kind
|
// TODO: Check for unsupported kind
|
||||||
event.firstTaggedEvent()
|
event.firstTaggedEvent()
|
||||||
|
|
||||||
|
|||||||
+14
-3
@@ -81,14 +81,25 @@ class DatabaseMarmotRepository(
|
|||||||
)
|
)
|
||||||
logger.d("keyPackageEventId: $id")
|
logger.d("keyPackageEventId: $id")
|
||||||
|
|
||||||
|
// ⚠️ SECURITY / KNOWN VULNERABILITY (H1: MLS private keys stored in
|
||||||
|
// plaintext at rest). Despite the `ncryptsec*` field names, the values
|
||||||
|
// below are RAW hex private keys (MLS init / encryption / signature).
|
||||||
|
// Any DB compromise — device backup, forensic extraction, another app
|
||||||
|
// on a rooted/jailbroken device — yields the group's decryption and
|
||||||
|
// signing keys, defeating E2E confidentiality and enabling
|
||||||
|
// impersonation. The same applies to `mlsGroupState` persisted in
|
||||||
|
// MarmotInboundManager.persistGroup (see the warning there).
|
||||||
|
// TODO(security): Encrypt these at rest with a platform-backed
|
||||||
|
// key-encryption-key (Android Keystore / iOS Keychain / a JVM secret
|
||||||
|
// store) — the intended ncryptsec scheme — before this ships.
|
||||||
database.marmotKeyPackageBundleDao().upsert(
|
database.marmotKeyPackageBundleDao().upsert(
|
||||||
MarmotKeyPackageBundle(
|
MarmotKeyPackageBundle(
|
||||||
id = id,
|
id = id,
|
||||||
publicKey = publicKey,
|
publicKey = publicKey,
|
||||||
tlsEncodedMarmotKeyPackage = bundle.keyPackage.toTlsBytes(),
|
tlsEncodedMarmotKeyPackage = bundle.keyPackage.toTlsBytes(),
|
||||||
ncryptsecInitPrivateKey = bundle.initPrivateKey.toHex(), // TODO: ncryptSec this using the private key
|
ncryptsecInitPrivateKey = bundle.initPrivateKey.toHex(), // TODO(security): encrypt at rest, do not store raw
|
||||||
ncryptsecEncryptionPrivateKey = bundle.encryptionPrivateKey.toHex(), // TODO: ncryptSec this using the private key
|
ncryptsecEncryptionPrivateKey = bundle.encryptionPrivateKey.toHex(), // TODO(security): encrypt at rest, do not store raw
|
||||||
ncryptsecSignaturePrivateKey = bundle.signaturePrivateKey.toHex() // TODO: ncryptSec this using the private key
|
ncryptsecSignaturePrivateKey = bundle.signaturePrivateKey.toHex() // TODO(security): encrypt at rest, do not store raw
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|||||||
+57
-4
@@ -76,10 +76,10 @@ object MarmotInboundManager {
|
|||||||
logger.d("excludedParticipants: $excludedParticipants")
|
logger.d("excludedParticipants: $excludedParticipants")
|
||||||
val existingParticipantPublicKeys = localChatRoom.localParticipants.map { it.participant.participantPublicKey }
|
val existingParticipantPublicKeys = localChatRoom.localParticipants.map { it.participant.participantPublicKey }
|
||||||
val newParticipants = commitedGroupMembers.filter { committedGroupMember ->
|
val newParticipants = commitedGroupMembers.filter { committedGroupMember ->
|
||||||
existingParticipantPublicKeys.contains(committedGroupMember.participantPublicKey)
|
existingParticipantPublicKeys.contains(committedGroupMember.participantPublicKey).not()
|
||||||
}.map { newParticipant ->
|
}.map { newParticipant ->
|
||||||
newParticipant.copy(
|
newParticipant.copy(
|
||||||
adminAt = if (adminPublicKeys?.contains(newParticipant.participantPublicKey) == null) {
|
adminAt = if (adminPublicKeys?.contains(newParticipant.participantPublicKey) == true) {
|
||||||
Clock.System.now()
|
Clock.System.now()
|
||||||
} else {
|
} else {
|
||||||
null
|
null
|
||||||
@@ -88,7 +88,7 @@ object MarmotInboundManager {
|
|||||||
}
|
}
|
||||||
logger.d("newParticipants: $newParticipants")
|
logger.d("newParticipants: $newParticipants")
|
||||||
val removedAdmins = localChatRoom.localParticipants.filter { localParticipant ->
|
val removedAdmins = localChatRoom.localParticipants.filter { localParticipant ->
|
||||||
localParticipant.participant.adminAt != null && adminPublicKeys?.contains(localParticipant.participant.participantPublicKey) == true
|
localParticipant.participant.adminAt != null && adminPublicKeys?.contains(localParticipant.participant.participantPublicKey) != true
|
||||||
}.map { localParticipant ->
|
}.map { localParticipant ->
|
||||||
localParticipant.participant.copy(
|
localParticipant.participant.copy(
|
||||||
adminAt = null
|
adminAt = null
|
||||||
@@ -389,9 +389,22 @@ object MarmotInboundManager {
|
|||||||
}
|
}
|
||||||
|
|
||||||
else -> {
|
else -> {
|
||||||
|
// `decrypt` applies the commit inline, advancing the
|
||||||
|
// epoch in memory. Capture the outgoing epoch's
|
||||||
|
// secrets first, then retain + persist on a
|
||||||
|
// successful advance — mirroring decryptMessageBytes
|
||||||
|
// and processCommit. Without this the advance is lost
|
||||||
|
// on reload and prior-epoch messages can no longer be
|
||||||
|
// decrypted.
|
||||||
|
val retainedBefore = mlsGroup.retainedSecrets()
|
||||||
|
val preEpoch = mlsGroup.epoch
|
||||||
val decrypted = mlsGroup.decrypt(mlsMessage.toTlsBytes())
|
val decrypted = mlsGroup.decrypt(mlsMessage.toTlsBytes())
|
||||||
if (decrypted.contentType == ContentType.COMMIT) {
|
if (decrypted.contentType == ContentType.COMMIT) {
|
||||||
GroupEventResult.CommitProcessed(groupId, mlsGroup.epoch ?: 0)
|
if (mlsGroup.epoch != preEpoch) {
|
||||||
|
pushRetainedEpoch(groupId, retainedBefore)
|
||||||
|
persistGroup(database, mlsGroup)
|
||||||
|
}
|
||||||
|
GroupEventResult.CommitProcessed(groupId, mlsGroup.epoch)
|
||||||
} else {
|
} else {
|
||||||
GroupEventResult.Error(
|
GroupEventResult.Error(
|
||||||
groupId,
|
groupId,
|
||||||
@@ -566,6 +579,14 @@ object MarmotInboundManager {
|
|||||||
if (localChatRoom == null) {
|
if (localChatRoom == null) {
|
||||||
throw MarmotMissingChatGroupException("chatRoom not found in database, skipping")
|
throw MarmotMissingChatGroupException("chatRoom not found in database, skipping")
|
||||||
} else {
|
} else {
|
||||||
|
// ⚠️ SECURITY / KNOWN VULNERABILITY (H1: MLS group state stored
|
||||||
|
// in plaintext at rest). `MlsGroupState` contains secret key
|
||||||
|
// material (signing key, encryption key, epoch secrets) and its
|
||||||
|
// own KDoc states it "MUST be stored in encrypted local
|
||||||
|
// storage", yet it is written here as plain hex. See the matching
|
||||||
|
// warning on the key-package bundle in DatabaseMarmotRepository.
|
||||||
|
// TODO(security): Encrypt `mlsGroupState` at rest with a
|
||||||
|
// platform-backed key-encryption-key before this ships.
|
||||||
database.chatRoomDao().upsert(
|
database.chatRoomDao().upsert(
|
||||||
localChatRoom.chatRoom.copy(
|
localChatRoom.chatRoom.copy(
|
||||||
mlsGroupState = encoded.toHex()
|
mlsGroupState = encoded.toHex()
|
||||||
@@ -610,6 +631,38 @@ object MarmotInboundManager {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* ⚠️ SECURITY / KNOWN VULNERABILITY (retained-epoch sender impersonation).
|
||||||
|
*
|
||||||
|
* This hand-rolled fallback decrypts a late APPLICATION message under a
|
||||||
|
* previous epoch's secrets but does NOT verify the sender's per-message
|
||||||
|
* `FramedContentTBS` signature — unlike the library's primary
|
||||||
|
* [MlsGroup.decrypt], which verifies it against the sender leaf's signature
|
||||||
|
* key (quartz MlsGroup.kt: `verifyWithLabel(senderLeaf.signatureKey,
|
||||||
|
* "FramedContentTBS", …)`).
|
||||||
|
*
|
||||||
|
* The application key/nonce here are derived from the epoch's *shared*
|
||||||
|
* `encryptionSecret` (via [SecretTree]), which every group member — and any
|
||||||
|
* removed member who kept a prior [RetainedEpochSecrets] — possesses. The
|
||||||
|
* only thing that normally binds a message to a specific member is that
|
||||||
|
* signature. Because it is skipped, a holder of a retained epoch secret can
|
||||||
|
* forge an APPLICATION message with an arbitrary `senderLeafIndex`, and the
|
||||||
|
* downstream MIP-03 check in [processPrivateMessage] passes (the attacker
|
||||||
|
* simply sets the inner event's pubkey to match that leaf's identity).
|
||||||
|
*
|
||||||
|
* This CANNOT be fixed here: [RetainedEpochSecrets] carries only symmetric
|
||||||
|
* material (epoch, senderDataSecret, encryptionSecret, leafCount,
|
||||||
|
* exporterSecret) — no ratchet tree, no leaf signature keys, and no
|
||||||
|
* past-epoch groupContext — so the `FramedContentTBS` for a past epoch
|
||||||
|
* cannot be reconstructed or verified in the app.
|
||||||
|
*
|
||||||
|
* TODO(security): Fix in the quartz library — snapshot the epoch's ratchet
|
||||||
|
* tree (or at least each leaf's signature key) and groupContext into
|
||||||
|
* [RetainedEpochSecrets], and expose a verified `decryptWithRetainedSecrets`
|
||||||
|
* that reuses the library's `buildApplicationFramedContentTbs` +
|
||||||
|
* `verifyWithLabel` path. Until then this path trusts sender identity for
|
||||||
|
* late prior-epoch messages.
|
||||||
|
*/
|
||||||
private fun tryDecryptWithRetainedEpoch(
|
private fun tryDecryptWithRetainedEpoch(
|
||||||
messageBytes: ByteArray,
|
messageBytes: ByteArray,
|
||||||
retained: RetainedEpochSecrets,
|
retained: RetainedEpochSecrets,
|
||||||
|
|||||||
Reference in new issue
Block a user