From e020ba10230d09088b5a2c56923fa797616c8248 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 11 Apr 2025 11:09:13 +0100 Subject: [PATCH] crypto: check `sender_device_keys` on incoming Olm messages MSC4147 added a `sender_device_keys` property to olm-encrypted to-device messages, with recommendations about checking the values in that propety. We do (most of?) those checks for `m.room_key` messages today, but not other types of to-device message. --- crates/matrix-sdk-crypto/CHANGELOG.md | 7 +++ crates/matrix-sdk-crypto/src/error.rs | 8 +++ .../src/machine/tests/megolm_sender_data.rs | 9 ++- .../src/machine/tests/olm_encryption.rs | 56 ++++++++++++++++++- crates/matrix-sdk-crypto/src/olm/account.rs | 54 ++++++++++++++++++ .../olm/group_sessions/sender_data_finder.rs | 5 +- 6 files changed, 134 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index 40de97f40..76dbb73b4 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -6,6 +6,13 @@ All notable changes to this project will be documented in this file. ## [Unreleased] - ReleaseDate +### Features + +- Check the `sender_device_keys` field on *all* incoming Olm-encrypted to-device messages + and ignore any to-device messages which include the field but whose data is invalid + (as per [MSC4147](https://github.com/matrix-org/matrix-spec-proposals/pull/4147)). + ([#4922](https://github.com/matrix-org/matrix-rust-sdk/pull/4922)) + ## [0.11.0] - 2025-04-11 ### Features diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index 6543d20d3..7d77144f7 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -207,6 +207,14 @@ pub enum EventError { decrypted event: expected {0}, got {1:?}" )] MismatchedRoom(OwnedRoomId, Option), + + /// The event includes `sender_device_keys` as per [MSC4147], but the + /// signature was invalid, or the ed25519 or curve25519 key did not + /// match other data in the event. + /// + /// [MSC4147]: https://github.com/matrix-org/matrix-spec-proposals/pull/4147 + #[error("the event included sender_device_keys which were invalid in some way")] + InvalidSenderDeviceKeys, } /// Error type describing different errors that can happen when we create an diff --git a/crates/matrix-sdk-crypto/src/machine/tests/megolm_sender_data.rs b/crates/matrix-sdk-crypto/src/machine/tests/megolm_sender_data.rs index 95d246b33..36477b241 100644 --- a/crates/matrix-sdk-crypto/src/machine/tests/megolm_sender_data.rs +++ b/crates/matrix-sdk-crypto/src/machine/tests/megolm_sender_data.rs @@ -18,7 +18,7 @@ use assert_matches::assert_matches; use futures_core::Stream; use futures_util::{FutureExt, StreamExt}; use matrix_sdk_test::async_test; -use ruma::{room_id, user_id, RoomId, TransactionId, UserId}; +use ruma::{events::AnyToDeviceEvent, room_id, serde::Raw, user_id, RoomId, TransactionId, UserId}; use serde::Serialize; use serde_json::json; use tokio_stream::wrappers::errors::BroadcastStreamRecvError; @@ -283,7 +283,10 @@ async fn create_and_share_session_without_sender_data( } /// Pipe a to-device event into an [`OlmMachine`]. -async fn receive_to_device_event(machine: &OlmMachine, event: &ToDeviceEvent) +pub async fn receive_to_device_event( + machine: &OlmMachine, + event: &ToDeviceEvent, +) -> (Vec>, Vec) where C: EventType + Serialize + Debug, { @@ -298,7 +301,7 @@ where next_batch_token: None, }) .await - .expect("Error receiving to-device event"); + .expect("Error receiving to-device event") } /// Given the `room_keys_received_stream`, check that there is a pending update, diff --git a/crates/matrix-sdk-crypto/src/machine/tests/olm_encryption.rs b/crates/matrix-sdk-crypto/src/machine/tests/olm_encryption.rs index afbc36017..478979a8c 100644 --- a/crates/matrix-sdk-crypto/src/machine/tests/olm_encryption.rs +++ b/crates/matrix-sdk-crypto/src/machine/tests/olm_encryption.rs @@ -24,12 +24,17 @@ use ruma::{ events::{dummy::ToDeviceDummyEventContent, AnyToDeviceEvent}, user_id, DeviceKeyAlgorithm, DeviceKeyId, SecondsSinceUnixEpoch, }; +use serde_json::json; use vodozemac::Ed25519SecretKey; use crate::{ machine::{ - test_helpers::{create_session, get_machine_pair, get_machine_pair_with_session}, + test_helpers::{ + create_session, get_machine_pair, get_machine_pair_with_session, + get_machine_pair_with_setup_sessions_test_helper, + }, tests, + tests::megolm_sender_data::receive_to_device_event, }, olm::utility::SignJson, store::Changes, @@ -242,3 +247,52 @@ async fn test_olm_encryption() { async fn test_olm_encryption_with_fallback_key() { olm_encryption_test_helper(true).await; } + +/// Encrypted to-device messages which hold a `sender_device_keys`, but that +/// data is unsigned, should not be decrypted. +#[async_test] +async fn test_decrypt_to_device_message_with_unsigned_sender_keys() { + let (alice, bob) = get_machine_pair_with_setup_sessions_test_helper( + tests::alice_id(), + tests::user_id(), + false, + ) + .await; + + let mut alice_session = alice + .get_device(bob.user_id(), bob.device_id(), None) + .await + .unwrap() + .unwrap() + .get_most_recent_session() + .await + .unwrap() + .unwrap(); + + let mut malformed_device_keys = alice_session.our_device_keys.clone(); + malformed_device_keys.signatures.clear(); + let plaintext = serde_json::to_string(&json!({ + "sender": alice.user_id(), + "sender_device": alice.device_id(), + "keys": { "ed25519": alice.identity_keys().ed25519.to_base64() }, + "recipient": bob.user_id(), + "recipient_keys": { "ed25519": bob.identity_keys().ed25519.to_base64() }, + "type": "org.matrix.test", + "content": {"a": "b"}, + "sender_device_keys": malformed_device_keys, + })) + .unwrap(); + + let ciphertext = alice_session.encrypt_helper(&plaintext).await; + let event = ToDeviceEvent::new( + alice.user_id().to_owned(), + alice_session.build_encrypted_event(ciphertext, None).await.unwrap(), + ); + + // Bob receives the to-device message + let (to_device_events, _) = receive_to_device_event(&bob, &event).await; + + // The to-device event should remain decrypted. + let event = to_device_events.first().expect("Bob did not get a to-device event"); + assert_eq!(event.get_field("type").unwrap(), Some("m.room.encrypted")); +} diff --git a/crates/matrix-sdk-crypto/src/olm/account.rs b/crates/matrix-sdk-crypto/src/olm/account.rs index 7f237f72e..ecd2d01b1 100644 --- a/crates/matrix-sdk-crypto/src/olm/account.rs +++ b/crates/matrix-sdk-crypto/src/olm/account.rs @@ -1464,6 +1464,9 @@ impl Account { ) .into()) } else { + // If the event contained sender_device_keys, check them now. + Self::check_sender_device_keys(event.as_ref(), sender_key)?; + // If this event is an `m.room_key` event, defer the check for the // Ed25519 key of the sender until we decrypt room events. This // ensures that we receive the room key even if we don't have access @@ -1496,6 +1499,57 @@ impl Account { } } + /// If the plaintext of the decrypted message includes a + /// `sender_device_keys` property per [MSC4147], check that it is valid. + /// + /// # Arguments + /// + /// * `event` - The decrypted and deserialized plaintext of the event. + /// * `sender_key` - The curve25519 key of the sender of the event. + /// + /// [MSC4147]: https://github.com/matrix-org/matrix-spec-proposals/pull/4147 + fn check_sender_device_keys( + event: &AnyDecryptedOlmEvent, + sender_key: Curve25519PublicKey, + ) -> OlmResult<()> { + let Some(sender_device_keys) = event.sender_device_keys() else { + return Ok(()); + }; + + // Check the signature within the device_keys structure + let sender_device_data = DeviceData::try_from(sender_device_keys).map_err(|err| { + warn!( + "Received a to-device message with sender_device_keys with invalid signature: {:?}", + err + ); + OlmError::EventError(EventError::InvalidSenderDeviceKeys) + })?; + + // Check that the Ed25519 key in the sender_device_keys matches the `ed25519` + // key in the `keys` field in the event. + if sender_device_data.ed25519_key() != Some(event.keys().ed25519) { + warn!( + "Received a to-device message with sender_device_keys with incorrect ed25519 key: expected {:?}, got {:?}", + event.keys().ed25519, + sender_device_data.ed25519_key(), + ); + return Err(OlmError::EventError(EventError::InvalidSenderDeviceKeys)); + } + + // Check that the Curve25519 key in the sender_device_keys matches the key that + // was used for the Olm session. + if sender_device_data.curve25519_key() != Some(sender_key) { + warn!( + "Received a to-device message with sender_device_keys with incorrect curve25519 key: expected {:?}, got {:?}", + sender_key, + sender_device_data.curve25519_key(), + ); + return Err(OlmError::EventError(EventError::InvalidSenderDeviceKeys)); + } + + Ok(()) + } + /// Internal use only. /// /// Cloning should only be done for testing purposes or when we are certain diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 2a9ee9b96..142195203 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -174,7 +174,10 @@ impl<'a> SenderDataFinder<'a> { if let Some(sender_device_keys) = &room_key_event.sender_device_keys { // Yes: use the device keys to continue. - // Validate the signature of the DeviceKeys supplied. + // Validate the signature of the DeviceKeys supplied. (We've actually already + // done this when decrypting the event, but doing it again here is + // relatively harmless and is the easiest way of getting hold of a + // DeviceData so that we can follow the rest of this logic). let sender_device_data = DeviceData::try_from(sender_device_keys)?; Ok(self.have_device_data(sender_device_data).await?) } else {