crypto: check sender_device_keys on incoming Olm messages

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

crates/matrix-sdk-crypto/src/error.rs

@@ -207,6 +207,14 @@ pub enum EventError {
         decrypted event: expected {0}, got {1:?}"
     )]
     MismatchedRoom(OwnedRoomId, Option<OwnedRoomId>),
+
+    /// 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

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<C>(machine: &OlmMachine, event: &ToDeviceEvent<C>)
+pub async fn receive_to_device_event<C>(
+    machine: &OlmMachine,
+    event: &ToDeviceEvent<C>,
+) -> (Vec<Raw<AnyToDeviceEvent>>, Vec<RoomKeyInfo>)
 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,

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"));
+}

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

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 {

crates/matrix-sdk-crypto/src/types/events/olm_v1.rs

@@ -152,7 +152,7 @@ impl AnyDecryptedOlmEvent {
     /// The sender's device keys, if supplied in the message as per MSC4147
     pub fn sender_device_keys(&self) -> Option<&DeviceKeys> {
         match self {
-            AnyDecryptedOlmEvent::Custom(_) => None,
+            AnyDecryptedOlmEvent::Custom(e) => e.sender_device_keys.as_ref(),
             AnyDecryptedOlmEvent::RoomKey(e) => e.sender_device_keys.as_ref(),
             AnyDecryptedOlmEvent::ForwardedRoomKey(e) => e.sender_device_keys.as_ref(),
             AnyDecryptedOlmEvent::SecretSend(e) => e.sender_device_keys.as_ref(),
@@ -274,6 +274,9 @@ pub struct ToDeviceCustomEvent {
     pub keys: OlmV1Keys,
     /// The recipient's signing keys of the encrypted event.
     pub recipient_keys: OlmV1Keys,
+    /// The device keys if supplied as per MSC4147
+    #[serde(alias = "org.matrix.msc4147.device_keys")]
+    pub sender_device_keys: Option<DeviceKeys>,
     /// The type of the event.
     #[serde(rename = "type")]
     pub event_type: String,