From 13c1d2048286bbabf5e7bc6b015aafee98f04d55 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 23 May 2025 13:43:26 +0100 Subject: [PATCH] fix(crypto): Check the sender of an event matches owner of session Having decrypted an event with a given megolm session, we need to check that the owner of that session actually matches the sender of an event, otherwise there is a danger of the sender being spoofed to make it look like it was sent by another user. Security-Impact: High CVE: CVE-2025-48937 GitHub-Advisory: GHSA-x958-rvg6-956w --- crates/matrix-sdk-crypto/src/machine/mod.rs | 26 +++++++- .../tests/decryption_verification_state.rs | 63 +++++++++++++++++++ .../src/olm/group_sessions/sender_data.rs | 20 ++++++ 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index bac19190c..613a2c4ab 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -1540,8 +1540,30 @@ impl OlmMachine { ) -> MegolmResult<(VerificationState, Option)> { let sender_data = self.get_or_update_sender_data(session, sender).await?; - let (verification_state, device_id) = - sender_data_to_verification_state(sender_data, session.has_been_imported()); + // If the user ID in the sender data doesn't match that in the event envelope, + // this event is not from who it appears to be from. + // + // If `sender_data.user_id()` returns `None`, that means we don't have any + // information about the owner of the session (i.e. we have + // `SenderData::UnknownDevice`); in that case we fall through to the + // logic in `sender_data_to_verification_state` which will pick an appropriate + // `DeviceLinkProblem` for `VerificationLevel::None`. + let (verification_state, device_id) = match sender_data.user_id() { + Some(i) if i != sender => { + // For backwards compatibility, we treat this the same as "Unknown device". + // TODO: use a dedicated VerificationLevel here. + ( + VerificationState::Unverified(VerificationLevel::None( + DeviceLinkProblem::MissingDevice, + )), + None, + ) + } + + Some(_) | None => { + sender_data_to_verification_state(sender_data, session.has_been_imported()) + } + }; Ok((verification_state, device_id)) } diff --git a/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs b/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs index ca617988d..e37e48112 100644 --- a/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs +++ b/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs @@ -311,6 +311,69 @@ pub async fn mark_alice_identity_as_verified_test_helper(alice: &OlmMachine, bob .is_verified()); } +/// Test that the verification state is set correctly when the sender of an +/// event does not match the owner of the device that sent us the session. +/// +/// In this test, Bob receives an event from Alice, but the HS admin has +/// rewritten the `sender` of the event to look like another user. +#[async_test] +async fn test_verification_states_spoofed_sender() { + let (alice, bob) = get_machine_pair_with_setup_sessions_test_helper( + tests::alice_id(), + tests::user_id(), + false, + ) + .await; + + let room_id = room_id!("!test:example.org"); + let decryption_settings = + DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted }; + + // Alice sends a message to Bob. + let (event, _) = encrypt_message(&alice, room_id, &bob, "Secret message").await; + bob.decrypt_room_event(&event, room_id, &decryption_settings) + .await + .expect("Bob could not decrypt event"); + let event_encryption_info = bob.get_room_event_encryption_info(&event, room_id).await.unwrap(); + assert_matches!( + event_encryption_info.verification_state, + VerificationState::Unverified(VerificationLevel::UnsignedDevice) + ); + + // Alice now sends a second message to Bob, using the same room key, but the HS + // admin rewrites the 'sender' to Charlie. + let encrypted_content = alice + .encrypt_room_event( + room_id, + AnyMessageLikeEventContent::RoomMessage(RoomMessageEventContent::text_plain( + "spoofed message", + )), + ) + .await + .unwrap(); + let event = json!({ + "event_id": "$xxxxy:example.org", + "origin_server_ts": MilliSecondsSinceUnixEpoch::now(), + "sender": "@charlie:example.org", // Note! spoofed sender + "type": "m.room.encrypted", + "content": encrypted_content, + }); + let event = json_convert(&event).unwrap(); + + bob.decrypt_room_event(&event, room_id, &decryption_settings) + .await + .expect("Bob could not decrypt spoofed event"); + + // The verification_state of the event should be `MissingDevice` (since it + // manifests as a message from Charlie which does not correspond to one of + // Charlie's devices). + let event_encryption_info = bob.get_room_event_encryption_info(&event, room_id).await.unwrap(); + assert_matches!( + event_encryption_info.verification_state, + VerificationState::Unverified(VerificationLevel::None(DeviceLinkProblem::MissingDevice)) + ); +} + #[async_test] async fn test_verification_states_multiple_device() { let (bob, _) = get_prepared_machine_test_helper(tests::user_id(), false).await; diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs index 9ac3b4a54..1e7538244 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs @@ -252,6 +252,26 @@ impl SenderData { Self::SenderVerified { .. } => SenderDataType::SenderVerified, } } + + /// Return our best guess of the owner of the associated megolm session. + /// + /// For `SenderData::UnknownDevice`, we don't record any information about + /// the owner of the sender, so returns `None`. + pub(crate) fn user_id(&self) -> Option { + match &self { + SenderData::UnknownDevice { .. } => None, + SenderData::DeviceInfo { device_keys, .. } => Some(device_keys.user_id.clone()), + SenderData::VerificationViolation(known_sender_data) => { + Some(known_sender_data.user_id.clone()) + } + SenderData::SenderUnverified(known_sender_data) => { + Some(known_sender_data.user_id.clone()) + } + SenderData::SenderVerified(known_sender_data) => { + Some(known_sender_data.user_id.clone()) + } + } + } } /// Used when deserialising and the sender_data property is missing.