Skip to content

Commit 13c1d20

Browse files
richvdhpoljar
authored andcommitted
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
1 parent 7f3e144 commit 13c1d20

File tree

3 files changed

+107
-2
lines changed

3 files changed

+107
-2
lines changed

crates/matrix-sdk-crypto/src/machine/mod.rs

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1540,8 +1540,30 @@ impl OlmMachine {
15401540
) -> MegolmResult<(VerificationState, Option<OwnedDeviceId>)> {
15411541
let sender_data = self.get_or_update_sender_data(session, sender).await?;
15421542

1543-
let (verification_state, device_id) =
1544-
sender_data_to_verification_state(sender_data, session.has_been_imported());
1543+
// If the user ID in the sender data doesn't match that in the event envelope,
1544+
// this event is not from who it appears to be from.
1545+
//
1546+
// If `sender_data.user_id()` returns `None`, that means we don't have any
1547+
// information about the owner of the session (i.e. we have
1548+
// `SenderData::UnknownDevice`); in that case we fall through to the
1549+
// logic in `sender_data_to_verification_state` which will pick an appropriate
1550+
// `DeviceLinkProblem` for `VerificationLevel::None`.
1551+
let (verification_state, device_id) = match sender_data.user_id() {
1552+
Some(i) if i != sender => {
1553+
// For backwards compatibility, we treat this the same as "Unknown device".
1554+
// TODO: use a dedicated VerificationLevel here.
1555+
(
1556+
VerificationState::Unverified(VerificationLevel::None(
1557+
DeviceLinkProblem::MissingDevice,
1558+
)),
1559+
None,
1560+
)
1561+
}
1562+
1563+
Some(_) | None => {
1564+
sender_data_to_verification_state(sender_data, session.has_been_imported())
1565+
}
1566+
};
15451567

15461568
Ok((verification_state, device_id))
15471569
}

crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,69 @@ pub async fn mark_alice_identity_as_verified_test_helper(alice: &OlmMachine, bob
311311
.is_verified());
312312
}
313313

314+
/// Test that the verification state is set correctly when the sender of an
315+
/// event does not match the owner of the device that sent us the session.
316+
///
317+
/// In this test, Bob receives an event from Alice, but the HS admin has
318+
/// rewritten the `sender` of the event to look like another user.
319+
#[async_test]
320+
async fn test_verification_states_spoofed_sender() {
321+
let (alice, bob) = get_machine_pair_with_setup_sessions_test_helper(
322+
tests::alice_id(),
323+
tests::user_id(),
324+
false,
325+
)
326+
.await;
327+
328+
let room_id = room_id!("!test:example.org");
329+
let decryption_settings =
330+
DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted };
331+
332+
// Alice sends a message to Bob.
333+
let (event, _) = encrypt_message(&alice, room_id, &bob, "Secret message").await;
334+
bob.decrypt_room_event(&event, room_id, &decryption_settings)
335+
.await
336+
.expect("Bob could not decrypt event");
337+
let event_encryption_info = bob.get_room_event_encryption_info(&event, room_id).await.unwrap();
338+
assert_matches!(
339+
event_encryption_info.verification_state,
340+
VerificationState::Unverified(VerificationLevel::UnsignedDevice)
341+
);
342+
343+
// Alice now sends a second message to Bob, using the same room key, but the HS
344+
// admin rewrites the 'sender' to Charlie.
345+
let encrypted_content = alice
346+
.encrypt_room_event(
347+
room_id,
348+
AnyMessageLikeEventContent::RoomMessage(RoomMessageEventContent::text_plain(
349+
"spoofed message",
350+
)),
351+
)
352+
.await
353+
.unwrap();
354+
let event = json!({
355+
"event_id": "$xxxxy:example.org",
356+
"origin_server_ts": MilliSecondsSinceUnixEpoch::now(),
357+
"sender": "@charlie:example.org", // Note! spoofed sender
358+
"type": "m.room.encrypted",
359+
"content": encrypted_content,
360+
});
361+
let event = json_convert(&event).unwrap();
362+
363+
bob.decrypt_room_event(&event, room_id, &decryption_settings)
364+
.await
365+
.expect("Bob could not decrypt spoofed event");
366+
367+
// The verification_state of the event should be `MissingDevice` (since it
368+
// manifests as a message from Charlie which does not correspond to one of
369+
// Charlie's devices).
370+
let event_encryption_info = bob.get_room_event_encryption_info(&event, room_id).await.unwrap();
371+
assert_matches!(
372+
event_encryption_info.verification_state,
373+
VerificationState::Unverified(VerificationLevel::None(DeviceLinkProblem::MissingDevice))
374+
);
375+
}
376+
314377
#[async_test]
315378
async fn test_verification_states_multiple_device() {
316379
let (bob, _) = get_prepared_machine_test_helper(tests::user_id(), false).await;

crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,26 @@ impl SenderData {
252252
Self::SenderVerified { .. } => SenderDataType::SenderVerified,
253253
}
254254
}
255+
256+
/// Return our best guess of the owner of the associated megolm session.
257+
///
258+
/// For `SenderData::UnknownDevice`, we don't record any information about
259+
/// the owner of the sender, so returns `None`.
260+
pub(crate) fn user_id(&self) -> Option<OwnedUserId> {
261+
match &self {
262+
SenderData::UnknownDevice { .. } => None,
263+
SenderData::DeviceInfo { device_keys, .. } => Some(device_keys.user_id.clone()),
264+
SenderData::VerificationViolation(known_sender_data) => {
265+
Some(known_sender_data.user_id.clone())
266+
}
267+
SenderData::SenderUnverified(known_sender_data) => {
268+
Some(known_sender_data.user_id.clone())
269+
}
270+
SenderData::SenderVerified(known_sender_data) => {
271+
Some(known_sender_data.user_id.clone())
272+
}
273+
}
274+
}
255275
}
256276

257277
/// Used when deserialising and the sender_data property is missing.

0 commit comments

Comments
 (0)