Merge pull request #4988 from matrix-org/rav/history_sharing/better_sender_data

crypto: improve SenderData stored with room key bundle data

Author: richvdh

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

@@ -952,9 +952,17 @@ impl OlmMachine {
             return Ok(());
         };
 
+        // We already checked that `sender_device_keys` matches the actual sender of the
+        // message when we decrypted the message, which included doing
+        // `DeviceData::try_from` on it, so it can't fail.
+
+        let sender_device_data =
+            DeviceData::try_from(sender_device_keys).expect("failed to verify sender device keys");
+        let sender_device = self.store().wrap_device_data(sender_device_data).await?;
+
         changes.received_room_key_bundles.push(StoredRoomKeyBundleData {
             sender_user: event.sender.clone(),
-            sender_data: SenderData::device_info(sender_device_keys.clone()),
+            sender_data: SenderData::from_device(&sender_device),
             bundle_data: event.content.clone(),
         });
         Ok(())

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

@@ -15,7 +15,7 @@
 //! A set of helper functions for creating [`OlmMachine`]s, and pairs of
 //! interconnected machines.
 
-use std::collections::BTreeMap;
+use std::{collections::BTreeMap, ops::Deref, sync::Arc};
 
 use as_variant::as_variant;
 use matrix_sdk_test::{ruma_response_from_json, test_json};
@@ -32,11 +32,14 @@ use ruma::{
     user_id, DeviceId, OwnedOneTimeKeyId, TransactionId, UserId,
 };
 use serde_json::json;
+use tokio::sync::Mutex;
 
 use crate::{
-    store::{Changes, MemoryStore},
-    types::{events::ToDeviceEvent, requests::AnyOutgoingRequest},
-    CrossSigningBootstrapRequests, DeviceData, OlmMachine,
+    olm::PrivateCrossSigningIdentity,
+    store::{Changes, CryptoStoreWrapper, MemoryStore},
+    types::{events::ToDeviceEvent, requests::AnyOutgoingRequest, DeviceKeys},
+    verification::VerificationMachine,
+    Account, CrossSigningBootstrapRequests, Device, DeviceData, OlmMachine, OtherUserIdentityData,
 };
 
 /// These keys need to be periodically uploaded to the server.
@@ -276,3 +279,99 @@ pub fn bootstrap_requests_to_keys_query_response(
 
     ruma_response_from_json(&kq_response)
 }
+
+/// Create a [`VerificationMachine`] which won't do any useful verification.
+///
+/// Helper for [`create_signed_device_of_unverified_user`] and
+/// [`create_unsigned_device`].
+fn dummy_verification_machine() -> VerificationMachine {
+    let account = Account::new(user_id!("@TEST_USER:example.com"));
+    VerificationMachine::new(
+        account.deref().clone(),
+        Arc::new(Mutex::new(PrivateCrossSigningIdentity::new(account.user_id().to_owned()))),
+        Arc::new(CryptoStoreWrapper::new(
+            account.user_id(),
+            account.device_id(),
+            MemoryStore::new(),
+        )),
+    )
+}
+
+/// Wrap the given [`DeviceKeys`] into a [`Device`], with no known owner
+/// identity.
+pub fn create_unsigned_device(device_keys: DeviceKeys) -> Device {
+    Device {
+        inner: DeviceData::try_from(&device_keys).unwrap(),
+        verification_machine: dummy_verification_machine(),
+        own_identity: None,
+        device_owner_identity: None,
+    }
+}
+
+/// Sign the given [`DeviceKeys`] with a cross-signing identity, and wrap it up
+/// as a [`Device`] with that identity.
+pub async fn create_signed_device_of_unverified_user(
+    mut device_keys: DeviceKeys,
+    device_owner_identity: &PrivateCrossSigningIdentity,
+) -> Device {
+    {
+        let self_signing = device_owner_identity.self_signing_key.lock().await;
+        let self_signing = self_signing.as_ref().unwrap();
+        self_signing.sign_device(&mut device_keys).unwrap();
+    }
+
+    let public_identity = OtherUserIdentityData::from_private(device_owner_identity).await;
+
+    let device = Device {
+        inner: DeviceData::try_from(&device_keys).unwrap(),
+        verification_machine: dummy_verification_machine(),
+        own_identity: None,
+        device_owner_identity: Some(public_identity.into()),
+    };
+    assert!(device.is_cross_signed_by_owner());
+    device
+}
+
+/// Sign the given [`DeviceKeys`] with a cross-signing identity, then sign the
+/// identity with our own identity, and wrap both up as a [`Device`].
+pub async fn create_signed_device_of_verified_user(
+    mut device_keys: DeviceKeys,
+    device_owner_identity: &PrivateCrossSigningIdentity,
+    own_identity: &PrivateCrossSigningIdentity,
+) -> Device {
+    // Sign the device keys with the owner's identity.
+    {
+        let self_signing = device_owner_identity.self_signing_key.lock().await;
+        let self_signing = self_signing.as_ref().unwrap();
+        self_signing.sign_device(&mut device_keys).unwrap();
+    }
+
+    let mut public_identity = OtherUserIdentityData::from_private(device_owner_identity).await;
+
+    // Sign the public identity of the owner with our own identity.
+    sign_user_identity_data(own_identity, &mut public_identity).await;
+
+    let device = Device {
+        inner: DeviceData::try_from(&device_keys).unwrap(),
+        verification_machine: dummy_verification_machine(),
+        own_identity: Some(own_identity.to_public_identity().await.unwrap()),
+        device_owner_identity: Some(public_identity.into()),
+    };
+    assert!(device.is_cross_signed_by_owner());
+    assert!(device.is_cross_signing_trusted());
+    device
+}
+
+/// Sign a public user identity with our own user-signing key.
+pub async fn sign_user_identity_data(
+    signer_private_identity: &PrivateCrossSigningIdentity,
+    other_user_identity: &mut OtherUserIdentityData,
+) {
+    let user_signing = signer_private_identity.user_signing_key.lock().await;
+
+    let user_signing = user_signing.as_ref().unwrap();
+    let master = user_signing.sign_user(&*other_user_identity).unwrap();
+    other_user_identity.master_key = Arc::new(master.try_into().unwrap());
+
+    user_signing.public_key().verify_master_key(other_user_identity.master_key()).unwrap();
+}

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

@@ -16,9 +16,13 @@ use std::{cmp::Ordering, fmt};
 
 use ruma::{DeviceId, OwnedDeviceId, OwnedUserId, UserId};
 use serde::{de, de::Visitor, Deserialize, Deserializer, Serialize};
+use tracing::error;
 use vodozemac::Ed25519PublicKey;
 
-use crate::types::{serialize_ed25519_key, DeviceKeys};
+use crate::{
+    types::{serialize_ed25519_key, DeviceKeys},
+    Device,
+};
 
 /// Information about the sender of a megolm session where we know the
 /// cross-signing identity of the sender.
@@ -216,6 +220,58 @@ impl SenderData {
         Self::UnknownDevice { legacy_session: true, owner_check_failed: false }
     }
 
+    /// Create a [`SenderData`] representing the current verification state of
+    /// the given device.
+    ///
+    /// Depending on whether the device is correctly cross-signed or not, and
+    /// whether the user has been verified or not, this can return
+    /// [`SenderData::DeviceInfo`], [`SenderData::VerificationViolation`],
+    /// [`SenderData::SenderUnverified`] or [`SenderData::SenderVerified`]
+    pub fn from_device(sender_device: &Device) -> Self {
+        // Is the device cross-signed?
+        // Does the cross-signing key match that used to sign the device?
+        // And is the signature in the device valid?
+        let cross_signed = sender_device.is_cross_signed_by_owner();
+
+        if cross_signed {
+            Self::from_cross_signed_device(sender_device)
+        } else {
+            // We have device keys, but they are not signed by the sender
+            SenderData::device_info(sender_device.as_device_keys().clone())
+        }
+    }
+
+    fn from_cross_signed_device(sender_device: &Device) -> Self {
+        let user_id = sender_device.user_id().to_owned();
+        let device_id = Some(sender_device.device_id().to_owned());
+
+        let device_owner = sender_device.device_owner_identity.as_ref();
+        let master_key = device_owner.and_then(|i| i.master_key().get_first_key());
+
+        match (device_owner, master_key) {
+            (Some(device_owner), Some(master_key)) => {
+                // We have user_id and master_key for the user sending the to-device message.
+                let master_key = Box::new(master_key);
+                let known_sender_data = KnownSenderData { user_id, device_id, master_key };
+                if sender_device.is_cross_signing_trusted() {
+                    Self::SenderVerified(known_sender_data)
+                } else if device_owner.was_previously_verified() {
+                    Self::VerificationViolation(known_sender_data)
+                } else {
+                    Self::SenderUnverified(known_sender_data)
+                }
+            }
+
+            (_, _) => {
+                // Surprisingly, there was no key in the MasterPubkey. We did not expect this:
+                // treat it as if the device was not signed by this master key.
+                //
+                error!("MasterPubkey for user {user_id} does not contain any keys!");
+                Self::device_info(sender_device.as_device_keys().clone())
+            }
+        }
+    }
+
     /// Returns `Greater` if this `SenderData` represents a greater level of
     /// trust than the supplied one, `Equal` if they have the same level, and
     /// `Less` if the supplied one has a greater level of trust.
@@ -344,10 +400,11 @@ pub enum SenderDataType {
 
 #[cfg(test)]
 mod tests {
-    use std::{cmp::Ordering, collections::BTreeMap};
+    use std::{cmp::Ordering, collections::BTreeMap, ops::Deref};
 
     use assert_matches2::assert_let;
     use insta::assert_json_snapshot;
+    use matrix_sdk_test::async_test;
     use ruma::{
         device_id, owned_device_id, owned_user_id, user_id, DeviceKeyAlgorithm, DeviceKeyId,
     };
@@ -356,8 +413,13 @@ mod tests {
 
     use super::SenderData;
     use crate::{
-        olm::{KnownSenderData, PickledInboundGroupSession},
+        machine::test_helpers::{
+            create_signed_device_of_unverified_user, create_signed_device_of_verified_user,
+            create_unsigned_device,
+        },
+        olm::{KnownSenderData, PickledInboundGroupSession, PrivateCrossSigningIdentity},
         types::{DeviceKey, DeviceKeys, EventEncryptionAlgorithm, Signatures},
+        Account,
     };
 
     #[test]
@@ -648,4 +710,108 @@ mod tests {
 
         assert_eq!(master_key.to_base64(), "kOp9s4ClyQujYD7rRZA8xgE6kvYlqKSNnMrQNmSrcuE");
     }
+
+    #[async_test]
+    async fn test_from_device_for_unsigned_device() {
+        let bob_account =
+            Account::with_device_id(user_id!("@bob:example.com"), device_id!("BOB_DEVICE"));
+        let bob_device = create_unsigned_device(bob_account.device_keys());
+
+        let sender_data = SenderData::from_device(&bob_device);
+
+        assert_eq!(
+            sender_data,
+            SenderData::DeviceInfo {
+                device_keys: bob_device.device_keys.deref().clone(),
+                legacy_session: false
+            }
+        );
+    }
+
+    #[async_test]
+    async fn test_from_device_for_unverified_user() {
+        let bob_identity =
+            PrivateCrossSigningIdentity::new(user_id!("@bob:example.com").to_owned());
+        let bob_account =
+            Account::with_device_id(user_id!("@bob:example.com"), device_id!("BOB_DEVICE"));
+        let bob_device = create_signed_device_of_unverified_user(
+            bob_account.device_keys().clone(),
+            &bob_identity,
+        )
+        .await;
+
+        let sender_data = SenderData::from_device(&bob_device);
+
+        assert_eq!(
+            sender_data,
+            SenderData::SenderUnverified(KnownSenderData {
+                user_id: bob_account.user_id().to_owned(),
+                device_id: Some(bob_account.device_id().to_owned()),
+                master_key: Box::new(
+                    bob_identity.master_public_key().await.unwrap().get_first_key().unwrap()
+                ),
+            })
+        );
+    }
+
+    #[async_test]
+    async fn test_from_device_for_verified_user() {
+        let alice_account =
+            Account::with_device_id(user_id!("@alice:example.com"), device_id!("ALICE_DEVICE"));
+        let alice_identity = PrivateCrossSigningIdentity::with_account(&alice_account).await.0;
+
+        let bob_identity =
+            PrivateCrossSigningIdentity::new(user_id!("@bob:example.com").to_owned());
+        let bob_account =
+            Account::with_device_id(user_id!("@bob:example.com"), device_id!("BOB_DEVICE"));
+        let bob_device = create_signed_device_of_verified_user(
+            bob_account.device_keys().clone(),
+            &bob_identity,
+            &alice_identity,
+        )
+        .await;
+
+        let sender_data = SenderData::from_device(&bob_device);
+
+        assert_eq!(
+            sender_data,
+            SenderData::SenderVerified(KnownSenderData {
+                user_id: bob_account.user_id().to_owned(),
+                device_id: Some(bob_account.device_id().to_owned()),
+                master_key: Box::new(
+                    bob_identity.master_public_key().await.unwrap().get_first_key().unwrap()
+                ),
+            })
+        );
+    }
+
+    #[async_test]
+    async fn test_from_device_for_verification_violation_user() {
+        let bob_identity =
+            PrivateCrossSigningIdentity::new(user_id!("@bob:example.com").to_owned());
+        let bob_account =
+            Account::with_device_id(user_id!("@bob:example.com"), device_id!("BOB_DEVICE"));
+        let bob_device =
+            create_signed_device_of_unverified_user(bob_account.device_keys(), &bob_identity).await;
+        bob_device
+            .device_owner_identity
+            .as_ref()
+            .unwrap()
+            .other()
+            .unwrap()
+            .mark_as_previously_verified();
+
+        let sender_data = SenderData::from_device(&bob_device);
+
+        assert_eq!(
+            sender_data,
+            SenderData::VerificationViolation(KnownSenderData {
+                user_id: bob_account.user_id().to_owned(),
+                device_id: Some(bob_account.device_id().to_owned()),
+                master_key: Box::new(
+                    bob_identity.master_public_key().await.unwrap().get_first_key().unwrap()
+                ),
+            })
+        );
+    }
 }