From bb7383d15ee26981b735f20e4a0d9174f4b69f1f Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 2 Jul 2024 09:33:18 +0200 Subject: [PATCH 01/38] feature(crypto): Add support for master key local pinning --- .../src/identities/manager.rs | 128 +++++++- .../matrix-sdk-crypto/src/identities/user.rs | 285 +++++++++++++++++- .../src/test_json/keys_query_sets.rs | 216 ++++++++++++- 3 files changed, 617 insertions(+), 12 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index dfbb4a47d55..699dfcade88 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -542,6 +542,7 @@ impl IdentityManager { *changed_private_identity = self.check_private_identity(&identity).await; Ok(identity.into()) } else { + // First time seen, create the identity. The current MSK will be pinned. let identity = ReadOnlyUserIdentity::new(master_key, self_signing)?; Ok(identity.into()) } @@ -1329,7 +1330,7 @@ pub(crate) mod tests { use std::ops::Deref; use futures_util::pin_mut; - use matrix_sdk_test::{async_test, response_from_file}; + use matrix_sdk_test::{async_test, response_from_file, test_json}; use ruma::{ api::{client::keys::get_keys::v3::Response as KeysQueryResponse, IncomingResponse}, device_id, user_id, TransactionId, @@ -1893,4 +1894,129 @@ pub(crate) mod tests { .unwrap() .unwrap(); } + + #[async_test] + async fn test_manager_identity_updates() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let manager = manager_test_helper(user_id(), device_id()).await; + let other_user = DataSet::user_id(); + let devices = manager.store.get_user_devices(other_user).await.unwrap(); + assert_eq!(devices.devices().count(), 0); + + let identity = manager.store.get_user_identity(other_user).await.unwrap(); + assert!(identity.is_none()); + + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_a(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // There should be now an identity and no pin violation (pinned msk is the + // current one) + assert!(!other_identity.has_pin_violation()); + let first_device = manager + .store + .get_readonly_device(other_user, DataSet::first_device_id()) + .await + .unwrap() + .unwrap(); + assert!(first_device.is_cross_signed_by_owner(&identity)); + + // We receive a new keys update for that user, with a new identity + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_b(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // The previous known identity has been replaced, there should be a pin + // violation + assert!(other_identity.has_pin_violation()); + + let second_device = manager + .store + .get_readonly_device(other_user, DataSet::second_device_id()) + .await + .unwrap() + .unwrap(); + + // There is a new device signed by the new identity + assert!(second_device.is_cross_signed_by_owner(&identity)); + + // The first device should not be signed by the new identity + let first_device = manager + .store + .get_readonly_device(other_user, DataSet::first_device_id()) + .await + .unwrap() + .unwrap(); + assert!(!first_device.is_cross_signed_by_owner(&identity)); + + let remember_previous_identity = other_identity.clone(); + // We receive a new keys update for that user, with no identity anymore + // Notice that there is no server API to delete identity, but we want to test + // here that a home server cannot clear the identity and serve a new one + // after that would get automatically approved. + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_no_identity(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + assert_eq!(other_identity, &remember_previous_identity); + assert!(other_identity.has_pin_violation()); + } + + #[async_test] + async fn test_manager_resolve_identity_mismatch() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let manager = manager_test_helper(user_id(), device_id()).await; + let other_user = DataSet::user_id(); + + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_a(), + ) + .await + .unwrap(); + + // We receive a new keys update for that user, with a new identity + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_b(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // We have a new identity now, so there should be a pin violation + assert!(other_identity.has_pin_violation()); + + // Resolve the misatch by pinning the new identity + other_identity.pin(); + + assert!(!other_identity.has_pin_violation()); + } } diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index b719f4bfa86..57adf365ee6 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -17,7 +17,7 @@ use std::{ ops::Deref, sync::{ atomic::{AtomicBool, Ordering}, - Arc, + Arc, RwLock, }, }; @@ -30,6 +30,7 @@ use ruma::{ DeviceId, EventId, OwnedDeviceId, OwnedUserId, RoomId, UserId, }; use serde::{Deserialize, Serialize}; +use serde_json::Value; use tracing::error; use super::{atomic_bool_deserializer, atomic_bool_serializer}; @@ -291,6 +292,36 @@ impl UserIdentity { methods, ) } + + /// Pin the current identity (Master public key). + pub async fn pin_current_master_key(&self) -> Result<(), CryptoStoreError> { + self.inner.pin(); + let to_save = ReadOnlyUserIdentities::Other(self.inner.clone()); + let changes = Changes { + identities: IdentityChanges { changed: vec![to_save], ..Default::default() }, + ..Default::default() + }; + self.verification_machine.store.inner().save_changes(changes).await?; + Ok(()) + } + + /// An identity mismatch is detected when there is a trust problem with the + /// user identity. There is an identity mismatch if the current identity + /// is not verified and there is a pinning violation. An identity + /// mismatch must be reported to the user, and can be resolved by: + /// - Verifying the new identity (see + /// [`UserIdentity::request_verification`]) + /// - Or by updating the pinned key + /// ([`UserIdentity::pin_current_master_key`]). + pub fn has_identity_mismatch(&self) -> bool { + // First check if the current identity is verified. + if self.is_verified() { + return false; + } + // If not we can check the pinned identity. Verification always have + // higher priority than pinning. + self.inner.has_pin_violation() + } } /// Enum over the different user identity types we can have. @@ -371,10 +402,87 @@ impl ReadOnlyUserIdentities { /// only contain a master key and a self signing key, meaning that only device /// signatures can be checked with this identity. #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct ReadOnlyUserIdentity { user_id: OwnedUserId, pub(crate) master_key: Arc, self_signing_key: Arc, + /// The first time a cryptographic identity is seen for a given user, it + /// will be associated to that user (i.e pinned). Future interaction + /// will expect this user crypto identity to stay the same, + /// this will help prevent some MITM attacks. + /// In case of identity change, it will be possible to pin the new identity + /// is the user wants. + pinned_msk: Arc>, +} + +/// Intermediate struct to help serialize ReadOnlyUserIdentity and support +/// versioning and migration. +/// Version v1 is adding support for identity pinning (`pinned_msk`), as part +/// of migration we just pin the currently known msk. +#[derive(Deserialize, Serialize)] +struct ReadOnlyUserIdentitySerializer { + version: Option, + #[serde(flatten)] + other: Value, +} + +#[derive(Debug, Deserialize, Serialize)] +struct ReadOnlyUserIdentityV0 { + user_id: OwnedUserId, + master_key: MasterPubkey, + self_signing_key: SelfSigningPubkey, +} + +#[derive(Debug, Deserialize, Serialize)] +struct ReadOnlyUserIdentityV1 { + user_id: OwnedUserId, + master_key: MasterPubkey, + self_signing_key: SelfSigningPubkey, + pinned_msk: MasterPubkey, +} + +impl From for ReadOnlyUserIdentity { + fn from(value: ReadOnlyUserIdentitySerializer) -> Self { + match value.version { + None => { + // Old format, migrate the pinned identity + let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other).unwrap(); + ReadOnlyUserIdentity { + user_id: v0.user_id, + master_key: Arc::new(v0.master_key.clone()), + self_signing_key: Arc::new(v0.self_signing_key), + // We migrate by pinning the current msk + pinned_msk: Arc::new(RwLock::new(v0.master_key.clone())), + } + } + _ => { + // v1 format + let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other).unwrap(); + ReadOnlyUserIdentity { + user_id: v1.user_id, + master_key: Arc::new(v1.master_key.clone()), + self_signing_key: Arc::new(v1.self_signing_key), + pinned_msk: Arc::new(RwLock::new(v1.pinned_msk)), + } + } + } + } +} + +impl From for ReadOnlyUserIdentitySerializer { + fn from(value: ReadOnlyUserIdentity) -> Self { + let v1 = ReadOnlyUserIdentityV1 { + user_id: value.user_id.clone(), + master_key: value.master_key().to_owned(), + self_signing_key: value.self_signing_key().to_owned(), + pinned_msk: value.pinned_msk.read().unwrap().clone(), + }; + ReadOnlyUserIdentitySerializer { + version: Some("1".to_owned()), + other: serde_json::to_value(v1).unwrap(), + } + } } impl PartialEq for ReadOnlyUserIdentity { @@ -417,19 +525,24 @@ impl ReadOnlyUserIdentity { Ok(Self { user_id: master_key.user_id().into(), - master_key: master_key.into(), + master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), + pinned_msk: RwLock::new(master_key).into(), }) } #[cfg(test)] pub(crate) async fn from_private(identity: &crate::olm::PrivateCrossSigningIdentity) -> Self { - let master_key = - identity.master_key.lock().await.as_ref().unwrap().public_key().clone().into(); + let master_key = identity.master_key.lock().await.as_ref().unwrap().public_key().clone(); let self_signing_key = identity.self_signing_key.lock().await.as_ref().unwrap().public_key().clone().into(); - Self { user_id: identity.user_id().into(), master_key, self_signing_key } + Self { + user_id: identity.user_id().into(), + master_key: Arc::new(master_key.clone()), + self_signing_key, + pinned_msk: Arc::new(RwLock::new(master_key.clone())), + } } /// Get the user id of this identity. @@ -447,6 +560,24 @@ impl ReadOnlyUserIdentity { &self.self_signing_key } + /// Pin the current identity + pub(crate) fn pin(&self) { + let mut m = self.pinned_msk.write().unwrap(); + *m = self.master_key.as_ref().clone() + } + + /// Key pinning acts as a trust on first use mechanism, the first time an + /// identity is known for a user it will be pinned. + /// For future interaction with a user, the identity is expected to be the + /// one that was pinned. In case of identity change the UI client should + /// receive reports of pinning violation and decide to act accordingly; + /// that is accept and pin the new identity, perform a verification or + /// stop communications. + pub(crate) fn has_pin_violation(&self) -> bool { + let pinned_msk = self.pinned_msk.read().unwrap(); + pinned_msk.get_first_key() != self.master_key().get_first_key() + } + /// Update the identity with a new master key and self signing key. /// /// # Arguments @@ -465,7 +596,15 @@ impl ReadOnlyUserIdentity { ) -> Result { master_key.verify_subkey(&self_signing_key)?; - let new = Self::new(master_key, self_signing_key)?; + // The pin is maintained. + let pinned_msk = self.pinned_msk.read().unwrap().clone(); + + let new = Self { + user_id: master_key.user_id().into(), + master_key: master_key.clone().into(), + self_signing_key: self_signing_key.into(), + pinned_msk: RwLock::new(pinned_msk).into(), + }; let changed = new != *self; *self = new; @@ -786,8 +925,11 @@ pub(crate) mod tests { use std::{collections::HashMap, sync::Arc}; use assert_matches::assert_matches; - use matrix_sdk_test::async_test; - use ruma::{device_id, user_id}; + use matrix_sdk_test::{async_test, response_from_file, test_json}; + use ruma::{ + api::{client::keys::get_keys::v3::Response as KeyQueryResponse, IncomingResponse}, + device_id, user_id, TransactionId, + }; use serde_json::{json, Value}; use tokio::sync::Mutex; @@ -796,11 +938,16 @@ pub(crate) mod tests { ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, }; use crate::{ - identities::{manager::testing::own_key_query, Device}, + identities::{ + manager::testing::own_key_query, + user::{ReadOnlyUserIdentitySerializer, ReadOnlyUserIdentityV1}, + Device, + }, olm::{Account, PrivateCrossSigningIdentity}, store::{CryptoStoreWrapper, MemoryStore}, types::{CrossSigningKey, MasterPubkey, SelfSigningPubkey, Signatures, UserSigningPubkey}, verification::VerificationMachine, + OlmMachine, ReadOnlyUserIdentity, }; #[test] @@ -861,6 +1008,56 @@ pub(crate) mod tests { get_other_identity(); } + #[test] + fn deserialization_migration_test() { + let serialized_value = json!({ + "user_id":"@example2:localhost", + "master_key":{ + "user_id":"@example2:localhost", + "usage":[ + "master" + ], + "keys":{ + "ed25519:kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do":"kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do" + }, + "signatures":{ + "@example2:localhost":{ + "ed25519:SKISMLNIMH":"KdUZqzt8VScGNtufuQ8lOf25byYLWIhmUYpPENdmM8nsldexD7vj+Sxoo7PknnTX/BL9h2N7uBq0JuykjunCAw" + } + } + }, + "self_signing_key":{ + "user_id":"@example2:localhost", + "usage":[ + "self_signing" + ], + "keys":{ + "ed25519:ZtFrSkJ1qB8Jph/ql9Eo/lKpIYCzwvKAKXfkaS4XZNc":"ZtFrSkJ1qB8Jph/ql9Eo/lKpIYCzwvKAKXfkaS4XZNc" + }, + "signatures":{ + "@example2:localhost":{ + "ed25519:kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do":"W/O8BnmiUETPpH02mwYaBgvvgF/atXnusmpSTJZeUSH/vHg66xiZOhveQDG4cwaW8iMa+t9N4h1DWnRoHB4mCQ" + } + } + } + }); + let migrated: ReadOnlyUserIdentity = serde_json::from_value(serialized_value).unwrap(); + + let pinned_msk = migrated.pinned_msk.read().unwrap(); + assert_eq!(*pinned_msk, migrated.master_key().clone()); + + // Serialize back + let value = serde_json::to_value(migrated.clone()).unwrap(); + + // Should be serialized with latest version + let _: ReadOnlyUserIdentityV1 = + serde_json::from_value(value.clone()).expect("Should deserialize as version 1"); + + let with_serializer: ReadOnlyUserIdentitySerializer = + serde_json::from_value(value).unwrap(); + assert_eq!("1", with_serializer.version.unwrap()); + } + #[test] fn own_identity_check_signatures() { let response = own_key_query(); @@ -1015,4 +1212,74 @@ pub(crate) mod tests { [second_device_id] ); } + + #[async_test] + async fn resolve_identity_mismacth_with_verification() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let my_user_id = user_id!("@me:localhost"); + let machine = OlmMachine::new(my_user_id, device_id!("ABCDEFGH")).await; + machine.bootstrap_cross_signing(false).await.unwrap(); + + let my_id = machine.get_identity(my_user_id, None).await.unwrap().unwrap().own().unwrap(); + let usk_key_id = my_id.inner.user_signing_key().keys().iter().next().unwrap().0; + + println!("USK ID: {}", usk_key_id); + let keys_query = DataSet::key_query_with_identity_a(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + // Simulate an identity hange + let keys_query = DataSet::key_query_with_identity_b(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let other_user_id = DataSet::user_id(); + + let other_identity = + machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); + + // There should be an identity mismatch + assert!(other_identity.has_identity_mismatch()); + + // Manually verify for the purpose of this test + let sig_upload = other_identity.verify().await.unwrap(); + + let raw_extracted = + sig_upload.signed_keys.get(other_user_id).unwrap().iter().next().unwrap().1.get(); + + let new_signature: CrossSigningKey = serde_json::from_str(raw_extracted).unwrap(); + + let mut msk_to_update: CrossSigningKey = + serde_json::from_value(DataSet::msk_b().get("@bob:localhost").unwrap().clone()) + .unwrap(); + + msk_to_update.signatures.add_signature( + my_user_id.to_owned(), + usk_key_id.to_owned(), + new_signature.signatures.get_signature(my_user_id, usk_key_id).unwrap(), + ); + + // we want to update bob device keys with the new signature + let data = json!({ + "device_keys": {}, // For the purpose of this test we don't need devices here + "failures": {}, + "master_keys": { + DataSet::user_id(): msk_to_update + , + }, + "self_signing_keys": DataSet::ssk_b(), + }); + + let kq_response = KeyQueryResponse::try_from_http_response(response_from_file(&data)) + .expect("Can't parse the `/keys/upload` response"); + machine.mark_request_as_sent(&TransactionId::new(), &kq_response).await.unwrap(); + + // There should not be an identity mismatch anymore + let other_identity = + machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); + assert!(!other_identity.has_identity_mismatch()); + // But there is still a pin violation + assert!(other_identity.inner.has_pin_violation()); + } } diff --git a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs index c4e06e190fc..755369902bf 100644 --- a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs +++ b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs @@ -2,7 +2,7 @@ use ruma::{ api::{client::keys::get_keys::v3::Response as KeyQueryResponse, IncomingResponse}, device_id, user_id, DeviceId, UserId, }; -use serde_json::json; +use serde_json::{json, Value}; use crate::response_from_file; @@ -105,7 +105,7 @@ impl KeyDistributionTestData { /// but not the other one `FRGNMZVOKA`. /// `@dan` identity is signed by `@me` identity (alice trust dan) pub fn dan_keys_query_response() -> KeyQueryResponse { - let data: serde_json::Value = json!({ + let data: Value = json!({ "device_keys": { "@dan:localhost": { "JHPUERYQUW": { @@ -457,3 +457,215 @@ impl KeyDistributionTestData { user_id!("@good:localhost") } } + +/// A set of keys query to test identity changes, +/// For user @bob, several payloads with no identities then identity A and B. +pub struct IdentityChangeDataSet {} + +#[allow(dead_code)] +impl IdentityChangeDataSet { + pub fn user_id() -> &'static UserId { + user_id!("@bob:localhost") + } + + pub fn first_device_id() -> &'static DeviceId { + device_id!("GYKSNAWLVK") + } + + pub fn second_device_id() -> &'static DeviceId { + device_id!("ATWKQFSFRN") + } + + pub fn third_device_id() -> &'static DeviceId { + device_id!("OPABMDDXGX") + } + + fn device_keys_payload_1_signed_by_a() -> Value { + json!({ + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "GYKSNAWLVK", + "keys": { + "curve25519:GYKSNAWLVK": "dBcZBzQaiQYWf6rBPh2QypIOB/dxSoTeyaFaxNNbeHs", + "ed25519:GYKSNAWLVK": "6melQNnhoI9sT2b4VzNPAwa8aB179ym45fON8Yo7kVk" + }, + "signatures": { + "@bob:localhost": { + "ed25519:GYKSNAWLVK": "Fk45zHAbrd+1j9wZXLjL2Y/+DU/Mnz9yuvlfYBOOT7qExN2Jdud+5BAuNs8nZ/caS4wTF39Kg3zQpzaGERoCBg", + "ed25519:dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw": "md0Pa1MYlneFb1fp6KCsvZpi2ySb6/G+ULoCbQDWBeDxNEcoNMzf7PEKY04UToCZKUU4LifvRWmiWFDanOlkCQ" + } + }, + "user_id": "@bob:localhost", + }) + } + + fn msk_a() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY" + }, + "signatures": { + "@bob:localhost": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "6vGDbPO5XzlcwbU3aV+kcck+iHHEBtX85ow2gW5U05/DZdtda/JNVa5Nn7B9lQHNnnrMqt1sX00y/JrIkSS1Aw", + "ed25519:GYKSNAWLVK": "jLxmUPr0Ny2Ai9+NGKGhed9BAuKikOc7r6gr7MQVawePYS95w8NJ8Tzaq9zFFOmIiojACNdQ/ksy3QAdwD6vBQ" + } + }, + "usage": [ + "master" + ], + "user_id": "@bob:localhost" + } + }) + } + fn ssk_a() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw": "dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw" + }, + "signatures": { + "@bob:localhost": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "7md6mwjUK8zjintmffJ0+kImC59/Y8PdySy99EZz5Neu+VMX3LT7txhKO2gC/hmDduRw+JGfGXIiDxR7GmQqDw" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@bob:localhost" + } + }) + } + /// A key query with an identity (Ia), and a first device `GYKSNAWLVK` + /// signed by Ia. + pub fn key_query_with_identity_a() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a() + } + }, + "failures": {}, + "master_keys": Self::msk_a(), + "self_signing_keys": Self::ssk_a(), + "user_signing_keys": {} + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + pub fn msk_b() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4" + }, + "signatures": { + "@bob:localhost": { + "ed25519:ATWKQFSFRN": "MBOzCKYPQLQMpBY2lFZJ4c8451xJfQCdhPBb1AHlTUSxKFiWi6V+k1oRRnhQein/PjkIY7ZO+HoOrIeOtbRMAw", + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "xqLhC3sIUci1W2CNVW7HZWXreQApgjv2RDwB0WPiMd1P4vbZ/qJM0KWqK2piGPWliPi8YVREMrg216KXM3IhCA" + } + }, + "usage": [ + "master" + ], + "user_id": "@bob:localhost" + } + }) + } + + pub fn ssk_b() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc": "At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc" + }, + "signatures": { + "@bob:localhost": { + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "Ls6CeoA4LoPCHuSwG96kbhd1dEV09TgdMROIZi6vFz/MT9Wtik6joQi/tQ3zCwIZCSR53ksLO4jG1DD31AiBAA" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@bob:localhost" + } + }) + } + + pub fn device_keys_payload_2_signed_by_b() -> Value { + json!({ + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "ATWKQFSFRN", + "keys": { + "curve25519:ATWKQFSFRN": "CY0TWVK1/Kj3ZADuBcGe3UKvpT+IKAPMUsMeJhSDqno", + "ed25519:ATWKQFSFRN": "TyTQqd6j2JlWZh97r+kTYuCbvqnPoNwO6EGovYsjY00" + }, + "signatures": { + "@bob:localhost": { + "ed25519:ATWKQFSFRN": "BQ9Gp0p+6srF+c8OyruqKKd9R4yaub3THYAyyBB/7X/rG8BwcAqFynzl1aGyFYun4Q+087a5OSiglCXI+/kQAA", + "ed25519:At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc": "TWmDPaG7t0rZ6luauonELD3dmBDTIRryqXhgsIQRiGint2rJdic8RVyZ6a61bgu6mtBjfvU3prqMNp6sVi16Cg" + } + }, + "user_id": "@bob:localhost", + }) + } + /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. + /// `ATWKQFSFRN` is signed with the new identity but + pub fn key_query_with_identity_b() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "ATWKQFSFRN": Self::device_keys_payload_2_signed_by_b(), + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a(), + } + }, + "failures": {}, + "master_keys": Self::msk_b(), + "self_signing_keys": Self::ssk_b(), + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. + /// `ATWKQFSFRN` is signed with the new identity but + pub fn key_query_with_identity_no_identity() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "ATWKQFSFRN": Self::device_keys_payload_2_signed_by_b(), + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a(), + "OPABMDDXGX": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "OPABMDDXGX", + "keys": { + "curve25519:OPABMDDXGX": "O6bwa9Op0E+PQPCrbTOfdYwU+j95RRPhXIHuNpe94ns", + "ed25519:OPABMDDXGX": "DvjkSNOM9XrR1gWrr2YSDvTnwnLIgKDMRr5v8HgMKak" + }, + "signatures": { + "@bob:localhost": { + "ed25519:OPABMDDXGX": "o+BBnw/SIJWxSf799Adq6jEl9X3lwCg5MJkS8GlfId+pW3ReEETK0l+9bhCAgBsNSKRtB/fmZQBhjMx4FJr+BA" + } + }, + "user_id": "@bob:localhost", + "unsigned": { + "device_display_name": "develop.element.io: Chrome on macOS" + } + } + } + }, + "failures": {}, + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } +} From e08dfabd8d6e2db7a754d628497fe55917b55dff Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 11 Jul 2024 18:25:41 -0400 Subject: [PATCH 02/38] initial decryption implementation --- crates/matrix-sdk-crypto/src/error.rs | 9 + .../matrix-sdk-crypto/src/identities/user.rs | 6 + crates/matrix-sdk-crypto/src/machine.rs | 178 +++++++++++++++--- .../src/olm/group_sessions/inbound.rs | 19 ++ .../src/olm/group_sessions/mod.rs | 4 +- crates/matrix-sdk-crypto/src/olm/mod.rs | 3 +- 6 files changed, 188 insertions(+), 31 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index cedb9712da5..d7e5ed3625c 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -115,6 +115,15 @@ pub enum MegolmError { /// The storage layer returned an error. #[error(transparent)] Store(#[from] CryptoStoreError), + + /// The sender's cross-signing identity has changed, and the change hasn't + /// been acknowledged + #[error("message quarantined because sender's cross-signing identity has changed")] + SenderCrossSigningIdentityChanged, + + /// The sender's cross-signing identity is unknown + #[error("message quarantined because sender is unknown")] + SenderCrossSigningIdentityUnknown, } /// Error that occurs when decrypting an event that is malformed. diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 57adf365ee6..ad434c9f66a 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -32,6 +32,7 @@ use ruma::{ use serde::{Deserialize, Serialize}; use serde_json::Value; use tracing::error; +use vodozemac::Ed25519PublicKey; use super::{atomic_bool_deserializer, atomic_bool_serializer}; use crate::{ @@ -578,6 +579,11 @@ impl ReadOnlyUserIdentity { pinned_msk.get_first_key() != self.master_key().get_first_key() } + pub(crate) fn matches_pinned_identity(&self, msk: &Ed25519PublicKey) -> bool { + let pinned_msk = self.pinned_msk.read().unwrap(); + pinned_msk.get_first_key() == Some(*msk) + } + /// Update the identity with a new master key and self signing key. /// /// # Arguments diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 95822e77454..4ec0a51af01 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -65,8 +65,9 @@ use crate::{ gossiping::GossipMachine, identities::{user::UserIdentities, Device, IdentityManager, UserDevices}, olm::{ - Account, CrossSigningStatus, EncryptionSettings, IdentityKeys, InboundGroupSession, - OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, SessionType, StaticAccountData, + Account, CrossSigningStatus, DecryptionSettings, EncryptionSettings, IdentityKeys, + InboundGroupSession, OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, + SessionType, StaticAccountData, TrustRequirement, }, requests::{IncomingResponse, OutgoingRequest, UploadSigningKeysRequest}, session_manager::{GroupSessionManager, SessionManager}, @@ -1539,15 +1540,86 @@ impl OlmMachine { self.get_encryption_info(&session, &event.sender).await } + async fn check_sender_trust_requirement( + &self, + session: &InboundGroupSession, + sender: &UserId, + decryption_settings: &DecryptionSettings, + ) -> MegolmResult<()> { + match decryption_settings.trust_requirement { + TrustRequirement::Untrusted => Ok(()), + TrustRequirement::CrossSignedOrLegacy => { + match session.sender_data { + SenderData::SenderKnown { ref msk, .. } => { + if let Some(identity) = self.inner.store.get_identity(sender).await? { + if sender == self.inner.user_id { + // FIXME: check whether identity matches + Ok(()) + } else { + let identity = identity.other().unwrap(); + if !identity.matches_pinned_identity(msk) { + Err(MegolmError::SenderCrossSigningIdentityChanged) + } else { + Ok(()) + } + } + } else { + // FIXME: is it right that we should be rejecting here? + Err(MegolmError::SenderCrossSigningIdentityUnknown) + } + } + SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), + SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), + _ => Err(MegolmError::SenderCrossSigningIdentityUnknown), + } + } + TrustRequirement::CrossSigned => { + match session.sender_data { + SenderData::SenderKnown { ref msk, .. } => { + if let Some(identity) = self.inner.store.get_identity(sender).await? { + if sender == self.inner.user_id { + // FIXME: check whether identity matches + Ok(()) + } else { + let identity = identity.other().unwrap(); + if !identity.matches_pinned_identity(msk) { + Err(MegolmError::SenderCrossSigningIdentityChanged) + } else { + Ok(()) + } + } + } else { + // FIXME: is it right that we should be rejecting here? + Err(MegolmError::SenderCrossSigningIdentityUnknown) + } + } + _ => Err(MegolmError::SenderCrossSigningIdentityUnknown), + } + } + TrustRequirement::VerifiedUser => { + match session.sender_data { + SenderData::SenderKnown { msk_verified: true, .. } => { + // FIXME: is this all the checking we need to do? + Ok(()) + } + _ => Err(MegolmError::SenderCrossSigningIdentityUnknown), + } + } + } + } + async fn decrypt_megolm_events( &self, room_id: &RoomId, event: &EncryptedEvent, content: &SupportedEventEncryptionSchemes<'_>, + decryption_settings: &DecryptionSettings, ) -> MegolmResult<(JsonObject, EncryptionInfo)> { let session = self.get_inbound_group_session_or_error(room_id, content.session_id()).await?; + self.check_sender_trust_requirement(&session, &event.sender, decryption_settings).await?; + // This function is only ever called by decrypt_room_event, so // room_id, sender, algorithm and session_id are recorded already // @@ -1617,8 +1689,9 @@ impl OlmMachine { &self, event: &Raw, room_id: &RoomId, + decryption_settings: &DecryptionSettings, ) -> MegolmResult { - self.decrypt_room_event_inner(event, room_id, true).await + self.decrypt_room_event_inner(event, room_id, decryption_settings, true).await } #[instrument(name = "decrypt_room_event", skip_all, fields(?room_id, event_id, origin_server_ts, sender, algorithm, session_id, sender_key))] @@ -1626,6 +1699,7 @@ impl OlmMachine { &self, event: &Raw, room_id: &RoomId, + decryption_settings: &DecryptionSettings, decrypt_unsigned: bool, ) -> MegolmResult { let event = event.deserialize()?; @@ -1654,7 +1728,8 @@ impl OlmMachine { }; Span::current().record("session_id", content.session_id()); - let result = self.decrypt_megolm_events(room_id, &event, &content).await; + let result = + self.decrypt_megolm_events(room_id, &event, &content, decryption_settings).await; if let Err(e) = &result { #[cfg(feature = "automatic-room-key-forwarding")] @@ -1679,8 +1754,9 @@ impl OlmMachine { let mut unsigned_encryption_info = None; if decrypt_unsigned { // Try to decrypt encrypted unsigned events. - unsigned_encryption_info = - self.decrypt_unsigned_events(&mut decrypted_event, room_id).await; + unsigned_encryption_info = self + .decrypt_unsigned_events(&mut decrypted_event, room_id, decryption_settings) + .await; } let event = serde_json::from_value::>(decrypted_event.into())?; @@ -1706,6 +1782,7 @@ impl OlmMachine { &self, main_event: &mut JsonObject, room_id: &RoomId, + decryption_settings: &DecryptionSettings, ) -> Option> { let unsigned = main_event.get_mut("unsigned")?.as_object_mut()?; let mut unsigned_encryption_info: Option< @@ -1715,7 +1792,9 @@ impl OlmMachine { // Search for an encrypted event in `m.replace`, an edit. let location = UnsignedEventLocation::RelationsReplace; let replace = location.find_mut(unsigned); - if let Some(decryption_result) = self.decrypt_unsigned_event(replace, room_id).await { + if let Some(decryption_result) = + self.decrypt_unsigned_event(replace, room_id, decryption_settings).await + { unsigned_encryption_info .get_or_insert_with(Default::default) .insert(location, decryption_result); @@ -1726,7 +1805,7 @@ impl OlmMachine { let location = UnsignedEventLocation::RelationsThreadLatestEvent; let thread_latest_event = location.find_mut(unsigned); if let Some(decryption_result) = - self.decrypt_unsigned_event(thread_latest_event, room_id).await + self.decrypt_unsigned_event(thread_latest_event, room_id, decryption_settings).await { unsigned_encryption_info .get_or_insert_with(Default::default) @@ -1747,6 +1826,7 @@ impl OlmMachine { &'a self, event: Option<&'a mut Value>, room_id: &'a RoomId, + decryption_settings: &'a DecryptionSettings, ) -> BoxFuture<'a, Option> { Box::pin(async move { let event = event?; @@ -1760,7 +1840,10 @@ impl OlmMachine { } let raw_event = serde_json::from_value(event.clone()).ok()?; - match self.decrypt_room_event_inner(&raw_event, room_id, false).await { + match self + .decrypt_room_event_inner(&raw_event, room_id, decryption_settings, false) + .await + { Ok(decrypted_event) => { // Replace the encrypted event. *event = serde_json::to_value(decrypted_event.event).ok()?; @@ -2420,8 +2503,8 @@ pub(crate) mod tests { error::{EventError, SetRoomSettingsError}, machine::{EncryptionSyncChanges, OlmMachine}, olm::{ - BackedUpRoomKey, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, - SenderData, VerifyJson, + BackedUpRoomKey, DecryptionSettings, ExportedRoomKey, InboundGroupSession, + OutboundGroupSession, SenderData, TrustRequirement, VerifyJson, }, session_manager::CollectStrategy, store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore, RoomSettings}, @@ -3211,8 +3294,15 @@ pub(crate) mod tests { let event = json_convert(&event).unwrap(); - let decrypted_event = - bob.decrypt_room_event(&event, room_id).await.unwrap().event.deserialize().unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let decrypted_event = bob + .decrypt_room_event(&event, room_id, &decryption_settings) + .await + .unwrap() + .event + .deserialize() + .unwrap(); if let AnyTimelineEvent::MessageLike(AnyMessageLikeEvent::RoomMessage( MessageLikeEvent::Original(OriginalMessageLikeEvent { sender, content, .. }), @@ -3298,7 +3388,10 @@ pub(crate) mod tests { }); let room_event = json_convert(&room_event).unwrap(); - let decrypt_result = bob.decrypt_room_event(&room_event, room_id).await; + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let decrypt_result = + bob.decrypt_room_event(&room_event, room_id, &decryption_settings).await; assert_matches!(&decrypt_result, Err(MegolmError::MissingRoomKey(Some(_)))); @@ -3365,8 +3458,14 @@ pub(crate) mod tests { let event = json_convert(&event).unwrap(); - let encryption_info = - bob.decrypt_room_event(&event, room_id).await.unwrap().encryption_info.unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let encryption_info = bob + .decrypt_room_event(&event, room_id, &decryption_settings) + .await + .unwrap() + .encryption_info + .unwrap(); assert_eq!( VerificationState::Unverified(VerificationLevel::UnsignedDevice), @@ -3466,9 +3565,11 @@ pub(crate) mod tests { let event = json_convert(&event).unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; // decrypt_room_event should return an error assert_matches!( - bob.decrypt_room_event(&event, room_id).await, + bob.decrypt_room_event(&event, room_id, &decryption_settings).await, Err(MegolmError::JsonError(..)) ); @@ -3811,7 +3912,10 @@ pub(crate) mod tests { let room_event = json_convert(&room_event).unwrap(); - let decrypt_error = bob.decrypt_room_event(&room_event, room_id).await.unwrap_err(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let decrypt_error = + bob.decrypt_room_event(&room_event, room_id, &decryption_settings).await.unwrap_err(); if let MegolmError::Decryption(vodo_error) = decrypt_error { if let vodozemac::megolm::DecryptionError::UnknownMessageIndex(_, _) = vodo_error { @@ -4192,8 +4296,10 @@ pub(crate) mod tests { }); let event = json_convert(&event).unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; assert_matches!( - alice.decrypt_room_event(&event, room_id).await, + alice.decrypt_room_event(&event, room_id, &decryption_settings).await, Err(MegolmError::MismatchedIdentityKeys { .. }) ); } @@ -4652,9 +4758,13 @@ pub(crate) mod tests { }); let raw_encrypted_event = json_convert(&first_message_encrypted_event).unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; // Bob has the room key, so first message should be decrypted successfully. - let raw_decrypted_event = - bob.decrypt_room_event(&raw_encrypted_event, room_id).await.unwrap(); + let raw_decrypted_event = bob + .decrypt_room_event(&raw_encrypted_event, room_id, &decryption_settings) + .await + .unwrap(); let decrypted_event = raw_decrypted_event.event.deserialize().unwrap(); assert_matches!( @@ -4707,10 +4817,14 @@ pub(crate) mod tests { .insert("unsigned".to_owned(), relations); let raw_encrypted_event = json_convert(&first_message_encrypted_event).unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; // Bob does not have the second room key, so second message should fail to // decrypt. - let raw_decrypted_event = - bob.decrypt_room_event(&raw_encrypted_event, room_id).await.unwrap(); + let raw_decrypted_event = bob + .decrypt_room_event(&raw_encrypted_event, room_id, &decryption_settings) + .await + .unwrap(); let decrypted_event = raw_decrypted_event.event.deserialize().unwrap(); assert_matches!( @@ -4757,8 +4871,10 @@ pub(crate) mod tests { bob.store().save_inbound_group_sessions(&[group_session]).await.unwrap(); // Second message should decrypt now. - let raw_decrypted_event = - bob.decrypt_room_event(&raw_encrypted_event, room_id).await.unwrap(); + let raw_decrypted_event = bob + .decrypt_room_event(&raw_encrypted_event, room_id, &decryption_settings) + .await + .unwrap(); let decrypted_event = raw_decrypted_event.event.deserialize().unwrap(); assert_matches!( @@ -4825,8 +4941,10 @@ pub(crate) mod tests { // Bob does not have the third room key, so third message should fail to // decrypt. - let raw_decrypted_event = - bob.decrypt_room_event(&raw_encrypted_event, room_id).await.unwrap(); + let raw_decrypted_event = bob + .decrypt_room_event(&raw_encrypted_event, room_id, &decryption_settings) + .await + .unwrap(); let decrypted_event = raw_decrypted_event.event.deserialize().unwrap(); assert_matches!( @@ -4881,8 +4999,10 @@ pub(crate) mod tests { bob.store().save_inbound_group_sessions(&[group_session]).await.unwrap(); // Third message should decrypt now. - let raw_decrypted_event = - bob.decrypt_room_event(&raw_encrypted_event, room_id).await.unwrap(); + let raw_decrypted_event = bob + .decrypt_room_event(&raw_encrypted_event, room_id, &decryption_settings) + .await + .unwrap(); let decrypted_event = raw_decrypted_event.event.deserialize().unwrap(); assert_matches!( diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index dac2c21794f..42b2dd97b51 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -55,6 +55,25 @@ use crate::{ }, }; +/// The trust level required to decrypt an event +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +pub enum TrustRequirement { + /// Decrypt events from everyone regardless of trust + Untrusted, + /// Only decrypt events from cross-signed or legacy devices + CrossSignedOrLegacy, + /// Only decrypt events from cross-signed devices + CrossSigned, + /// Only decrypt events from cross-signed devices where we have verified the user + VerifiedUser, +} + +/// Settings for decrypting messages +#[derive(Clone, Debug, Deserialize, Serialize)] +pub struct DecryptionSettings { + pub trust_requirement: TrustRequirement, +} + // TODO add creation times to the inbound group sessions so we can export // sessions that were created between some time period, this should only be set // for non-imported sessions. diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs index cf1c5255357..80ec6d5abfd 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs @@ -19,7 +19,9 @@ mod inbound; mod outbound; mod sender_data; -pub use inbound::{InboundGroupSession, PickledInboundGroupSession}; +pub use inbound::{ + DecryptionSettings, InboundGroupSession, PickledInboundGroupSession, TrustRequirement, +}; pub(crate) use outbound::ShareState; pub use outbound::{ EncryptionSettings, OutboundGroupSession, PickledOutboundGroupSession, ShareInfo, diff --git a/crates/matrix-sdk-crypto/src/olm/mod.rs b/crates/matrix-sdk-crypto/src/olm/mod.rs index 51396e67c89..e40f2123c7c 100644 --- a/crates/matrix-sdk-crypto/src/olm/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/mod.rs @@ -27,9 +27,10 @@ pub use account::{Account, OlmMessageHash, PickledAccount, StaticAccountData}; pub(crate) use account::{OlmDecryptionInfo, SessionType}; pub(crate) use group_sessions::ShareState; pub use group_sessions::{ - BackedUpRoomKey, EncryptionSettings, ExportedRoomKey, InboundGroupSession, + BackedUpRoomKey, DecryptionSettings, EncryptionSettings, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, PickledInboundGroupSession, PickledOutboundGroupSession, SenderData, SenderDataRetryDetails, SessionCreationError, SessionExportError, SessionKey, ShareInfo, + TrustRequirement, }; pub use session::{PickledSession, Session}; pub use signing::{CrossSigningStatus, PickledCrossSigningIdentity, PrivateCrossSigningIdentity}; From 94153086a0ac7c96c2ab6fe47b19740ba8de8bf1 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 15 Jul 2024 17:42:07 -0400 Subject: [PATCH 03/38] add test for trust requirement --- crates/matrix-sdk-crypto/src/machine.rs | 129 +++++++++++++++++- .../src/olm/group_sessions/inbound.rs | 1 + 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 4ec0a51af01..26797408790 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -2504,7 +2504,7 @@ pub(crate) mod tests { machine::{EncryptionSyncChanges, OlmMachine}, olm::{ BackedUpRoomKey, DecryptionSettings, ExportedRoomKey, InboundGroupSession, - OutboundGroupSession, SenderData, TrustRequirement, VerifyJson, + OutboundGroupSession, SenderData, SenderDataRetryDetails, TrustRequirement, VerifyJson, }, session_manager::CollectStrategy, store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore, RoomSettings}, @@ -5032,4 +5032,131 @@ pub(crate) mod tests { .unwrap(); assert_matches!(thread_encryption_result, UnsignedDecryptionResult::Decrypted(_)); } + + #[async_test] + async fn test_decryption_trust_requirement() { + let (alice, bob) = + get_machine_pair_with_setup_sessions_test_helper(alice_id(), user_id(), false).await; + let room_id = room_id!("!test:example.org"); + + let to_device_requests = alice + .share_room_key(room_id, iter::once(bob.user_id()), EncryptionSettings::default()) + .await + .unwrap(); + + let event = ToDeviceEvent::new( + alice.user_id().to_owned(), + to_device_requests_to_content(to_device_requests), + ); + + let group_session = bob + .store() + .with_transaction(|mut tr| async { + let res = + bob.decrypt_to_device_event(&mut tr, &event, &mut Changes::default()).await?; + Ok((tr, res)) + }) + .await + .unwrap() + .inbound_group_session + .unwrap(); + bob.store().save_inbound_group_sessions(&[group_session.clone()]).await.unwrap(); + + let plaintext = "It is a secret to everybody"; + + let content = RoomMessageEventContent::text_plain(plaintext); + + let encrypted_content = alice + .encrypt_room_event(room_id, AnyMessageLikeEventContent::RoomMessage(content.clone())) + .await + .unwrap(); + + let event = json!({ + "event_id": "$xxxxx:example.org", + "origin_server_ts": MilliSecondsSinceUnixEpoch::now(), + "sender": alice.user_id(), + "type": "m.room.encrypted", + "content": encrypted_content, + }); + let event = json_convert(&event).unwrap(); + + // Set the sender data to various values, and test that we can or can't + // decrypt, depending on what the trust requirement is. + + // An unknown non-legacy session should be decryptable only when the trust + // requirement allows untrusted sessions + let bob_store = bob.store(); + let mut session = bob_store + .get_inbound_group_session(&room_id, group_session.session_id()) + .await + .unwrap() + .unwrap(); + session.sender_data = SenderData::UnknownDevice { + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: false, + }; + bob_store.save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + + // An unknown legacy session should be decryptable only when the trust + // requirement allows untrusted or legacy sessions + session.sender_data = SenderData::UnknownDevice { + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: true, + }; + bob_store.save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + + // A session where we have the device keys but no cross-signing + // information should be just like an unknown device + session.sender_data = SenderData::DeviceInfo { + device_keys: alice + .get_device(alice.user_id(), alice.device_id(), None) + .await + .unwrap() + .unwrap() + .as_device_keys() + .clone(), + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: false, + }; + bob_store.save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + } } diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index 42b2dd97b51..c724bb625c2 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -71,6 +71,7 @@ pub enum TrustRequirement { /// Settings for decrypting messages #[derive(Clone, Debug, Deserialize, Serialize)] pub struct DecryptionSettings { + /// The trust level required to decrypt the event pub trust_requirement: TrustRequirement, } From 67af31a2b9be8b73ecc294dd91d86ac5c53a2ce8 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 2 Jul 2024 09:33:18 +0200 Subject: [PATCH 04/38] feature(crypto): Add support for master key local pinning --- .../src/identities/manager.rs | 128 ++++++- .../matrix-sdk-crypto/src/identities/user.rs | 343 +++++++++++++----- .../src/test_json/keys_query_sets.rs | 216 ++++++++++- 3 files changed, 602 insertions(+), 85 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index 1fd7de79e9d..ff543f57e94 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -512,6 +512,7 @@ impl IdentityManager { *changed_private_identity = self.check_private_identity(&identity).await; Ok(identity.into()) } else { + // First time seen, create the identity. The current MSK will be pinned. let identity = OtherUserIdentityData::new(master_key, self_signing)?; Ok(identity.into()) } @@ -1338,7 +1339,7 @@ pub(crate) mod tests { use std::ops::Deref; use futures_util::pin_mut; - use matrix_sdk_test::{async_test, response_from_file}; + use matrix_sdk_test::{async_test, response_from_file, test_json}; use ruma::{ api::{client::keys::get_keys::v3::Response as KeysQueryResponse, IncomingResponse}, device_id, user_id, TransactionId, @@ -1897,4 +1898,129 @@ pub(crate) mod tests { manager.store.get_device_data(other_user, device_id!("OBEBOSKTBE")).await.unwrap().unwrap(); } + + #[async_test] + async fn test_manager_identity_updates() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let manager = manager_test_helper(user_id(), device_id()).await; + let other_user = DataSet::user_id(); + let devices = manager.store.get_user_devices(other_user).await.unwrap(); + assert_eq!(devices.devices().count(), 0); + + let identity = manager.store.get_user_identity(other_user).await.unwrap(); + assert!(identity.is_none()); + + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_a(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // There should be now an identity and no pin violation (pinned msk is the + // current one) + assert!(!other_identity.has_pin_violation()); + let first_device = manager + .store + .get_device_data(other_user, DataSet::first_device_id()) + .await + .unwrap() + .unwrap(); + assert!(first_device.is_cross_signed_by_owner(&identity)); + + // We receive a new keys update for that user, with a new identity + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_b(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // The previous known identity has been replaced, there should be a pin + // violation + assert!(other_identity.has_pin_violation()); + + let second_device = manager + .store + .get_device_data(other_user, DataSet::second_device_id()) + .await + .unwrap() + .unwrap(); + + // There is a new device signed by the new identity + assert!(second_device.is_cross_signed_by_owner(&identity)); + + // The first device should not be signed by the new identity + let first_device = manager + .store + .get_device_data(other_user, DataSet::first_device_id()) + .await + .unwrap() + .unwrap(); + assert!(!first_device.is_cross_signed_by_owner(&identity)); + + let remember_previous_identity = other_identity.clone(); + // We receive a new keys update for that user, with no identity anymore + // Notice that there is no server API to delete identity, but we want to test + // here that a home server cannot clear the identity and serve a new one + // after that would get automatically approved. + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_no_identity(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + assert_eq!(other_identity, &remember_previous_identity); + assert!(other_identity.has_pin_violation()); + } + + #[async_test] + async fn test_manager_resolve_identity_mismatch() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let manager = manager_test_helper(user_id(), device_id()).await; + let other_user = DataSet::user_id(); + + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_a(), + ) + .await + .unwrap(); + + // We receive a new keys update for that user, with a new identity + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_b(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // We have a new identity now, so there should be a pin violation + assert!(other_identity.has_pin_violation()); + + // Resolve the misatch by pinning the new identity + other_identity.pin(); + + assert!(!other_identity.has_pin_violation()); + } } diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 93b7f4bcffc..9229094cf51 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -17,7 +17,7 @@ use std::{ ops::Deref, sync::{ atomic::{AtomicBool, Ordering}, - Arc, + Arc, RwLock, }, }; @@ -30,6 +30,7 @@ use ruma::{ DeviceId, EventId, OwnedDeviceId, OwnedUserId, RoomId, UserId, }; use serde::{Deserialize, Serialize}; +use serde_json::Value; use tracing::error; use super::{atomic_bool_deserializer, atomic_bool_serializer}; @@ -295,6 +296,36 @@ impl UserIdentity { methods, ) } + + /// Pin the current identity (Master public key). + pub async fn pin_current_master_key(&self) -> Result<(), CryptoStoreError> { + self.inner.pin(); + let to_save = UserIdentityData::Other(self.inner.clone()); + let changes = Changes { + identities: IdentityChanges { changed: vec![to_save], ..Default::default() }, + ..Default::default() + }; + self.verification_machine.store.inner().save_changes(changes).await?; + Ok(()) + } + + /// An identity mismatch is detected when there is a trust problem with the + /// user identity. There is an identity mismatch if the current identity + /// is not verified and there is a pinning violation. An identity + /// mismatch must be reported to the user, and can be resolved by: + /// - Verifying the new identity (see + /// [`UserIdentity::request_verification`]) + /// - Or by updating the pinned key + /// ([`UserIdentity::pin_current_master_key`]). + pub fn has_identity_mismatch(&self) -> bool { + // First check if the current identity is verified. + if self.is_verified() { + return false; + } + // If not we can check the pinned identity. Verification always have + // higher priority than pinning. + self.inner.has_pin_violation() + } } /// Enum over the different user identity types we can have. @@ -377,10 +408,87 @@ impl UserIdentityData { /// only contain a master key and a self signing key, meaning that only device /// signatures can be checked with this identity. #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, self_signing_key: Arc, + /// The first time a cryptographic identity is seen for a given user, it + /// will be associated to that user (i.e pinned). Future interaction + /// will expect this user crypto identity to stay the same, + /// this will help prevent some MITM attacks. + /// In case of identity change, it will be possible to pin the new identity + /// is the user wants. + pinned_msk: Arc>, +} + +/// Intermediate struct to help serialize ReadOnlyUserIdentity and support +/// versioning and migration. +/// Version v1 is adding support for identity pinning (`pinned_msk`), as part +/// of migration we just pin the currently known msk. +#[derive(Deserialize, Serialize)] +struct ReadOnlyUserIdentitySerializer { + version: Option, + #[serde(flatten)] + other: Value, +} + +#[derive(Debug, Deserialize, Serialize)] +struct ReadOnlyUserIdentityV0 { + user_id: OwnedUserId, + master_key: MasterPubkey, + self_signing_key: SelfSigningPubkey, +} + +#[derive(Debug, Deserialize, Serialize)] +struct ReadOnlyUserIdentityV1 { + user_id: OwnedUserId, + master_key: MasterPubkey, + self_signing_key: SelfSigningPubkey, + pinned_msk: MasterPubkey, +} + +impl From for OtherUserIdentityData { + fn from(value: ReadOnlyUserIdentitySerializer) -> Self { + match value.version { + None => { + // Old format, migrate the pinned identity + let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other).unwrap(); + OtherUserIdentityData { + user_id: v0.user_id, + master_key: Arc::new(v0.master_key.clone()), + self_signing_key: Arc::new(v0.self_signing_key), + // We migrate by pinning the current msk + pinned_msk: Arc::new(RwLock::new(v0.master_key.clone())), + } + } + _ => { + // v1 format + let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other).unwrap(); + OtherUserIdentityData { + user_id: v1.user_id, + master_key: Arc::new(v1.master_key.clone()), + self_signing_key: Arc::new(v1.self_signing_key), + pinned_msk: Arc::new(RwLock::new(v1.pinned_msk)), + } + } + } + } +} + +impl From for ReadOnlyUserIdentitySerializer { + fn from(value: OtherUserIdentityData) -> Self { + let v1 = ReadOnlyUserIdentityV1 { + user_id: value.user_id.clone(), + master_key: value.master_key().to_owned(), + self_signing_key: value.self_signing_key().to_owned(), + pinned_msk: value.pinned_msk.read().unwrap().clone(), + }; + ReadOnlyUserIdentitySerializer { + version: Some("1".to_owned()), + other: serde_json::to_value(v1).unwrap(), + } + } } impl PartialEq for OtherUserIdentityData { @@ -423,19 +531,24 @@ impl OtherUserIdentityData { Ok(Self { user_id: master_key.user_id().into(), - master_key: master_key.into(), + master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), + pinned_msk: RwLock::new(master_key).into(), }) } #[cfg(test)] pub(crate) async fn from_private(identity: &crate::olm::PrivateCrossSigningIdentity) -> Self { - let master_key = - identity.master_key.lock().await.as_ref().unwrap().public_key().clone().into(); + let master_key = identity.master_key.lock().await.as_ref().unwrap().public_key().clone(); let self_signing_key = identity.self_signing_key.lock().await.as_ref().unwrap().public_key().clone().into(); - Self { user_id: identity.user_id().into(), master_key, self_signing_key } + Self { + user_id: identity.user_id().into(), + master_key: Arc::new(master_key.clone()), + self_signing_key, + pinned_msk: Arc::new(RwLock::new(master_key.clone())), + } } /// Get the user id of this identity. @@ -453,6 +566,24 @@ impl OtherUserIdentityData { &self.self_signing_key } + /// Pin the current identity + pub(crate) fn pin(&self) { + let mut m = self.pinned_msk.write().unwrap(); + *m = self.master_key.as_ref().clone() + } + + /// Key pinning acts as a trust on first use mechanism, the first time an + /// identity is known for a user it will be pinned. + /// For future interaction with a user, the identity is expected to be the + /// one that was pinned. In case of identity change the UI client should + /// receive reports of pinning violation and decide to act accordingly; + /// that is accept and pin the new identity, perform a verification or + /// stop communications. + pub(crate) fn has_pin_violation(&self) -> bool { + let pinned_msk = self.pinned_msk.read().unwrap(); + pinned_msk.get_first_key() != self.master_key().get_first_key() + } + /// Update the identity with a new master key and self signing key. /// /// # Arguments @@ -471,7 +602,15 @@ impl OtherUserIdentityData { ) -> Result { master_key.verify_subkey(&self_signing_key)?; - let new = Self::new(master_key, self_signing_key)?; + // The pin is maintained. + let pinned_msk = self.pinned_msk.read().unwrap().clone(); + + let new = Self { + user_id: master_key.user_id().into(), + master_key: master_key.clone().into(), + self_signing_key: self_signing_key.into(), + pinned_msk: RwLock::new(pinned_msk).into(), + }; let changed = new != *self; *self = new; @@ -792,8 +931,11 @@ pub(crate) mod tests { use std::{collections::HashMap, sync::Arc}; use assert_matches::assert_matches; - use matrix_sdk_test::async_test; - use ruma::{device_id, user_id, UserId}; + use matrix_sdk_test::{async_test, response_from_file, test_json}; + use ruma::{ + api::{client::keys::get_keys::v3::Response as KeyQueryResponse, IncomingResponse}, + device_id, user_id, TransactionId, + }; use serde_json::{json, Value}; use tokio::sync::Mutex; @@ -802,16 +944,16 @@ pub(crate) mod tests { OwnUserIdentityData, UserIdentityData, }; use crate::{ - identities::{manager::testing::own_key_query, Device}, - machine::tests::{ - get_machine_pair, mark_alice_identity_as_verified_test_helper, - setup_cross_signing_for_machine_test_helper, + identities::{ + manager::testing::own_key_query, + user::{ReadOnlyUserIdentitySerializer, ReadOnlyUserIdentityV1}, + Device, }, olm::{Account, PrivateCrossSigningIdentity}, - store::{Changes, CryptoStoreWrapper, MemoryStore}, + store::{CryptoStoreWrapper, MemoryStore}, types::{CrossSigningKey, MasterPubkey, SelfSigningPubkey, Signatures, UserSigningPubkey}, verification::VerificationMachine, - OlmMachine, + OlmMachine, OtherUserIdentityData, }; #[test] @@ -872,6 +1014,56 @@ pub(crate) mod tests { get_other_identity(); } + #[test] + fn deserialization_migration_test() { + let serialized_value = json!({ + "user_id":"@example2:localhost", + "master_key":{ + "user_id":"@example2:localhost", + "usage":[ + "master" + ], + "keys":{ + "ed25519:kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do":"kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do" + }, + "signatures":{ + "@example2:localhost":{ + "ed25519:SKISMLNIMH":"KdUZqzt8VScGNtufuQ8lOf25byYLWIhmUYpPENdmM8nsldexD7vj+Sxoo7PknnTX/BL9h2N7uBq0JuykjunCAw" + } + } + }, + "self_signing_key":{ + "user_id":"@example2:localhost", + "usage":[ + "self_signing" + ], + "keys":{ + "ed25519:ZtFrSkJ1qB8Jph/ql9Eo/lKpIYCzwvKAKXfkaS4XZNc":"ZtFrSkJ1qB8Jph/ql9Eo/lKpIYCzwvKAKXfkaS4XZNc" + }, + "signatures":{ + "@example2:localhost":{ + "ed25519:kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do":"W/O8BnmiUETPpH02mwYaBgvvgF/atXnusmpSTJZeUSH/vHg66xiZOhveQDG4cwaW8iMa+t9N4h1DWnRoHB4mCQ" + } + } + } + }); + let migrated: OtherUserIdentityData = serde_json::from_value(serialized_value).unwrap(); + + let pinned_msk = migrated.pinned_msk.read().unwrap(); + assert_eq!(*pinned_msk, migrated.master_key().clone()); + + // Serialize back + let value = serde_json::to_value(migrated.clone()).unwrap(); + + // Should be serialized with latest version + let _: ReadOnlyUserIdentityV1 = + serde_json::from_value(value.clone()).expect("Should deserialize as version 1"); + + let with_serializer: ReadOnlyUserIdentitySerializer = + serde_json::from_value(value).unwrap(); + assert_eq!("1", with_serializer.version.unwrap()); + } + #[test] fn own_identity_check_signatures() { let response = own_key_query(); @@ -1027,86 +1219,73 @@ pub(crate) mod tests { ); } - async fn get_machine_pair_with_signed_identities( - alice: &UserId, - bob: &UserId, - ) -> (OlmMachine, OlmMachine) { - let (alice, bob, _) = get_machine_pair(alice, bob, false).await; - setup_cross_signing_for_machine_test_helper(&alice, &bob).await; - mark_alice_identity_as_verified_test_helper(&alice, &bob).await; + #[async_test] + async fn resolve_identity_mismacth_with_verification() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; - (alice, bob) - } + let my_user_id = user_id!("@me:localhost"); + let machine = OlmMachine::new(my_user_id, device_id!("ABCDEFGH")).await; + machine.bootstrap_cross_signing(false).await.unwrap(); - #[async_test] - async fn test_other_user_is_verified_if_my_identity_is_verified_and_they_are_cross_signed() { - let alice_user_id = user_id!("@alice:localhost"); - let bob_user_id = user_id!("@bob:localhost"); - let (alice, bob) = - get_machine_pair_with_signed_identities(alice_user_id, bob_user_id).await; + let my_id = machine.get_identity(my_user_id, None).await.unwrap().unwrap().own().unwrap(); + let usk_key_id = my_id.inner.user_signing_key().keys().iter().next().unwrap().0; - let bobs_own_identity = - bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); - let bobs_alice_identity = - bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + println!("USK ID: {}", usk_key_id); + let keys_query = DataSet::key_query_with_identity_a(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); - assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); - assert!(bobs_alice_identity.is_verified(), "Alice's identity should be verified as well."); - } + // Simulate an identity hange + let keys_query = DataSet::key_query_with_identity_b(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); - #[async_test] - async fn test_other_user_is_not_verified_if_they_are_not_cross_signed() { - let alice_user_id = user_id!("@alice:localhost"); - let bob_user_id = user_id!("@bob:localhost"); - let (alice, bob, _) = get_machine_pair(alice_user_id, bob_user_id, false).await; - setup_cross_signing_for_machine_test_helper(&alice, &bob).await; - - let bobs_own_identity = - bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); - let bobs_alice_identity = - bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); - - assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); - assert!( - !bobs_alice_identity.is_verified(), - "Alice's identity should not be considered verified since Bob has not signed it." - ); - } + let other_user_id = DataSet::user_id(); - #[async_test] - async fn test_other_user_is_not_verified_if_my_identity_is_not_verified() { - let alice_user_id = user_id!("@alice:localhost"); - let bob_user_id = user_id!("@bob:localhost"); + let other_identity = + machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); - let (alice, bob, _) = get_machine_pair(alice_user_id, bob_user_id, false).await; - setup_cross_signing_for_machine_test_helper(&alice, &bob).await; - mark_alice_identity_as_verified_test_helper(&alice, &bob).await; + // There should be an identity mismatch + assert!(other_identity.has_identity_mismatch()); - let bobs_own_identity = - bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); - let bobs_alice_identity = - bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + // Manually verify for the purpose of this test + let sig_upload = other_identity.verify().await.unwrap(); - assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); - assert!(bobs_alice_identity.is_verified(), "Alice's identity should be verified as well."); + let raw_extracted = + sig_upload.signed_keys.get(other_user_id).unwrap().iter().next().unwrap().1.get(); - bobs_own_identity.mark_as_unverified(); + let new_signature: CrossSigningKey = serde_json::from_str(raw_extracted).unwrap(); - bob.store() - .save_changes(Changes { - identities: crate::store::IdentityChanges { - changed: vec![bobs_own_identity.inner.clone().into()], - ..Default::default() - }, - ..Default::default() - }) - .await - .unwrap(); + let mut msk_to_update: CrossSigningKey = + serde_json::from_value(DataSet::msk_b().get("@bob:localhost").unwrap().clone()) + .unwrap(); - assert!(!bobs_own_identity.is_verified(), "Bob's identity should not be verified anymore."); - assert!( - !bobs_alice_identity.is_verified(), - "Alice's identity should not be verified either." + msk_to_update.signatures.add_signature( + my_user_id.to_owned(), + usk_key_id.to_owned(), + new_signature.signatures.get_signature(my_user_id, usk_key_id).unwrap(), ); + + // we want to update bob device keys with the new signature + let data = json!({ + "device_keys": {}, // For the purpose of this test we don't need devices here + "failures": {}, + "master_keys": { + DataSet::user_id(): msk_to_update + , + }, + "self_signing_keys": DataSet::ssk_b(), + }); + + let kq_response = KeyQueryResponse::try_from_http_response(response_from_file(&data)) + .expect("Can't parse the `/keys/upload` response"); + machine.mark_request_as_sent(&TransactionId::new(), &kq_response).await.unwrap(); + + // There should not be an identity mismatch anymore + let other_identity = + machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); + assert!(!other_identity.has_identity_mismatch()); + // But there is still a pin violation + assert!(other_identity.inner.has_pin_violation()); } } diff --git a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs index c4e06e190fc..755369902bf 100644 --- a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs +++ b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs @@ -2,7 +2,7 @@ use ruma::{ api::{client::keys::get_keys::v3::Response as KeyQueryResponse, IncomingResponse}, device_id, user_id, DeviceId, UserId, }; -use serde_json::json; +use serde_json::{json, Value}; use crate::response_from_file; @@ -105,7 +105,7 @@ impl KeyDistributionTestData { /// but not the other one `FRGNMZVOKA`. /// `@dan` identity is signed by `@me` identity (alice trust dan) pub fn dan_keys_query_response() -> KeyQueryResponse { - let data: serde_json::Value = json!({ + let data: Value = json!({ "device_keys": { "@dan:localhost": { "JHPUERYQUW": { @@ -457,3 +457,215 @@ impl KeyDistributionTestData { user_id!("@good:localhost") } } + +/// A set of keys query to test identity changes, +/// For user @bob, several payloads with no identities then identity A and B. +pub struct IdentityChangeDataSet {} + +#[allow(dead_code)] +impl IdentityChangeDataSet { + pub fn user_id() -> &'static UserId { + user_id!("@bob:localhost") + } + + pub fn first_device_id() -> &'static DeviceId { + device_id!("GYKSNAWLVK") + } + + pub fn second_device_id() -> &'static DeviceId { + device_id!("ATWKQFSFRN") + } + + pub fn third_device_id() -> &'static DeviceId { + device_id!("OPABMDDXGX") + } + + fn device_keys_payload_1_signed_by_a() -> Value { + json!({ + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "GYKSNAWLVK", + "keys": { + "curve25519:GYKSNAWLVK": "dBcZBzQaiQYWf6rBPh2QypIOB/dxSoTeyaFaxNNbeHs", + "ed25519:GYKSNAWLVK": "6melQNnhoI9sT2b4VzNPAwa8aB179ym45fON8Yo7kVk" + }, + "signatures": { + "@bob:localhost": { + "ed25519:GYKSNAWLVK": "Fk45zHAbrd+1j9wZXLjL2Y/+DU/Mnz9yuvlfYBOOT7qExN2Jdud+5BAuNs8nZ/caS4wTF39Kg3zQpzaGERoCBg", + "ed25519:dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw": "md0Pa1MYlneFb1fp6KCsvZpi2ySb6/G+ULoCbQDWBeDxNEcoNMzf7PEKY04UToCZKUU4LifvRWmiWFDanOlkCQ" + } + }, + "user_id": "@bob:localhost", + }) + } + + fn msk_a() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY" + }, + "signatures": { + "@bob:localhost": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "6vGDbPO5XzlcwbU3aV+kcck+iHHEBtX85ow2gW5U05/DZdtda/JNVa5Nn7B9lQHNnnrMqt1sX00y/JrIkSS1Aw", + "ed25519:GYKSNAWLVK": "jLxmUPr0Ny2Ai9+NGKGhed9BAuKikOc7r6gr7MQVawePYS95w8NJ8Tzaq9zFFOmIiojACNdQ/ksy3QAdwD6vBQ" + } + }, + "usage": [ + "master" + ], + "user_id": "@bob:localhost" + } + }) + } + fn ssk_a() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw": "dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw" + }, + "signatures": { + "@bob:localhost": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "7md6mwjUK8zjintmffJ0+kImC59/Y8PdySy99EZz5Neu+VMX3LT7txhKO2gC/hmDduRw+JGfGXIiDxR7GmQqDw" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@bob:localhost" + } + }) + } + /// A key query with an identity (Ia), and a first device `GYKSNAWLVK` + /// signed by Ia. + pub fn key_query_with_identity_a() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a() + } + }, + "failures": {}, + "master_keys": Self::msk_a(), + "self_signing_keys": Self::ssk_a(), + "user_signing_keys": {} + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + pub fn msk_b() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4" + }, + "signatures": { + "@bob:localhost": { + "ed25519:ATWKQFSFRN": "MBOzCKYPQLQMpBY2lFZJ4c8451xJfQCdhPBb1AHlTUSxKFiWi6V+k1oRRnhQein/PjkIY7ZO+HoOrIeOtbRMAw", + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "xqLhC3sIUci1W2CNVW7HZWXreQApgjv2RDwB0WPiMd1P4vbZ/qJM0KWqK2piGPWliPi8YVREMrg216KXM3IhCA" + } + }, + "usage": [ + "master" + ], + "user_id": "@bob:localhost" + } + }) + } + + pub fn ssk_b() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc": "At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc" + }, + "signatures": { + "@bob:localhost": { + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "Ls6CeoA4LoPCHuSwG96kbhd1dEV09TgdMROIZi6vFz/MT9Wtik6joQi/tQ3zCwIZCSR53ksLO4jG1DD31AiBAA" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@bob:localhost" + } + }) + } + + pub fn device_keys_payload_2_signed_by_b() -> Value { + json!({ + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "ATWKQFSFRN", + "keys": { + "curve25519:ATWKQFSFRN": "CY0TWVK1/Kj3ZADuBcGe3UKvpT+IKAPMUsMeJhSDqno", + "ed25519:ATWKQFSFRN": "TyTQqd6j2JlWZh97r+kTYuCbvqnPoNwO6EGovYsjY00" + }, + "signatures": { + "@bob:localhost": { + "ed25519:ATWKQFSFRN": "BQ9Gp0p+6srF+c8OyruqKKd9R4yaub3THYAyyBB/7X/rG8BwcAqFynzl1aGyFYun4Q+087a5OSiglCXI+/kQAA", + "ed25519:At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc": "TWmDPaG7t0rZ6luauonELD3dmBDTIRryqXhgsIQRiGint2rJdic8RVyZ6a61bgu6mtBjfvU3prqMNp6sVi16Cg" + } + }, + "user_id": "@bob:localhost", + }) + } + /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. + /// `ATWKQFSFRN` is signed with the new identity but + pub fn key_query_with_identity_b() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "ATWKQFSFRN": Self::device_keys_payload_2_signed_by_b(), + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a(), + } + }, + "failures": {}, + "master_keys": Self::msk_b(), + "self_signing_keys": Self::ssk_b(), + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. + /// `ATWKQFSFRN` is signed with the new identity but + pub fn key_query_with_identity_no_identity() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "ATWKQFSFRN": Self::device_keys_payload_2_signed_by_b(), + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a(), + "OPABMDDXGX": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "OPABMDDXGX", + "keys": { + "curve25519:OPABMDDXGX": "O6bwa9Op0E+PQPCrbTOfdYwU+j95RRPhXIHuNpe94ns", + "ed25519:OPABMDDXGX": "DvjkSNOM9XrR1gWrr2YSDvTnwnLIgKDMRr5v8HgMKak" + }, + "signatures": { + "@bob:localhost": { + "ed25519:OPABMDDXGX": "o+BBnw/SIJWxSf799Adq6jEl9X3lwCg5MJkS8GlfId+pW3ReEETK0l+9bhCAgBsNSKRtB/fmZQBhjMx4FJr+BA" + } + }, + "user_id": "@bob:localhost", + "unsigned": { + "device_display_name": "develop.element.io: Chrome on macOS" + } + } + } + }, + "failures": {}, + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } +} From 8253618a9b9279b7a366cd1b4e889cd51fa078c3 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 11:23:10 +0200 Subject: [PATCH 05/38] Review: Do not use msk abbreviation --- .../matrix-sdk-crypto/src/identities/user.rs | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 9229094cf51..db8541cc7c8 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -407,25 +407,27 @@ impl UserIdentityData { /// This is the user identity of a user that isn't our own. Other users will /// only contain a master key and a self signing key, meaning that only device /// signatures can be checked with this identity. +/// +/// This struct also contains the currently pinned master key for that user. +/// The first time a cryptographic identity is seen for a given user, it +/// will be associated to that user (i.e. pinned). Future interactions +/// will expect this user crypto identity to stay the same, +/// this will help prevent some MITM attacks. +/// In case of identity change, it will be possible to pin the new identity +/// is the user wants. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, self_signing_key: Arc, - /// The first time a cryptographic identity is seen for a given user, it - /// will be associated to that user (i.e pinned). Future interaction - /// will expect this user crypto identity to stay the same, - /// this will help prevent some MITM attacks. - /// In case of identity change, it will be possible to pin the new identity - /// is the user wants. - pinned_msk: Arc>, + pinned_master_key: Arc>, } /// Intermediate struct to help serialize ReadOnlyUserIdentity and support /// versioning and migration. -/// Version v1 is adding support for identity pinning (`pinned_msk`), as part -/// of migration we just pin the currently known msk. +/// Version v1 is adding support for identity pinning (`pinned_master_key`), as +/// part of migration we just pin the currently known master key. #[derive(Deserialize, Serialize)] struct ReadOnlyUserIdentitySerializer { version: Option, @@ -445,7 +447,7 @@ struct ReadOnlyUserIdentityV1 { user_id: OwnedUserId, master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, - pinned_msk: MasterPubkey, + pinned_master_key: MasterPubkey, } impl From for OtherUserIdentityData { @@ -458,8 +460,8 @@ impl From for OtherUserIdentityData { user_id: v0.user_id, master_key: Arc::new(v0.master_key.clone()), self_signing_key: Arc::new(v0.self_signing_key), - // We migrate by pinning the current msk - pinned_msk: Arc::new(RwLock::new(v0.master_key.clone())), + // We migrate by pinning the current master key + pinned_master_key: Arc::new(RwLock::new(v0.master_key.clone())), } } _ => { @@ -469,7 +471,7 @@ impl From for OtherUserIdentityData { user_id: v1.user_id, master_key: Arc::new(v1.master_key.clone()), self_signing_key: Arc::new(v1.self_signing_key), - pinned_msk: Arc::new(RwLock::new(v1.pinned_msk)), + pinned_master_key: Arc::new(RwLock::new(v1.pinned_master_key)), } } } @@ -482,7 +484,7 @@ impl From for ReadOnlyUserIdentitySerializer { user_id: value.user_id.clone(), master_key: value.master_key().to_owned(), self_signing_key: value.self_signing_key().to_owned(), - pinned_msk: value.pinned_msk.read().unwrap().clone(), + pinned_master_key: value.pinned_master_key.read().unwrap().clone(), }; ReadOnlyUserIdentitySerializer { version: Some("1".to_owned()), @@ -533,7 +535,7 @@ impl OtherUserIdentityData { user_id: master_key.user_id().into(), master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), - pinned_msk: RwLock::new(master_key).into(), + pinned_master_key: RwLock::new(master_key).into(), }) } @@ -547,7 +549,7 @@ impl OtherUserIdentityData { user_id: identity.user_id().into(), master_key: Arc::new(master_key.clone()), self_signing_key, - pinned_msk: Arc::new(RwLock::new(master_key.clone())), + pinned_master_key: Arc::new(RwLock::new(master_key.clone())), } } @@ -568,7 +570,7 @@ impl OtherUserIdentityData { /// Pin the current identity pub(crate) fn pin(&self) { - let mut m = self.pinned_msk.write().unwrap(); + let mut m = self.pinned_master_key.write().unwrap(); *m = self.master_key.as_ref().clone() } @@ -580,8 +582,8 @@ impl OtherUserIdentityData { /// that is accept and pin the new identity, perform a verification or /// stop communications. pub(crate) fn has_pin_violation(&self) -> bool { - let pinned_msk = self.pinned_msk.read().unwrap(); - pinned_msk.get_first_key() != self.master_key().get_first_key() + let pinned_master_key = self.pinned_master_key.read().unwrap(); + pinned_master_key.get_first_key() != self.master_key().get_first_key() } /// Update the identity with a new master key and self signing key. @@ -603,13 +605,13 @@ impl OtherUserIdentityData { master_key.verify_subkey(&self_signing_key)?; // The pin is maintained. - let pinned_msk = self.pinned_msk.read().unwrap().clone(); + let pinned_master_key = self.pinned_master_key.read().unwrap().clone(); let new = Self { user_id: master_key.user_id().into(), master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), - pinned_msk: RwLock::new(pinned_msk).into(), + pinned_master_key: RwLock::new(pinned_master_key).into(), }; let changed = new != *self; @@ -1049,8 +1051,8 @@ pub(crate) mod tests { }); let migrated: OtherUserIdentityData = serde_json::from_value(serialized_value).unwrap(); - let pinned_msk = migrated.pinned_msk.read().unwrap(); - assert_eq!(*pinned_msk, migrated.master_key().clone()); + let pinned_master_key = migrated.pinned_master_key.read().unwrap(); + assert_eq!(*pinned_master_key, migrated.master_key().clone()); // Serialize back let value = serde_json::to_value(migrated.clone()).unwrap(); From b7241c305b2b8423903a7c901c4d5604089a1cc8 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 15:41:55 +0200 Subject: [PATCH 06/38] Review: Use TryFrom instead of From for serializer helper struct --- .../matrix-sdk-crypto/src/identities/user.rs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index db8541cc7c8..27eacee807f 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -416,7 +416,7 @@ impl UserIdentityData { /// In case of identity change, it will be possible to pin the new identity /// is the user wants. #[derive(Debug, Clone, Deserialize, Serialize)] -#[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] +#[serde(try_from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, @@ -450,30 +450,33 @@ struct ReadOnlyUserIdentityV1 { pinned_master_key: MasterPubkey, } -impl From for OtherUserIdentityData { - fn from(value: ReadOnlyUserIdentitySerializer) -> Self { +impl TryFrom for OtherUserIdentityData { + type Error = serde_json::Error; + fn try_from( + value: ReadOnlyUserIdentitySerializer, + ) -> Result { match value.version { None => { // Old format, migrate the pinned identity - let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other).unwrap(); - OtherUserIdentityData { + let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other)?; + Ok(OtherUserIdentityData { user_id: v0.user_id, master_key: Arc::new(v0.master_key.clone()), self_signing_key: Arc::new(v0.self_signing_key), // We migrate by pinning the current master key pinned_master_key: Arc::new(RwLock::new(v0.master_key.clone())), - } + }) } - _ => { - // v1 format - let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other).unwrap(); - OtherUserIdentityData { + Some(v) if v == "1" => { + let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other)?; + Ok(OtherUserIdentityData { user_id: v1.user_id, master_key: Arc::new(v1.master_key.clone()), self_signing_key: Arc::new(v1.self_signing_key), pinned_master_key: Arc::new(RwLock::new(v1.pinned_master_key)), - } + }) } + _ => Err(serde::de::Error::custom(format!("Unsupported Version {:?}", value.version))), } } } From 4d668280ab0593165f1b9d5a3038cd4f14e1a798 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 15:55:13 +0200 Subject: [PATCH 07/38] Review: Fix typo --- crates/matrix-sdk-crypto/src/identities/user.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 27eacee807f..d2f03329018 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -1225,7 +1225,7 @@ pub(crate) mod tests { } #[async_test] - async fn resolve_identity_mismacth_with_verification() { + async fn resolve_identity_mismatch_with_verification() { use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; let my_user_id = user_id!("@me:localhost"); From c045d26d0693f363c90085dce50099cca3571dbb Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 15:55:50 +0200 Subject: [PATCH 08/38] Review: Improve doc --- crates/matrix-sdk-crypto/src/identities/user.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index d2f03329018..ebfdd250f05 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -309,14 +309,17 @@ impl UserIdentity { Ok(()) } + /// Returns true if the identity is not verified and did change since the + /// last time we pinned it. + /// /// An identity mismatch is detected when there is a trust problem with the /// user identity. There is an identity mismatch if the current identity /// is not verified and there is a pinning violation. An identity /// mismatch must be reported to the user, and can be resolved by: /// - Verifying the new identity (see /// [`UserIdentity::request_verification`]) - /// - Or by updating the pinned key - /// ([`UserIdentity::pin_current_master_key`]). + /// - Or by updating the pinned key (see + /// [`UserIdentity::pin_current_master_key`]). pub fn has_identity_mismatch(&self) -> bool { // First check if the current identity is verified. if self.is_verified() { @@ -577,6 +580,8 @@ impl OtherUserIdentityData { *m = self.master_key.as_ref().clone() } + /// Returns true if the identity has changed since we last pinned it. + /// /// Key pinning acts as a trust on first use mechanism, the first time an /// identity is known for a user it will be pinned. /// For future interaction with a user, the identity is expected to be the From bfb88183ccb678b85cc3423c8f89118911268934 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 16:15:53 +0200 Subject: [PATCH 09/38] Review: Fix data set comments --- .../matrix-sdk-test/src/test_json/keys_query_sets.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs index 755369902bf..bf5c1a3321e 100644 --- a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs +++ b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs @@ -616,7 +616,8 @@ impl IdentityChangeDataSet { }) } /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. - /// `ATWKQFSFRN` is signed with the new identity but + /// `ATWKQFSFRN` is signed with the new identity but `GYKSNAWLVK` is still + /// signed by the old identity (Ia). pub fn key_query_with_identity_b() -> KeyQueryResponse { let data = response_from_file(&json!({ "device_keys": { @@ -633,8 +634,8 @@ impl IdentityChangeDataSet { .expect("Can't parse the `/keys/upload` response") } - /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. - /// `ATWKQFSFRN` is signed with the new identity but + /// A key query with no identity and a new device `OPABMDDXGX` (not + /// cross-signed). pub fn key_query_with_identity_no_identity() -> KeyQueryResponse { let data = response_from_file(&json!({ "device_keys": { @@ -657,9 +658,6 @@ impl IdentityChangeDataSet { } }, "user_id": "@bob:localhost", - "unsigned": { - "device_display_name": "develop.element.io: Chrome on macOS" - } } } }, From fb39a9ec82eb230f8a68d3d72bb0985a143d1c0b Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 16:29:54 +0200 Subject: [PATCH 10/38] Review: Improve comments --- crates/matrix-sdk-crypto/src/identities/manager.rs | 2 +- crates/matrix-sdk-crypto/src/identities/user.rs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index ff543f57e94..61445a03c1e 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -1969,7 +1969,7 @@ pub(crate) mod tests { assert!(!first_device.is_cross_signed_by_owner(&identity)); let remember_previous_identity = other_identity.clone(); - // We receive a new keys update for that user, with no identity anymore + // We receive updated keys for that user, with no identity anymore. // Notice that there is no server API to delete identity, but we want to test // here that a home server cannot clear the identity and serve a new one // after that would get automatically approved. diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index ebfdd250f05..997e0b70df4 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -612,7 +612,10 @@ impl OtherUserIdentityData { ) -> Result { master_key.verify_subkey(&self_signing_key)?; - // The pin is maintained. + // We update the identity with the new master and self signing key, but we keep + // the previous pinned master key. + // This identity will have a pin violation until the new master key is pinned + // (see [`has_pin_violation`]). let pinned_master_key = self.pinned_master_key.read().unwrap().clone(); let new = Self { From 1d4cefbe90b9c671ae4d7b6166c1c52e204d216d Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 22 Jul 2024 17:29:06 +0200 Subject: [PATCH 11/38] Clippy | unneeded clone --- crates/matrix-sdk-crypto/src/identities/user.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 997e0b70df4..1e280c60af2 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -467,7 +467,7 @@ impl TryFrom for OtherUserIdentityData { master_key: Arc::new(v0.master_key.clone()), self_signing_key: Arc::new(v0.self_signing_key), // We migrate by pinning the current master key - pinned_master_key: Arc::new(RwLock::new(v0.master_key.clone())), + pinned_master_key: Arc::new(RwLock::new(v0.master_key)), }) } Some(v) if v == "1" => { From de60e6ed647a2fac4a3e604b14412f1f1111b877 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 22 Jul 2024 17:35:12 +0200 Subject: [PATCH 12/38] refactor: Rename serializer helper to match new OtherUserIdentityData --- .../matrix-sdk-crypto/src/identities/user.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 1e280c60af2..8de38656ea1 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -419,7 +419,7 @@ impl UserIdentityData { /// In case of identity change, it will be possible to pin the new identity /// is the user wants. #[derive(Debug, Clone, Deserialize, Serialize)] -#[serde(try_from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] +#[serde(try_from = "OtherUserIdentityDataSerializer", into = "OtherUserIdentityDataSerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, @@ -427,41 +427,41 @@ pub struct OtherUserIdentityData { pinned_master_key: Arc>, } -/// Intermediate struct to help serialize ReadOnlyUserIdentity and support +/// Intermediate struct to help serialize OtherUserIdentityData and support /// versioning and migration. /// Version v1 is adding support for identity pinning (`pinned_master_key`), as /// part of migration we just pin the currently known master key. #[derive(Deserialize, Serialize)] -struct ReadOnlyUserIdentitySerializer { +struct OtherUserIdentityDataSerializer { version: Option, #[serde(flatten)] other: Value, } #[derive(Debug, Deserialize, Serialize)] -struct ReadOnlyUserIdentityV0 { +struct OtherUserIdentityDataSerializerV0 { user_id: OwnedUserId, master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, } #[derive(Debug, Deserialize, Serialize)] -struct ReadOnlyUserIdentityV1 { +struct OtherUserIdentityDataSerializerV1 { user_id: OwnedUserId, master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, pinned_master_key: MasterPubkey, } -impl TryFrom for OtherUserIdentityData { +impl TryFrom for OtherUserIdentityData { type Error = serde_json::Error; fn try_from( - value: ReadOnlyUserIdentitySerializer, + value: OtherUserIdentityDataSerializer, ) -> Result { match value.version { None => { // Old format, migrate the pinned identity - let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other)?; + let v0: OtherUserIdentityDataSerializerV0 = serde_json::from_value(value.other)?; Ok(OtherUserIdentityData { user_id: v0.user_id, master_key: Arc::new(v0.master_key.clone()), @@ -471,7 +471,7 @@ impl TryFrom for OtherUserIdentityData { }) } Some(v) if v == "1" => { - let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other)?; + let v1: OtherUserIdentityDataSerializerV1 = serde_json::from_value(value.other)?; Ok(OtherUserIdentityData { user_id: v1.user_id, master_key: Arc::new(v1.master_key.clone()), @@ -484,15 +484,15 @@ impl TryFrom for OtherUserIdentityData { } } -impl From for ReadOnlyUserIdentitySerializer { +impl From for OtherUserIdentityDataSerializer { fn from(value: OtherUserIdentityData) -> Self { - let v1 = ReadOnlyUserIdentityV1 { + let v1 = OtherUserIdentityDataSerializerV1 { user_id: value.user_id.clone(), master_key: value.master_key().to_owned(), self_signing_key: value.self_signing_key().to_owned(), pinned_master_key: value.pinned_master_key.read().unwrap().clone(), }; - ReadOnlyUserIdentitySerializer { + OtherUserIdentityDataSerializer { version: Some("1".to_owned()), other: serde_json::to_value(v1).unwrap(), } @@ -959,7 +959,7 @@ pub(crate) mod tests { use crate::{ identities::{ manager::testing::own_key_query, - user::{ReadOnlyUserIdentitySerializer, ReadOnlyUserIdentityV1}, + user::{OtherUserIdentityDataSerializer, OtherUserIdentityDataSerializerV1}, Device, }, olm::{Account, PrivateCrossSigningIdentity}, @@ -1069,10 +1069,10 @@ pub(crate) mod tests { let value = serde_json::to_value(migrated.clone()).unwrap(); // Should be serialized with latest version - let _: ReadOnlyUserIdentityV1 = + let _: OtherUserIdentityDataSerializerV1 = serde_json::from_value(value.clone()).expect("Should deserialize as version 1"); - let with_serializer: ReadOnlyUserIdentitySerializer = + let with_serializer: OtherUserIdentityDataSerializer = serde_json::from_value(value).unwrap(); assert_eq!("1", with_serializer.version.unwrap()); } From 3731190079f272af80d00e0c8239b3bbaf241ed1 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 22 Jul 2024 16:49:23 -0400 Subject: [PATCH 13/38] implement logic for changing cross-signing keys, and add test --- crates/matrix-sdk-crypto/src/error.rs | 4 + .../matrix-sdk-crypto/src/identities/user.rs | 6 - crates/matrix-sdk-crypto/src/machine.rs | 261 +++++++++++++++--- 3 files changed, 220 insertions(+), 51 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index d7e5ed3625c..f3853b7f016 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -116,6 +116,10 @@ pub enum MegolmError { #[error(transparent)] Store(#[from] CryptoStoreError), + /// The sender's cross-signing identity isn't trusted + #[error("message quarantined because sender's cross-signing identity is not trusted")] + SenderCrossSigningUntrusted, + /// The sender's cross-signing identity has changed, and the change hasn't /// been acknowledged #[error("message quarantined because sender's cross-signing identity has changed")] diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index ad434c9f66a..57adf365ee6 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -32,7 +32,6 @@ use ruma::{ use serde::{Deserialize, Serialize}; use serde_json::Value; use tracing::error; -use vodozemac::Ed25519PublicKey; use super::{atomic_bool_deserializer, atomic_bool_serializer}; use crate::{ @@ -579,11 +578,6 @@ impl ReadOnlyUserIdentity { pinned_msk.get_first_key() != self.master_key().get_first_key() } - pub(crate) fn matches_pinned_identity(&self, msk: &Ed25519PublicKey) -> bool { - let pinned_msk = self.pinned_msk.read().unwrap(); - pinned_msk.get_first_key() == Some(*msk) - } - /// Update the identity with a new master key and self signing key. /// /// # Arguments diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 26797408790..ba979591f84 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1540,6 +1540,44 @@ impl OlmMachine { self.get_encryption_info(&session, &event.sender).await } + async fn check_sender_trusted( + &self, + sender_key: Curve25519PublicKey, + sender: &UserId, + require_trusted: bool, + ) -> MegolmResult<()> { + let Some(device) = self.inner.store.get_device_from_curve_key(sender, sender_key).await? else { + debug!("1"); + return Err(MegolmError::SenderCrossSigningIdentityUnknown) + }; + if !device.is_cross_signed_by_owner() { + debug!("2"); + return Err(MegolmError::SenderCrossSigningIdentityUnknown) + } + if device.is_cross_signing_trusted() { + debug!("3"); + return Ok(()) + } + if sender == self.inner.user_id { + // if we get here, the device (that claims to be ours) wasn't + // cross-signed by us, so we reject it + Err(MegolmError::SenderCrossSigningIdentityUnknown) + } else if require_trusted { + debug!("4"); + Err(MegolmError::SenderCrossSigningUntrusted) + } else { + debug!("5"); + // we don't require the sender to be trusted, but we require that + // the sender's identity matches their pinned identity + let identity = device.device_owner_identity.expect("Cross-signed device must have owner identity"); + if identity.other().expect("Device is not our own device").has_pin_violation() { + Err(MegolmError::SenderCrossSigningIdentityChanged) + } else { + Ok(()) + } + } + } + async fn check_sender_trust_requirement( &self, session: &InboundGroupSession, @@ -1550,59 +1588,22 @@ impl OlmMachine { TrustRequirement::Untrusted => Ok(()), TrustRequirement::CrossSignedOrLegacy => { match session.sender_data { - SenderData::SenderKnown { ref msk, .. } => { - if let Some(identity) = self.inner.store.get_identity(sender).await? { - if sender == self.inner.user_id { - // FIXME: check whether identity matches - Ok(()) - } else { - let identity = identity.other().unwrap(); - if !identity.matches_pinned_identity(msk) { - Err(MegolmError::SenderCrossSigningIdentityChanged) - } else { - Ok(()) - } - } - } else { - // FIXME: is it right that we should be rejecting here? - Err(MegolmError::SenderCrossSigningIdentityUnknown) - } - } + SenderData::SenderKnown { .. } => Ok(()), SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), - _ => Err(MegolmError::SenderCrossSigningIdentityUnknown), + _ => self.check_sender_trusted(session.sender_key(), sender, false).await, } } TrustRequirement::CrossSigned => { match session.sender_data { - SenderData::SenderKnown { ref msk, .. } => { - if let Some(identity) = self.inner.store.get_identity(sender).await? { - if sender == self.inner.user_id { - // FIXME: check whether identity matches - Ok(()) - } else { - let identity = identity.other().unwrap(); - if !identity.matches_pinned_identity(msk) { - Err(MegolmError::SenderCrossSigningIdentityChanged) - } else { - Ok(()) - } - } - } else { - // FIXME: is it right that we should be rejecting here? - Err(MegolmError::SenderCrossSigningIdentityUnknown) - } - } - _ => Err(MegolmError::SenderCrossSigningIdentityUnknown), + SenderData::SenderKnown { .. } => Ok(()), + _ => self.check_sender_trusted(session.sender_key(), sender, false).await, } } TrustRequirement::VerifiedUser => { match session.sender_data { - SenderData::SenderKnown { msk_verified: true, .. } => { - // FIXME: is this all the checking we need to do? - Ok(()) - } - _ => Err(MegolmError::SenderCrossSigningIdentityUnknown), + SenderData::SenderKnown { msk_verified: true, .. } => Ok(()), + _ => self.check_sender_trusted(session.sender_key(), sender, true).await, } } } @@ -2501,20 +2502,21 @@ pub(crate) mod tests { use super::{testing::response_from_file, CrossSigningBootstrapRequests}; use crate::{ error::{EventError, SetRoomSettingsError}, + identities::user::ReadOnlyUserIdentity, machine::{EncryptionSyncChanges, OlmMachine}, olm::{ BackedUpRoomKey, DecryptionSettings, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, SenderData, SenderDataRetryDetails, TrustRequirement, VerifyJson, }, session_manager::CollectStrategy, - store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore, RoomSettings}, + store::{BackupDecryptionKey, Changes, CryptoStore, IdentityChanges, MemoryStore, RoomSettings}, types::{ events::{ room::encrypted::{EncryptedToDeviceEvent, ToDeviceEncryptedEventContent}, room_key_withheld::{RoomKeyWithheldContent, WithheldCode}, ToDeviceEvent, }, - CrossSigningKey, DeviceKeys, EventEncryptionAlgorithm, SignedKey, SigningKeys, + CrossSigningKey, DeviceKeys, EventEncryptionAlgorithm, MasterPubkey, SelfSigningPubkey, SignedKey, SigningKeys, }, utilities::json_convert, verification::tests::{bob_id, outgoing_request_to_event, request_to_event}, @@ -5159,4 +5161,173 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); } + + #[async_test] + async fn test_decryption_trust_with_identity_changes() { + let (alice, bob) = + get_machine_pair_with_setup_sessions_test_helper(alice_id(), user_id(), false).await; + bob.bootstrap_cross_signing(false).await.unwrap(); + let room_id = room_id!("!test:example.org"); + + // Alice encrypts a message for Bob + let to_device_requests = alice + .share_room_key(room_id, iter::once(bob.user_id()), EncryptionSettings::default()) + .await + .unwrap(); + + let event = ToDeviceEvent::new( + alice.user_id().to_owned(), + to_device_requests_to_content(to_device_requests), + ); + + let group_session = bob + .store() + .with_transaction(|mut tr| async { + let res = + bob.decrypt_to_device_event(&mut tr, &event, &mut Changes::default()).await?; + Ok((tr, res)) + }) + .await + .unwrap() + .inbound_group_session + .unwrap(); + bob.store().save_inbound_group_sessions(&[group_session.clone()]).await.unwrap(); + + let plaintext = "It is a secret to everybody"; + + let content = RoomMessageEventContent::text_plain(plaintext); + + let encrypted_content = alice + .encrypt_room_event(room_id, AnyMessageLikeEventContent::RoomMessage(content.clone())) + .await + .unwrap(); + + let event = json!({ + "event_id": "$xxxxx:example.org", + "origin_server_ts": MilliSecondsSinceUnixEpoch::now(), + "sender": alice.user_id(), + "type": "m.room.encrypted", + "content": encrypted_content, + }); + let event = json_convert(&event).unwrap(); + + // Bob receives the message but Alice's keys are unknown at the time of + // reception + let mut session = bob.store() + .get_inbound_group_session(&room_id, group_session.session_id()) + .await + .unwrap() + .unwrap(); + session.sender_data = SenderData::UnknownDevice { + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: false, + }; + bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + + // Bob later gets Alice's device keys and identity + let cross_signing_requests = alice.bootstrap_cross_signing(false).await.unwrap(); + let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; + let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); + let alice_ssk: SelfSigningPubkey = upload_signing_keys_req.self_signing_key.unwrap().try_into().unwrap(); + let upload_keys_req = cross_signing_requests.upload_keys_req.unwrap().clone(); + assert_let!(OutgoingRequests::KeysUpload(device_upload_request) = upload_keys_req.request.as_ref()); + bob.store().save_devices(&[ReadOnlyDevice::try_from(&device_upload_request.device_keys.as_ref().unwrap().deserialize_as::().unwrap()).unwrap()]).await.unwrap(); + bob.store().save_changes(Changes { + identities: IdentityChanges { + new: vec![ReadOnlyUserIdentity::new(alice_msk.clone(), alice_ssk.clone()).unwrap().into()], + .. Default::default() + }, + .. Default::default() + }).await.unwrap(); + + // Since the sending device is now cross-signed by Alice, it should be + // decryptable in all modes except for verified. + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + + // If we verify Alice, the event should be decryptable in verified mode + let mut alice_identity = bob.store().get_identity(alice.user_id()).await.unwrap().unwrap().other().unwrap(); + let signature_upload_req = alice_identity.verify().await.unwrap(); + let alice_read_only_identity = &mut alice_identity.inner; + let mut alice_msk_json = serde_json::to_value(alice_msk).unwrap(); + for (_, value) in signature_upload_req.signed_keys.get(alice.user_id()).unwrap().iter() { + let value: serde_json::Value = serde_json::from_str(value.get()).unwrap(); + alice_msk_json["signatures"].as_object_mut().unwrap().insert(bob.user_id().to_string(), value["signatures"][bob.user_id().as_str()].clone()); + } + let alice_msk: MasterPubkey = serde_json::from_value(alice_msk_json).unwrap(); + alice_read_only_identity.update(alice_msk.clone(), alice_ssk.clone()).unwrap(); + bob.store().save_changes(Changes { + identities: IdentityChanges { + new: vec![alice_read_only_identity.clone().into()], + .. Default::default() + }, + .. Default::default() + }).await.unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + + // If alice's cross-signing key changes, the event should not be decryptable (except for in unverified mode) + let cross_signing_requests = alice.bootstrap_cross_signing(true).await.unwrap(); + let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; + let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); + let alice_ssk: SelfSigningPubkey = upload_signing_keys_req.self_signing_key.unwrap().try_into().unwrap(); + let upload_keys_req = cross_signing_requests.upload_keys_req.unwrap().clone(); + assert_let!(OutgoingRequests::KeysUpload(device_upload_request) = upload_keys_req.request.as_ref()); + bob.store().save_devices(&[ReadOnlyDevice::try_from(&device_upload_request.device_keys.as_ref().unwrap().deserialize_as::().unwrap()).unwrap()]).await.unwrap(); + alice_read_only_identity.update(alice_msk.clone(), alice_ssk.clone()).unwrap(); + bob.store().save_changes(Changes { + identities: IdentityChanges { + new: vec![alice_read_only_identity.clone().into()], + .. Default::default() + }, + .. Default::default() + }).await.unwrap(); + + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + + // ... until we acknowledge the change, at which point it should be + // decryptable in every mode except for verified + alice_read_only_identity.pin(); + bob.store().save_changes(Changes { + identities: IdentityChanges { + new: vec![alice_read_only_identity.clone().into()], + .. Default::default() + }, + .. Default::default() + }).await.unwrap(); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + } } From 78036173beb84ca9bfbd6ef6eeae335e6aa25ca5 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 3 Jul 2024 11:24:11 +0200 Subject: [PATCH 14/38] crypto: refactor extract detection of removed device from outbound --- .../group_sessions/share_strategy.rs | 67 +++++++++++-------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index c2bec06f962..151583ff0b3 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -162,33 +162,7 @@ pub(crate) async fn collect_session_recipients( // of the devices in the session got deleted or blacklisted in the // meantime. If so, we should also rotate the session. if !should_rotate { - // Device IDs that should receive this session - let recipient_device_ids: BTreeSet<&DeviceId> = - recipients.iter().map(|d| d.device_id()).collect(); - - if let Some(shared) = outbound.shared_with_set.read().unwrap().get(user_id) { - // Devices that received this session - let shared: BTreeSet = shared.keys().cloned().collect(); - let shared: BTreeSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); - - // The set difference between - // - // 1. Devices that had previously received the session, and - // 2. Devices that would now receive the session - // - // Represents newly deleted or blacklisted devices. If this - // set is non-empty, we must rotate. - let newly_deleted_or_blacklisted = - shared.difference(&recipient_device_ids).collect::>(); - - should_rotate = !newly_deleted_or_blacklisted.is_empty(); - if should_rotate { - debug!( - "Rotating a room key due to these devices being deleted/blacklisted {:?}", - newly_deleted_or_blacklisted, - ); - } - }; + should_rotate = should_rotate_due_to_left_device(user_id, &recipients, outbound); } devices.entry(user_id.to_owned()).or_default().extend(recipients); @@ -209,6 +183,45 @@ pub(crate) async fn collect_session_recipients( Ok(CollectRecipientsResult { should_rotate, devices, withheld_devices }) } +// Check whether any of the devices currently in the discussion (outbound +// session stores the devices it already got shared with) got deleted or +// blacklisted in the meantime. If it's the case the session should be rotated. +fn should_rotate_due_to_left_device( + user_id: &UserId, + future_recipients: &Vec, + outbound: &OutboundGroupSession, +) -> bool { + // Device IDs that should receive this session + let recipient_device_ids: BTreeSet<&DeviceId> = + future_recipients.iter().map(|d| d.device_id()).collect(); + + if let Some(shared) = outbound.shared_with_set.read().unwrap().get(user_id) { + // Devices that received this session + let shared: BTreeSet = shared.keys().cloned().collect(); + let shared: BTreeSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); + + // The set difference between + // + // 1. Devices that had previously received the session, and + // 2. Devices that would now receive the session + // + // Represents newly deleted or blacklisted devices. If this + // set is non-empty, we must rotate. + let newly_deleted_or_blacklisted = + shared.difference(&recipient_device_ids).collect::>(); + + let should_rotate = !newly_deleted_or_blacklisted.is_empty(); + if should_rotate { + debug!( + "Rotating a room key due to these devices being deleted/blacklisted {:?}", + newly_deleted_or_blacklisted, + ); + } + return should_rotate; + }; + false +} + struct RecipientDevices { allowed_devices: Vec, denied_devices_with_code: Vec<(DeviceData, WithheldCode)>, From 9b00de374b5a4b74dcdb12299a5de7d973c6154b Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 3 Jul 2024 16:31:42 +0200 Subject: [PATCH 15/38] refactor: Move user loop inside strategy specific branch --- .../group_sessions/share_strategy.rs | 83 +++++++++++++------ 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index 151583ff0b3..54fb035036d 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -98,8 +98,6 @@ pub(crate) async fn collect_session_recipients( outbound: &OutboundGroupSession, ) -> OlmResult { let users: BTreeSet<&UserId> = users.collect(); - let mut devices: BTreeMap> = Default::default(); - let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); trace!(?users, ?settings, "Calculating group session recipients"); @@ -125,49 +123,80 @@ pub(crate) async fn collect_session_recipients( // This is calculated in the following code and stored in this variable. let mut should_rotate = user_left || visibility_changed || algorithm_changed; - let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + let mut devices: BTreeMap> = Default::default(); + let mut withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)> = Default::default(); - for user_id in users { - let user_devices = store.get_device_data_for_user_filtered(user_id).await?; + match settings.sharing_strategy { + CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { + let own_identity = + store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + for user_id in users { + let user_devices = store.get_readonly_devices_filtered(user_id).await?; - let recipient_devices = match settings.sharing_strategy { - CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { // We only need the user identity if only_allow_trusted_devices is set. let device_owner_identity = if only_allow_trusted_devices { store.get_user_identity(user_id).await? } else { None }; - split_recipients_withhelds_for_user( + let recipient_devices = split_recipients_withhelds_for_user( user_devices, &own_identity, &device_owner_identity, only_allow_trusted_devices, - ) + ); + + // If we haven't already concluded that the session should be + // rotated for other reasons, we also need to check whether any + // of the devices in the session got deleted or blacklisted in the + // meantime. If so, we should also rotate the session. + if !should_rotate { + should_rotate = should_rotate_due_to_left_device( + user_id, + &recipient_devices.allowed_devices, + outbound, + ); + } + + devices + .entry(user_id.to_owned()) + .or_default() + .extend(recipient_devices.allowed_devices); + withheld_devices.extend(recipient_devices.denied_devices_with_code); } - CollectStrategy::IdentityBasedStrategy => { + } + CollectStrategy::IdentityBasedStrategy => { + let _maybe_own_identity = + store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + + for user_id in users { + let user_devices = store.get_readonly_devices_filtered(user_id).await?; let device_owner_identity = store.get_user_identity(user_id).await?; - split_recipients_withhelds_for_user_based_on_identity( + let recipient_devices = split_recipients_withhelds_for_user_based_on_identity( user_devices, &device_owner_identity, - ) - } - }; - - let recipients = recipient_devices.allowed_devices; - let withheld_recipients = recipient_devices.denied_devices_with_code; + ); + + // If we haven't already concluded that the session should be + // rotated for other reasons, we also need to check whether any + // of the devices in the session got deleted or blacklisted in the + // meantime. If so, we should also rotate the session. + if !should_rotate { + should_rotate = should_rotate_due_to_left_device( + user_id, + &recipient_devices.allowed_devices, + outbound, + ); + } - // If we haven't already concluded that the session should be - // rotated for other reasons, we also need to check whether any - // of the devices in the session got deleted or blacklisted in the - // meantime. If so, we should also rotate the session. - if !should_rotate { - should_rotate = should_rotate_due_to_left_device(user_id, &recipients, outbound); + devices + .entry(user_id.to_owned()) + .or_default() + .extend(recipient_devices.allowed_devices); + withheld_devices.extend(recipient_devices.denied_devices_with_code); + } } - - devices.entry(user_id.to_owned()).or_default().extend(recipients); - withheld_devices.extend(withheld_recipients); - } + }; if should_rotate { debug!( From e3092195ae1bec2641a3d0d687b101c1549b9259 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 8 Jul 2024 14:45:04 +0200 Subject: [PATCH 16/38] feat(crypto): Key distribution errors for Pin violations --- crates/matrix-sdk-crypto/src/error.rs | 26 ++ .../group_sessions/share_strategy.rs | 230 +++++++++++++++++- .../src/test_json/keys_query_sets.rs | 146 +++++++++++ 3 files changed, 392 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index c73a8ed27e9..5578f67753d 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -69,8 +69,34 @@ pub enum OlmError { have a valid Olm session with us" )] MissingSession, + #[error(transparent)] + /// The room key that should be shared was not due to an error. + KeyDistributionError(RoomKeyDistributionError), } +/// Depending on the sharing strategy for room keys, the distribution of the +/// room key could fail. +#[derive(Error, Debug)] +pub enum RoomKeyDistributionError { + /// When encrypting using the IdentityBased strategy. + /// Will be thrown when sharing room keys when there is a new identity for a + /// user that has not been confirmed by the user. + /// Application should display identity changes to the user as soon as + /// possible to avoid hitting this case. If it happens the app might + /// just retry automatically after the identity change has been + /// notified, or offer option to cancel. + #[error("Encryption failed because there are key pinning violation, please re-pin or verify the problematic users")] + KeyPinningViolation(Vec), + + /// Cross-signing is required for encryption with invisible crypto + #[error("Encryption failed: Setup cross-signing on your account")] + CrossSigningNotSetup, + /// The current device needs to be verified when encrypting using the + /// IdentityBased strategy. Apps should prevent sending in the UI to + /// avoid hitting this case. + #[error("Encryption failed: Verify your device to send encrypted messages")] + SendingFromUnverifiedDevice, +} /// Error representing a failure during a group encryption operation. #[derive(Error, Debug)] pub enum MegolmError { diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index 54fb035036d..86ee62d9523 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -24,8 +24,10 @@ use tracing::{debug, instrument, trace}; use super::OutboundGroupSession; use crate::{ - error::OlmResult, store::Store, types::events::room_key_withheld::WithheldCode, DeviceData, - EncryptionSettings, OwnUserIdentityData, UserIdentityData, + error::{OlmResult, RoomKeyDistributionError}, + store::Store, + types::events::room_key_withheld::WithheldCode, + DeviceData, EncryptionSettings, OlmError, OwnUserIdentityData, UserIdentityData, }; /// Strategy to collect the devices that should receive room keys for the @@ -123,15 +125,15 @@ pub(crate) async fn collect_session_recipients( // This is calculated in the following code and stored in this variable. let mut should_rotate = user_left || visibility_changed || algorithm_changed; - let mut devices: BTreeMap> = Default::default(); - let mut withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)> = Default::default(); + let mut devices: BTreeMap> = Default::default(); + let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); match settings.sharing_strategy { CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); for user_id in users { - let user_devices = store.get_readonly_devices_filtered(user_id).await?; + let user_devices = store.get_device_data_for_user(user_id).await?; // We only need the user identity if only_allow_trusted_devices is set. let device_owner_identity = if only_allow_trusted_devices { @@ -166,12 +168,42 @@ pub(crate) async fn collect_session_recipients( } } CollectStrategy::IdentityBasedStrategy => { - let _maybe_own_identity = + let maybe_own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + let own_verified_identity = match maybe_own_identity { + None => { + return Err(OlmError::KeyDistributionError( + RoomKeyDistributionError::CrossSigningNotSetup, + )) + } + Some(identity) if !identity.is_verified() => { + return Err(OlmError::KeyDistributionError( + RoomKeyDistributionError::SendingFromUnverifiedDevice, + )) + } + Some(identity) => identity, + }; + + // Collect all potential identity mismatch and report at the end if any. + let mut users_with_identity_mismatch: Vec = Vec::default(); for user_id in users { - let user_devices = store.get_readonly_devices_filtered(user_id).await?; let device_owner_identity = store.get_user_identity(user_id).await?; + // debug!(?device_owner_identity, "Other Checking identity"); + + if let Some(other_identity) = device_owner_identity.as_ref().and_then(|i| i.other()) + { + if other_identity.has_pin_violation() + && own_verified_identity.is_identity_signed(other_identity).is_err() + { + debug!(?other_identity, "Identity Mismatch detected"); + // Identity mismatch + users_with_identity_mismatch.push(other_identity.user_id().to_owned()); + continue; + } + } + let user_devices = store.get_device_data_for_user_filtered(user_id).await?; + let recipient_devices = split_recipients_withhelds_for_user_based_on_identity( user_devices, &device_owner_identity, @@ -195,6 +227,13 @@ pub(crate) async fn collect_session_recipients( .extend(recipient_devices.allowed_devices); withheld_devices.extend(recipient_devices.denied_devices_with_code); } + + // Abort sharing and fail when there are identities mismatch. + if !users_with_identity_mismatch.is_empty() { + return Err(OlmError::KeyDistributionError( + RoomKeyDistributionError::KeyPinningViolation(users_with_identity_mismatch), + )); + } } }; @@ -217,7 +256,7 @@ pub(crate) async fn collect_session_recipients( // blacklisted in the meantime. If it's the case the session should be rotated. fn should_rotate_due_to_left_device( user_id: &UserId, - future_recipients: &Vec, + future_recipients: &[DeviceData], outbound: &OutboundGroupSession, ) -> bool { // Device IDs that should receive this session @@ -323,7 +362,12 @@ mod tests { use std::sync::Arc; - use matrix_sdk_test::{async_test, test_json::keys_query_sets::KeyDistributionTestData}; + use matrix_sdk_test::{ + async_test, + test_json::keys_query_sets::{ + IdentityChangeDataSet, KeyDistributionTestData, MaloIdentityChangeDataSet, + }, + }; use ruma::{events::room::history_visibility::HistoryVisibility, room_id, TransactionId}; use crate::{ @@ -332,7 +376,7 @@ mod tests { group_sessions::share_strategy::collect_session_recipients, CollectStrategy, }, types::events::room_key_withheld::WithheldCode, - CrossSigningKeyExport, EncryptionSettings, OlmMachine, + CrossSigningKeyExport, EncryptionSettings, OlmError, OlmMachine, }; async fn set_up_test_machine() -> OlmMachine { @@ -570,6 +614,172 @@ mod tests { assert_eq!(code, &WithheldCode::Unauthorised); } + #[async_test] + async fn test_share_identity_strategy_no_cross_signing() { + let machine: OlmMachine = OlmMachine::new( + KeyDistributionTestData::me_id(), + KeyDistributionTestData::me_device_id(), + ) + .await; + + let keys_query = KeyDistributionTestData::dan_keys_query_response(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let strategy = CollectStrategy::new_identity_based(); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let request_result = machine + .share_room_key( + fake_room_id, + vec![KeyDistributionTestData::dan_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_err()); + let err = request_result.unwrap_err(); + match err { + OlmError::KeyDistributionError( + crate::error::RoomKeyDistributionError::CrossSigningNotSetup, + ) => { + // ok + } + _ => panic!("Unexpected share error"), + } + + // Let's now have cross-signing, but the identity is not trusted + let keys_query = KeyDistributionTestData::me_keys_query_response(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let request_result = machine + .share_room_key( + fake_room_id, + vec![KeyDistributionTestData::dan_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_err()); + let err = request_result.unwrap_err(); + match err { + OlmError::KeyDistributionError( + crate::error::RoomKeyDistributionError::SendingFromUnverifiedDevice, + ) => { + // ok + } + _ => panic!("Unexpected share error"), + } + + // If we are verified it should then work + machine + .import_cross_signing_keys(CrossSigningKeyExport { + master_key: KeyDistributionTestData::MASTER_KEY_PRIVATE_EXPORT.to_owned().into(), + self_signing_key: KeyDistributionTestData::SELF_SIGNING_KEY_PRIVATE_EXPORT + .to_owned() + .into(), + user_signing_key: KeyDistributionTestData::USER_SIGNING_KEY_PRIVATE_EXPORT + .to_owned() + .into(), + }) + .await + .unwrap(); + + let request_result = machine + .share_room_key( + fake_room_id, + vec![KeyDistributionTestData::dan_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_ok()); + } + + #[async_test] + async fn test_share_identity_strategy_report_pinning_violation() { + let machine: OlmMachine = OlmMachine::new( + KeyDistributionTestData::me_id(), + KeyDistributionTestData::me_device_id(), + ) + .await; + + machine.bootstrap_cross_signing(false).await.unwrap(); + + let keys_query = IdentityChangeDataSet::key_query_with_identity_a(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let keys_query = MaloIdentityChangeDataSet::initial_key_query(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + // Simulate pinning violation + let keys_query = IdentityChangeDataSet::key_query_with_identity_b(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let keys_query = MaloIdentityChangeDataSet::updated_key_query(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + let strategy = CollectStrategy::new_identity_based(); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let request_result = machine + .share_room_key( + fake_room_id, + vec![IdentityChangeDataSet::user_id(), MaloIdentityChangeDataSet::user_id()] + .into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_err()); + let err = request_result.unwrap_err(); + match err { + OlmError::KeyDistributionError( + crate::error::RoomKeyDistributionError::KeyPinningViolation(affected_users), + ) => { + assert_eq!(2, affected_users.len()); + + // resolve by pinning the new identity + for user_id in affected_users { + machine + .get_identity(&user_id, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .pin_current_master_key() + .await + .unwrap() + } + + let request_result = machine + .share_room_key( + fake_room_id, + // vec![KeyDistributionTestData::dan_id()].into_iter(), + vec![IdentityChangeDataSet::user_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_ok()); + } + _ => panic!("Unexpected share error"), + } + } + #[async_test] async fn test_should_rotate_based_on_visibility() { let machine = set_up_test_machine().await; diff --git a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs index bf5c1a3321e..070898b4581 100644 --- a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs +++ b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs @@ -667,3 +667,149 @@ impl IdentityChangeDataSet { .expect("Can't parse the `/keys/upload` response") } } + +/// A set of keys query to test identity changes, +/// For user @malo, that performed an identity change with the same device. +pub struct MaloIdentityChangeDataSet {} + +#[allow(dead_code)] +impl MaloIdentityChangeDataSet { + pub fn user_id() -> &'static UserId { + user_id!("@malo:localhost") + } + + pub fn device_id() -> &'static DeviceId { + device_id!("NZFSPBRLDO") + } + + pub fn initial_key_query() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@malo:localhost": { + "NZFSPBRLDO": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "NZFSPBRLDO", + "keys": { + "curve25519:NZFSPBRLDO": "L3jdbw42+9i+K7LPjAY+kmqG9nr2n/U0ow8hEbLCoCs", + "ed25519:NZFSPBRLDO": "VDJt3xI4SzrgQkuE3sEIauluaXawx3wWoWOynPI8Zko" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "lmtbdrJ5xBweo677Fg2qrSHsRi4R3x2WNlvSNJY6Zbg0R5lJS9syN2HZw/irL9PA644GYm4QM/t+DX0grnn+BQ", + "ed25519:+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8": "Ql1fq+SvVDx+8mjNMzSaR0hBCEkdPirbs2+BK0gwsIH1zkuMADnBoNWP7LJiKo/EO9gnpiCzyQQgI4e9pIVPDA" + } + }, + "user_id": "@malo:localhost", + "unsigned": {} + } + } + }, + "failures": {}, + "master_keys": { + "@malo:localhost": { + "keys": { + "ed25519:WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE": "WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "crJcXqFpEHRM8KNUw419XrVFaHoM8/kV4ebgpuuIiD9wfX0AhHE2iGRGpKzsrVCqne9k181/uN0sgDMpK2y4Aw", + "ed25519:WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE": "/xwFF5AC3GhkpvJ449Srh8kNQS6CXAxQMmBpQvPEHx5BHPXJ08u2ZDd1EPYY4zk4QsePk+tEYu8gDnB0bggHCA" + } + }, + "usage": [ + "master" + ], + "user_id": "@malo:localhost" + } + }, + "self_signing_keys": { + "@malo:localhost": { + "keys": { + "ed25519:+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8": "+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8" + }, + "signatures": { + "@malo:localhost": { + "ed25519:WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE": "sSGQ6ny6aXtIvgKPGOYJzcmnNDSkbaJFVRe9wekOry7EaiWf2l28MkGTUBt4cPoRiMkNjuRBupNEARqHF72sAQ" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@malo:localhost" + } + }, + "user_signing_keys": {}, + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + pub fn updated_key_query() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@malo:localhost": { + "NZFSPBRLDO": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "NZFSPBRLDO", + "keys": { + "curve25519:NZFSPBRLDO": "L3jdbw42+9i+K7LPjAY+kmqG9nr2n/U0ow8hEbLCoCs", + "ed25519:NZFSPBRLDO": "VDJt3xI4SzrgQkuE3sEIauluaXawx3wWoWOynPI8Zko" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "lmtbdrJ5xBweo677Fg2qrSHsRi4R3x2WNlvSNJY6Zbg0R5lJS9syN2HZw/irL9PA644GYm4QM/t+DX0grnn+BQ", + "ed25519:+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8": "Ql1fq+SvVDx+8mjNMzSaR0hBCEkdPirbs2+BK0gwsIH1zkuMADnBoNWP7LJiKo/EO9gnpiCzyQQgI4e9pIVPDA", + "ed25519:8my6+zgnzEP0ZqmQFyvscJh7isHlf8lxBmHg+fzdJkE": "OvqDE7C2mrHxjwNyMIEz+m/AO6I6lM5HoPYY2bvLjrJJDOF5sJOtw4JoYiCWyt90ZIWsbEqmfbazrblLD50tCg" + } + }, + "user_id": "@malo:localhost", + "unsigned": {} + } + } + }, + "failures": {}, + "master_keys": { + "@malo:localhost": { + "keys": { + "ed25519:dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU": "dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "2Ye96l4srBSWskNQszuMpea1r97rFoUyfNqegvu/hGeP47w0OVvqYuNtZRNwqb7TMS7aPEn6l9lhWEk7v06wCg", + "ed25519:dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU": "btkxAJpJeVtc9wgBmeHUI9QDpojd6ddLxK11E3403KoTQtP6Mnr5GsVdQr1HJToG7PG4k4eEZGWxVZr1GPndAA" + } + }, + "usage": [ + "master" + ], + "user_id": "@malo:localhost" + } + }, + "self_signing_keys": { + "@malo:localhost": { + "keys": { + "ed25519:8my6+zgnzEP0ZqmQFyvscJh7isHlf8lxBmHg+fzdJkE": "8my6+zgnzEP0ZqmQFyvscJh7isHlf8lxBmHg+fzdJkE" + }, + "signatures": { + "@malo:localhost": { + "ed25519:dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU": "KJt0y1p8v8RGLGk2wUyCMbX1irXJqup/mdRuG/cxJxs24BZhDMyIzyGrGXnWq2gx3I4fKIMtFPi/ecxf92ePAQ" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@malo:localhost" + } + }, + "user_signing_keys": {} + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } +} From f0e9a18d36f02c14fc56eaf736c75035e71b1af2 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 23 Jul 2024 15:53:31 -0400 Subject: [PATCH 17/38] remove debugging statements --- crates/matrix-sdk-crypto/src/machine.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 70cf80d3a55..6af727ea3bc 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1555,15 +1555,12 @@ impl OlmMachine { require_trusted: bool, ) -> MegolmResult<()> { let Some(device) = self.inner.store.get_device_from_curve_key(sender, sender_key).await? else { - debug!("1"); return Err(MegolmError::SenderCrossSigningIdentityUnknown) }; if !device.is_cross_signed_by_owner() { - debug!("2"); return Err(MegolmError::SenderCrossSigningIdentityUnknown) } if device.is_cross_signing_trusted() { - debug!("3"); return Ok(()) } if sender == self.inner.user_id { @@ -1571,10 +1568,8 @@ impl OlmMachine { // cross-signed by us, so we reject it Err(MegolmError::SenderCrossSigningIdentityUnknown) } else if require_trusted { - debug!("4"); Err(MegolmError::SenderCrossSigningUntrusted) } else { - debug!("5"); // we don't require the sender to be trusted, but we require that // the sender's identity matches their pinned identity let identity = device.device_owner_identity.expect("Cross-signed device must have owner identity"); From 7254e7eb48aa142fd177f441237675ab04f32826 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 23 Jul 2024 16:10:12 -0400 Subject: [PATCH 18/38] add some comments --- crates/matrix-sdk-crypto/src/machine.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 6af727ea3bc..789217ec213 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1548,11 +1548,17 @@ impl OlmMachine { self.get_encryption_info(&session, &event.sender).await } + /// Check whether the sender of a Megolm session is trusted. + /// + /// Checks that the device is cross-signed, that the sender's identity is + /// cross-signed, and that the sender's identity is pinned. If + /// `require_verified` is `true`, then also checks if we have verified the + /// sender's identity async fn check_sender_trusted( &self, sender_key: Curve25519PublicKey, sender: &UserId, - require_trusted: bool, + require_verified: bool, ) -> MegolmResult<()> { let Some(device) = self.inner.store.get_device_from_curve_key(sender, sender_key).await? else { return Err(MegolmError::SenderCrossSigningIdentityUnknown) @@ -1567,7 +1573,7 @@ impl OlmMachine { // if we get here, the device (that claims to be ours) wasn't // cross-signed by us, so we reject it Err(MegolmError::SenderCrossSigningIdentityUnknown) - } else if require_trusted { + } else if require_verified { Err(MegolmError::SenderCrossSigningUntrusted) } else { // we don't require the sender to be trusted, but we require that @@ -1581,6 +1587,8 @@ impl OlmMachine { } } + /// Check that the sender of a Megolm session satisfies the trust + /// requirement from the decryption settings. async fn check_sender_trust_requirement( &self, session: &InboundGroupSession, From 3f7b840373ffebf8d035e23a8bb9c5d9b67393be Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 24 Jul 2024 10:38:59 -0400 Subject: [PATCH 19/38] cargo fmt --- crates/matrix-sdk-crypto/src/machine.rs | 184 +++++++++++++++--------- 1 file changed, 116 insertions(+), 68 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 789217ec213..84ba466de59 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -66,8 +66,8 @@ use crate::{ identities::{user::UserIdentities, Device, IdentityManager, UserDevices}, olm::{ Account, CrossSigningStatus, DecryptionSettings, EncryptionSettings, IdentityKeys, - InboundGroupSession, OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, SenderDataFinder, - SessionType, StaticAccountData, TrustRequirement, + InboundGroupSession, OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, + SenderDataFinder, SessionType, StaticAccountData, TrustRequirement, }, requests::{IncomingResponse, OutgoingRequest, UploadSigningKeysRequest}, session_manager::{GroupSessionManager, SessionManager}, @@ -1560,14 +1560,15 @@ impl OlmMachine { sender: &UserId, require_verified: bool, ) -> MegolmResult<()> { - let Some(device) = self.inner.store.get_device_from_curve_key(sender, sender_key).await? else { - return Err(MegolmError::SenderCrossSigningIdentityUnknown) + let Some(device) = self.inner.store.get_device_from_curve_key(sender, sender_key).await? + else { + return Err(MegolmError::SenderCrossSigningIdentityUnknown); }; if !device.is_cross_signed_by_owner() { - return Err(MegolmError::SenderCrossSigningIdentityUnknown) + return Err(MegolmError::SenderCrossSigningIdentityUnknown); } if device.is_cross_signing_trusted() { - return Ok(()) + return Ok(()); } if sender == self.inner.user_id { // if we get here, the device (that claims to be ours) wasn't @@ -1578,7 +1579,8 @@ impl OlmMachine { } else { // we don't require the sender to be trusted, but we require that // the sender's identity matches their pinned identity - let identity = device.device_owner_identity.expect("Cross-signed device must have owner identity"); + let identity = + device.device_owner_identity.expect("Cross-signed device must have owner identity"); if identity.other().expect("Device is not our own device").has_pin_violation() { Err(MegolmError::SenderCrossSigningIdentityChanged) } else { @@ -1597,26 +1599,20 @@ impl OlmMachine { ) -> MegolmResult<()> { match decryption_settings.trust_requirement { TrustRequirement::Untrusted => Ok(()), - TrustRequirement::CrossSignedOrLegacy => { - match session.sender_data { - SenderData::SenderKnown { .. } => Ok(()), - SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), - SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), - _ => self.check_sender_trusted(session.sender_key(), sender, false).await, - } - } - TrustRequirement::CrossSigned => { - match session.sender_data { - SenderData::SenderKnown { .. } => Ok(()), - _ => self.check_sender_trusted(session.sender_key(), sender, false).await, - } - } - TrustRequirement::VerifiedUser => { - match session.sender_data { - SenderData::SenderKnown { master_key_verified: true, .. } => Ok(()), - _ => self.check_sender_trusted(session.sender_key(), sender, true).await, - } - } + TrustRequirement::CrossSignedOrLegacy => match session.sender_data { + SenderData::SenderKnown { .. } => Ok(()), + SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), + SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), + _ => self.check_sender_trusted(session.sender_key(), sender, false).await, + }, + TrustRequirement::CrossSigned => match session.sender_data { + SenderData::SenderKnown { .. } => Ok(()), + _ => self.check_sender_trusted(session.sender_key(), sender, false).await, + }, + TrustRequirement::VerifiedUser => match session.sender_data { + SenderData::SenderKnown { master_key_verified: true, .. } => Ok(()), + _ => self.check_sender_trusted(session.sender_key(), sender, true).await, + }, } } @@ -2520,7 +2516,9 @@ pub(crate) mod tests { OutboundGroupSession, SenderData, SenderDataRetryDetails, TrustRequirement, VerifyJson, }, session_manager::CollectStrategy, - store::{BackupDecryptionKey, Changes, CryptoStore, IdentityChanges, MemoryStore, RoomSettings}, + store::{ + BackupDecryptionKey, Changes, CryptoStore, IdentityChanges, MemoryStore, RoomSettings, + }, types::{ events::{ room::encrypted::{EncryptedToDeviceEvent, ToDeviceEncryptedEventContent}, @@ -2529,7 +2527,8 @@ pub(crate) mod tests { }, ToDeviceEvent, }, - CrossSigningKey, DeviceKeys, EventEncryptionAlgorithm, MasterPubkey, SelfSigningPubkey, SignedKey, SigningKeys, + CrossSigningKey, DeviceKeys, EventEncryptionAlgorithm, MasterPubkey, SelfSigningPubkey, + SignedKey, SigningKeys, }, utilities::json_convert, verification::tests::{bob_id, outgoing_request_to_event, request_to_event}, @@ -5246,7 +5245,8 @@ pub(crate) mod tests { // Bob receives the message but Alice's keys are unknown at the time of // reception - let mut session = bob.store() + let mut session = bob + .store() .get_inbound_group_session(&room_id, group_session.session_id()) .await .unwrap() @@ -5260,18 +5260,38 @@ pub(crate) mod tests { // Bob later gets Alice's device keys and identity let cross_signing_requests = alice.bootstrap_cross_signing(false).await.unwrap(); let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; - let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); - let alice_ssk: SelfSigningPubkey = upload_signing_keys_req.self_signing_key.unwrap().try_into().unwrap(); + let alice_msk: MasterPubkey = + upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); + let alice_ssk: SelfSigningPubkey = + upload_signing_keys_req.self_signing_key.unwrap().try_into().unwrap(); let upload_keys_req = cross_signing_requests.upload_keys_req.unwrap().clone(); - assert_let!(OutgoingRequests::KeysUpload(device_upload_request) = upload_keys_req.request.as_ref()); - bob.store().save_device_data(&[DeviceData::try_from(&device_upload_request.device_keys.as_ref().unwrap().deserialize_as::().unwrap()).unwrap()]).await.unwrap(); - bob.store().save_changes(Changes { - identities: IdentityChanges { - new: vec![OtherUserIdentityData::new(alice_msk.clone(), alice_ssk.clone()).unwrap().into()], - .. Default::default() - }, - .. Default::default() - }).await.unwrap(); + assert_let!( + OutgoingRequests::KeysUpload(device_upload_request) = upload_keys_req.request.as_ref() + ); + bob.store() + .save_device_data(&[DeviceData::try_from( + &device_upload_request + .device_keys + .as_ref() + .unwrap() + .deserialize_as::() + .unwrap(), + ) + .unwrap()]) + .await + .unwrap(); + bob.store() + .save_changes(Changes { + identities: IdentityChanges { + new: vec![OtherUserIdentityData::new(alice_msk.clone(), alice_ssk.clone()) + .unwrap() + .into()], + ..Default::default() + }, + ..Default::default() + }) + .await + .unwrap(); // Since the sending device is now cross-signed by Alice, it should be // decryptable in all modes except for verified. @@ -5289,23 +5309,30 @@ pub(crate) mod tests { assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); // If we verify Alice, the event should be decryptable in verified mode - let mut alice_identity = bob.store().get_identity(alice.user_id()).await.unwrap().unwrap().other().unwrap(); + let mut alice_identity = + bob.store().get_identity(alice.user_id()).await.unwrap().unwrap().other().unwrap(); let signature_upload_req = alice_identity.verify().await.unwrap(); let alice_read_only_identity = &mut alice_identity.inner; let mut alice_msk_json = serde_json::to_value(alice_msk).unwrap(); for (_, value) in signature_upload_req.signed_keys.get(alice.user_id()).unwrap().iter() { let value: serde_json::Value = serde_json::from_str(value.get()).unwrap(); - alice_msk_json["signatures"].as_object_mut().unwrap().insert(bob.user_id().to_string(), value["signatures"][bob.user_id().as_str()].clone()); + alice_msk_json["signatures"].as_object_mut().unwrap().insert( + bob.user_id().to_string(), + value["signatures"][bob.user_id().as_str()].clone(), + ); } let alice_msk: MasterPubkey = serde_json::from_value(alice_msk_json).unwrap(); alice_read_only_identity.update(alice_msk.clone(), alice_ssk.clone()).unwrap(); - bob.store().save_changes(Changes { - identities: IdentityChanges { - new: vec![alice_read_only_identity.clone().into()], - .. Default::default() - }, - .. Default::default() - }).await.unwrap(); + bob.store() + .save_changes(Changes { + identities: IdentityChanges { + new: vec![alice_read_only_identity.clone().into()], + ..Default::default() + }, + ..Default::default() + }) + .await + .unwrap(); let decryption_settings = DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); @@ -5313,19 +5340,37 @@ pub(crate) mod tests { // If alice's cross-signing key changes, the event should not be decryptable (except for in unverified mode) let cross_signing_requests = alice.bootstrap_cross_signing(true).await.unwrap(); let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; - let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); - let alice_ssk: SelfSigningPubkey = upload_signing_keys_req.self_signing_key.unwrap().try_into().unwrap(); + let alice_msk: MasterPubkey = + upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); + let alice_ssk: SelfSigningPubkey = + upload_signing_keys_req.self_signing_key.unwrap().try_into().unwrap(); let upload_keys_req = cross_signing_requests.upload_keys_req.unwrap().clone(); - assert_let!(OutgoingRequests::KeysUpload(device_upload_request) = upload_keys_req.request.as_ref()); - bob.store().save_device_data(&[DeviceData::try_from(&device_upload_request.device_keys.as_ref().unwrap().deserialize_as::().unwrap()).unwrap()]).await.unwrap(); + assert_let!( + OutgoingRequests::KeysUpload(device_upload_request) = upload_keys_req.request.as_ref() + ); + bob.store() + .save_device_data(&[DeviceData::try_from( + &device_upload_request + .device_keys + .as_ref() + .unwrap() + .deserialize_as::() + .unwrap(), + ) + .unwrap()]) + .await + .unwrap(); alice_read_only_identity.update(alice_msk.clone(), alice_ssk.clone()).unwrap(); - bob.store().save_changes(Changes { - identities: IdentityChanges { - new: vec![alice_read_only_identity.clone().into()], - .. Default::default() - }, - .. Default::default() - }).await.unwrap(); + bob.store() + .save_changes(Changes { + identities: IdentityChanges { + new: vec![alice_read_only_identity.clone().into()], + ..Default::default() + }, + ..Default::default() + }) + .await + .unwrap(); let decryption_settings = DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; @@ -5343,13 +5388,16 @@ pub(crate) mod tests { // ... until we acknowledge the change, at which point it should be // decryptable in every mode except for verified alice_read_only_identity.pin(); - bob.store().save_changes(Changes { - identities: IdentityChanges { - new: vec![alice_read_only_identity.clone().into()], - .. Default::default() - }, - .. Default::default() - }).await.unwrap(); + bob.store() + .save_changes(Changes { + identities: IdentityChanges { + new: vec![alice_read_only_identity.clone().into()], + ..Default::default() + }, + ..Default::default() + }) + .await + .unwrap(); let decryption_settings = DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); From ee9edf9743270104bdaf2df3d7697ed08f7e2c25 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 31 Jul 2024 18:07:03 -0400 Subject: [PATCH 20/38] make other crates compile --- crates/matrix-sdk-base/src/client.rs | 9 ++++--- crates/matrix-sdk-ui/src/timeline/traits.rs | 11 ++++++-- crates/matrix-sdk/src/room/mod.rs | 30 ++++++++++++--------- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 51f864a47df..656527213cd 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -28,8 +28,9 @@ use futures_util::Stream; use matrix_sdk_common::instant::Instant; #[cfg(feature = "e2e-encryption")] use matrix_sdk_crypto::{ - store::DynCryptoStore, EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine, - ToDeviceRequest, + olm::{DecryptionSettings, TrustRequirement}, + store::DynCryptoStore, + EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine, ToDeviceRequest, }; #[cfg(feature = "e2e-encryption")] use ruma::events::{ @@ -299,8 +300,10 @@ impl BaseClient { let olm = self.olm_machine().await; let Some(olm) = olm.as_ref() else { return Ok(None) }; + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; let event: SyncTimelineEvent = - olm.decrypt_room_event(event.cast_ref(), room_id).await?.into(); + olm.decrypt_room_event(event.cast_ref(), room_id, &decryption_settings).await?.into(); if let Ok(AnySyncTimelineEvent::MessageLike(e)) = event.event.deserialize() { match &e { diff --git a/crates/matrix-sdk-ui/src/timeline/traits.rs b/crates/matrix-sdk-ui/src/timeline/traits.rs index af8711cfe4b..0b985bf230b 100644 --- a/crates/matrix-sdk-ui/src/timeline/traits.rs +++ b/crates/matrix-sdk-ui/src/timeline/traits.rs @@ -15,7 +15,11 @@ use async_trait::async_trait; use indexmap::IndexMap; #[cfg(feature = "e2e-encryption")] -use matrix_sdk::{deserialized_responses::TimelineEvent, Result}; +use matrix_sdk::{ + crypto::olm::{DecryptionSettings, TrustRequirement}, + deserialized_responses::TimelineEvent, + Result, +}; use matrix_sdk::{event_cache::paginator::PaginableRoom, Room}; use matrix_sdk_base::latest_event::LatestEvent; #[cfg(feature = "e2e-encryption")] @@ -234,7 +238,10 @@ impl Decryptor for Room { impl Decryptor for (matrix_sdk_base::crypto::OlmMachine, ruma::OwnedRoomId) { async fn decrypt_event_impl(&self, raw: &Raw) -> Result { let (olm_machine, room_id) = self; - let event = olm_machine.decrypt_room_event(raw.cast_ref(), room_id).await?; + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let event = + olm_machine.decrypt_room_event(raw.cast_ref(), room_id, &decryption_settings).await?; Ok(event) } } diff --git a/crates/matrix-sdk/src/room/mod.rs b/crates/matrix-sdk/src/room/mod.rs index 04a1635c75f..344b0e2a070 100644 --- a/crates/matrix-sdk/src/room/mod.rs +++ b/crates/matrix-sdk/src/room/mod.rs @@ -14,6 +14,8 @@ use futures_util::{ future::{try_join, try_join_all}, stream::FuturesUnordered, }; +#[cfg(feature = "e2e-encryption")] +use matrix_sdk_base::crypto::olm::{DecryptionSettings, TrustRequirement}; use matrix_sdk_base::{ deserialized_responses::{ RawAnySyncOrStrippedState, RawSyncOrStrippedState, SyncOrStrippedState, TimelineEvent, @@ -1126,18 +1128,22 @@ impl Room { let machine = self.client.olm_machine().await; let machine = machine.as_ref().ok_or(Error::NoOlmMachine)?; - let mut event = - match machine.decrypt_room_event(event.cast_ref(), self.inner.room_id()).await { - Ok(event) => event, - Err(e) => { - self.client - .encryption() - .backups() - .maybe_download_room_key(self.room_id().to_owned(), event.clone()); - - return Err(e.into()); - } - }; + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let mut event = match machine + .decrypt_room_event(event.cast_ref(), self.inner.room_id(), &decryption_settings) + .await + { + Ok(event) => event, + Err(e) => { + self.client + .encryption() + .backups() + .maybe_download_room_key(self.room_id().to_owned(), event.clone()); + + return Err(e.into()); + } + }; event.push_actions = self.event_push_actions(&event.event).await?; From 62531c8ee9e042737ba46407895e5c50a972fea5 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 2 Aug 2024 17:51:53 -0400 Subject: [PATCH 21/38] address some review comments --- crates/matrix-sdk-crypto/src/machine.rs | 16 ++++++++-------- .../src/olm/group_sessions/inbound.rs | 14 ++++++++------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 84ba466de59..aed99a9816d 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1609,7 +1609,7 @@ impl OlmMachine { SenderData::SenderKnown { .. } => Ok(()), _ => self.check_sender_trusted(session.sender_key(), sender, false).await, }, - TrustRequirement::VerifiedUser => match session.sender_data { + TrustRequirement::VerifiedUserIdentity => match session.sender_data { SenderData::SenderKnown { master_key_verified: true, .. } => Ok(()), _ => self.check_sender_trusted(session.sender_key(), sender, true).await, }, @@ -5141,7 +5141,7 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); // An unknown legacy session should be decryptable only when the trust @@ -5162,7 +5162,7 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); // A session where we have the device keys but no cross-signing @@ -5190,7 +5190,7 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); } @@ -5305,7 +5305,7 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); // If we verify Alice, the event should be decryptable in verified mode @@ -5334,7 +5334,7 @@ pub(crate) mod tests { .await .unwrap(); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); // If alice's cross-signing key changes, the event should not be decryptable (except for in unverified mode) @@ -5382,7 +5382,7 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); // ... until we acknowledge the change, at which point it should be @@ -5408,7 +5408,7 @@ pub(crate) mod tests { DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUser }; + DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); } } diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index c724bb625c2..2d6ab8bb3da 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -55,17 +55,19 @@ use crate::{ }, }; -/// The trust level required to decrypt an event +/// The trust level required to decrypt an event. #[derive(Clone, Copy, Debug, Deserialize, Serialize)] pub enum TrustRequirement { - /// Decrypt events from everyone regardless of trust + /// Decrypt events from everyone regardless of trust. Untrusted, - /// Only decrypt events from cross-signed or legacy devices + /// Only decrypt events from cross-signed or legacy sessions (Megolm + /// sessions created before we started collecting trust information). CrossSignedOrLegacy, - /// Only decrypt events from cross-signed devices + /// Only decrypt events from cross-signed devices. CrossSigned, - /// Only decrypt events from cross-signed devices where we have verified the user - VerifiedUser, + /// Only decrypt events from cross-signed devices where we have verified the + /// user's identity. + VerifiedUserIdentity, } /// Settings for decrypting messages From b8b8c8b182cd2e81d6005879fe881c395eaf21ea Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 7 Aug 2024 17:28:55 -0400 Subject: [PATCH 22/38] fix test failure --- .../src/session_manager/group_sessions/share_strategy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index 86ee62d9523..2535b5d41cc 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -133,7 +133,7 @@ pub(crate) async fn collect_session_recipients( let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); for user_id in users { - let user_devices = store.get_device_data_for_user(user_id).await?; + let user_devices = store.get_device_data_for_user_filtered(user_id).await?; // We only need the user identity if only_allow_trusted_devices is set. let device_owner_identity = if only_allow_trusted_devices { From a09ede98b9edb772327cfc0415d0f31c7d319ddb Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 7 Aug 2024 17:41:55 -0400 Subject: [PATCH 23/38] add ChangedIdentity to VerificationLevel and use that for sender identity errors --- .../src/deserialized_responses.rs | 17 +++- crates/matrix-sdk-crypto/src/error.rs | 14 +-- crates/matrix-sdk-crypto/src/machine.rs | 98 +++++++++++-------- 3 files changed, 75 insertions(+), 54 deletions(-) diff --git a/crates/matrix-sdk-common/src/deserialized_responses.rs b/crates/matrix-sdk-common/src/deserialized_responses.rs index 1be75b99c99..89637def044 100644 --- a/crates/matrix-sdk-common/src/deserialized_responses.rs +++ b/crates/matrix-sdk-common/src/deserialized_responses.rs @@ -21,6 +21,7 @@ use ruma::{ DeviceKeyAlgorithm, OwnedDeviceId, OwnedEventId, OwnedUserId, }; use serde::{Deserialize, Serialize}; +use thiserror::Error; use crate::debug::{DebugRawEvent, DebugStructExt}; @@ -88,7 +89,7 @@ impl VerificationState { VerificationState::Verified => ShieldState::None, VerificationState::Unverified(level) => { let message = match level { - VerificationLevel::UnverifiedIdentity | VerificationLevel::UnsignedDevice => { + VerificationLevel::UnverifiedIdentity | VerificationLevel::ChangedIdentity | VerificationLevel::UnsignedDevice => { UNVERIFIED_IDENTITY } VerificationLevel::None(link) => match link { @@ -120,6 +121,11 @@ impl VerificationState { // then warn see https://github.com/matrix-org/matrix-rust-sdk/issues/1129 ShieldState::None } + VerificationLevel::ChangedIdentity => { + // As above, if you didn't show interest in verifying that + // user we don't nag you + ShieldState::None + } VerificationLevel::UnsignedDevice => { // This is a high warning. The sender hasn't verified his own device. ShieldState::Red { message: UNSIGNED_DEVICE } @@ -144,13 +150,19 @@ impl VerificationState { /// The sub-enum containing detailed information on why a message is considered /// to be unverified. -#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[derive(Clone, Debug, Error, Deserialize, Serialize, PartialEq, Eq)] pub enum VerificationLevel { /// The message was sent by a user identity we have not verified. + #[error("The sender's identity was not verified")] UnverifiedIdentity, + /// The message was sent by a user whose identity has changed and the change has not yet been confirmed + #[error("The sender's identity has changed and the change has not yet been confirmed")] + ChangedIdentity, + /// The message was sent by a device not linked to (signed by) any user /// identity. + #[error("The sending device was not signed by the user's identity")] UnsignedDevice, /// We weren't able to link the message back to any device. This might be @@ -158,6 +170,7 @@ pub enum VerificationLevel { /// not been able to obtain (for example, because the device was since /// deleted) or because the key to decrypt the message was obtained from /// an insecure source. + #[error("The sending device is not known")] None(DeviceLinkProblem), } diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index 69b1880d4c6..c263b39fb0e 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use matrix_sdk_common::deserialized_responses::VerificationLevel; use ruma::{CanonicalJsonError, IdParseError, OwnedDeviceId, OwnedRoomId, OwnedUserId}; use serde_json::Error as SerdeError; use thiserror::Error; @@ -143,17 +144,8 @@ pub enum MegolmError { Store(#[from] CryptoStoreError), /// The sender's cross-signing identity isn't trusted - #[error("message quarantined because sender's cross-signing identity is not trusted")] - SenderCrossSigningUntrusted, - - /// The sender's cross-signing identity has changed, and the change hasn't - /// been acknowledged - #[error("message quarantined because sender's cross-signing identity has changed")] - SenderCrossSigningIdentityChanged, - - /// The sender's cross-signing identity is unknown - #[error("message quarantined because sender is unknown")] - SenderCrossSigningIdentityUnknown, + #[error("message quarantined because sender's cross-signing identity is not trusted: {0}")] + SenderIdentity(#[from] VerificationLevel), } /// Error that occurs when decrypting an event that is malformed. diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index aed99a9816d..1a2a8982a9e 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1427,15 +1427,41 @@ impl OlmMachine { if device.is_cross_signed_by_owner() { // The device is cross signed by this owner Meaning that the user did self // verify it properly. Let's check if we trust the identity. - if device.is_device_owner_verified() { - (VerificationState::Verified, Some(device_id)) - } else { + let is_device_owner_verified = device.is_device_owner_verified(); + let identity = device + .device_owner_identity + .expect("Cross-signed device must have owner identity"); + if sender == self.user_id() { + if is_device_owner_verified { + (VerificationState::Verified, Some(device_id)) + } else { + ( + VerificationState::Unverified( + VerificationLevel::UnverifiedIdentity, + ), + Some(device_id), + ) + } + } else if identity + .other() + .expect("Device is not our own device") + .has_pin_violation() + { ( - VerificationState::Unverified( - VerificationLevel::UnverifiedIdentity, - ), + VerificationState::Unverified(VerificationLevel::ChangedIdentity), Some(device_id), ) + } else { + if is_device_owner_verified { + (VerificationState::Verified, Some(device_id)) + } else { + ( + VerificationState::Unverified( + VerificationLevel::UnverifiedIdentity, + ), + Some(device_id), + ) + } } } else { // The device owner hasn't self-verified its device. @@ -1554,47 +1580,32 @@ impl OlmMachine { /// cross-signed, and that the sender's identity is pinned. If /// `require_verified` is `true`, then also checks if we have verified the /// sender's identity - async fn check_sender_trusted( + fn check_sender_trusted( &self, - sender_key: Curve25519PublicKey, - sender: &UserId, + encryption_info: &EncryptionInfo, require_verified: bool, ) -> MegolmResult<()> { - let Some(device) = self.inner.store.get_device_from_curve_key(sender, sender_key).await? - else { - return Err(MegolmError::SenderCrossSigningIdentityUnknown); - }; - if !device.is_cross_signed_by_owner() { - return Err(MegolmError::SenderCrossSigningIdentityUnknown); - } - if device.is_cross_signing_trusted() { - return Ok(()); - } - if sender == self.inner.user_id { - // if we get here, the device (that claims to be ours) wasn't - // cross-signed by us, so we reject it - Err(MegolmError::SenderCrossSigningIdentityUnknown) - } else if require_verified { - Err(MegolmError::SenderCrossSigningUntrusted) - } else { - // we don't require the sender to be trusted, but we require that - // the sender's identity matches their pinned identity - let identity = - device.device_owner_identity.expect("Cross-signed device must have owner identity"); - if identity.other().expect("Device is not our own device").has_pin_violation() { - Err(MegolmError::SenderCrossSigningIdentityChanged) - } else { - Ok(()) + match dbg!(&encryption_info.verification_state) { + VerificationState::Verified => Ok(()), + VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => { + if require_verified { + Err(MegolmError::SenderIdentity(VerificationLevel::UnverifiedIdentity)) + } else { + Ok(()) + } + } + VerificationState::Unverified(verification_level) => { + Err(MegolmError::SenderIdentity(verification_level.clone())) } } } /// Check that the sender of a Megolm session satisfies the trust /// requirement from the decryption settings. - async fn check_sender_trust_requirement( + fn check_sender_trust_requirement( &self, session: &InboundGroupSession, - sender: &UserId, + encryption_info: &EncryptionInfo, decryption_settings: &DecryptionSettings, ) -> MegolmResult<()> { match decryption_settings.trust_requirement { @@ -1603,15 +1614,15 @@ impl OlmMachine { SenderData::SenderKnown { .. } => Ok(()), SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), - _ => self.check_sender_trusted(session.sender_key(), sender, false).await, + _ => self.check_sender_trusted(encryption_info, false), }, TrustRequirement::CrossSigned => match session.sender_data { SenderData::SenderKnown { .. } => Ok(()), - _ => self.check_sender_trusted(session.sender_key(), sender, false).await, + _ => self.check_sender_trusted(encryption_info, false), }, TrustRequirement::VerifiedUserIdentity => match session.sender_data { SenderData::SenderKnown { master_key_verified: true, .. } => Ok(()), - _ => self.check_sender_trusted(session.sender_key(), sender, true).await, + _ => self.check_sender_trusted(encryption_info, true), }, } } @@ -1626,8 +1637,6 @@ impl OlmMachine { let session = self.get_inbound_group_session_or_error(room_id, content.session_id()).await?; - self.check_sender_trust_requirement(&session, &event.sender, decryption_settings).await?; - // This function is only ever called by decrypt_room_event, so // room_id, sender, algorithm and session_id are recorded already // @@ -1639,6 +1648,13 @@ impl OlmMachine { match result { Ok((decrypted_event, _)) => { let encryption_info = self.get_encryption_info(&session, &event.sender).await?; + + self.check_sender_trust_requirement( + &session, + &encryption_info, + decryption_settings, + )?; + Ok((decrypted_event, encryption_info)) } Err(error) => Err( From 73300aa482ca902ee7770cc13336ca72a088392d Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 7 Aug 2024 20:00:49 -0400 Subject: [PATCH 24/38] factor out test utilities --- crates/matrix-sdk-crypto/src/machine.rs | 293 ++++++++++++------------ 1 file changed, 143 insertions(+), 150 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 1a2a8982a9e..8b5b2c4dbb2 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -2537,7 +2537,9 @@ pub(crate) mod tests { }, types::{ events::{ - room::encrypted::{EncryptedToDeviceEvent, ToDeviceEncryptedEventContent}, + room::encrypted::{ + EncryptedEvent, EncryptedToDeviceEvent, ToDeviceEncryptedEventContent, + }, room_key_withheld::{ MegolmV1AesSha2WithheldContent, RoomKeyWithheldContent, WithheldCode, }, @@ -5083,40 +5085,41 @@ pub(crate) mod tests { assert_matches!(thread_encryption_result, UnsignedDecryptionResult::Decrypted(_)); } - #[async_test] - async fn test_decryption_trust_requirement() { - let (alice, bob) = - get_machine_pair_with_setup_sessions_test_helper(alice_id(), user_id(), false).await; - let room_id = room_id!("!test:example.org"); - - let to_device_requests = alice - .share_room_key(room_id, iter::once(bob.user_id()), EncryptionSettings::default()) + // Helper function that encrypts a message and shares the Megolm session + // with a recipient + async fn encrypt_message( + sender: &OlmMachine, + room_id: &ruma::RoomId, + recipient: &OlmMachine, + plaintext: &str, + ) -> (Raw, String) { + let to_device_requests = sender + .share_room_key(room_id, iter::once(recipient.user_id()), EncryptionSettings::default()) .await .unwrap(); let event = ToDeviceEvent::new( - alice.user_id().to_owned(), + sender.user_id().to_owned(), to_device_requests_to_content(to_device_requests), ); - let group_session = bob + let group_session = recipient .store() .with_transaction(|mut tr| async { - let res = - bob.decrypt_to_device_event(&mut tr, &event, &mut Changes::default()).await?; + let res = recipient + .decrypt_to_device_event(&mut tr, &event, &mut Changes::default()) + .await?; Ok((tr, res)) }) .await .unwrap() .inbound_group_session .unwrap(); - bob.store().save_inbound_group_sessions(&[group_session.clone()]).await.unwrap(); - - let plaintext = "It is a secret to everybody"; + recipient.store().save_inbound_group_sessions(&[group_session.clone()]).await.unwrap(); let content = RoomMessageEventContent::text_plain(plaintext); - let encrypted_content = alice + let encrypted_content = sender .encrypt_room_event(room_id, AnyMessageLikeEventContent::RoomMessage(content.clone())) .await .unwrap(); @@ -5124,41 +5127,71 @@ pub(crate) mod tests { let event = json!({ "event_id": "$xxxxx:example.org", "origin_server_ts": MilliSecondsSinceUnixEpoch::now(), - "sender": alice.user_id(), + "sender": sender.user_id(), "type": "m.room.encrypted", "content": encrypted_content, }); let event = json_convert(&event).unwrap(); + (event, group_session.session_id().to_string()) + } + + // Helper function that checks whether a message is decryptable under different trust requirements. + // + // `tests` is a list of tuples, where the first element of the tuple is the + // trust requirement to check, and the second element indicates whether + // decryption should succeed (`true`) or fail (`false`). + async fn check_decryption_trust_requirement( + bob: &OlmMachine, + event: &Raw, + room_id: &ruma::RoomId, + tests: &[(TrustRequirement, bool)], + ) { + for (trust_requirement, is_ok) in tests { + let decryption_settings = + DecryptionSettings { trust_requirement: trust_requirement.clone() }; + if *is_ok { + assert!(bob.decrypt_room_event(event, room_id, &decryption_settings).await.is_ok()); + } else { + assert!(bob + .decrypt_room_event(event, room_id, &decryption_settings) + .await + .is_err()); + } + } + } + + #[async_test] + async fn test_decryption_trust_requirement() { + let (alice, bob) = + get_machine_pair_with_setup_sessions_test_helper(alice_id(), user_id(), false).await; + let room_id = room_id!("!test:example.org"); + let (event, session_id) = encrypt_message(&alice, room_id, &bob, "Secret message").await; + // Set the sender data to various values, and test that we can or can't // decrypt, depending on what the trust requirement is. // An unknown non-legacy session should be decryptable only when the trust // requirement allows untrusted sessions - let bob_store = bob.store(); - let mut session = bob_store - .get_inbound_group_session(&room_id, group_session.session_id()) - .await - .unwrap() - .unwrap(); + let mut session = + bob.store().get_inbound_group_session(room_id, &session_id).await.unwrap().unwrap(); session.sender_data = SenderData::UnknownDevice { retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: false, }; - bob_store.save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, false), + (TrustRequirement::CrossSigned, false), + (TrustRequirement::VerifiedUserIdentity, false), + ], + ); // An unknown legacy session should be decryptable only when the trust // requirement allows untrusted or legacy sessions @@ -5166,20 +5199,19 @@ pub(crate) mod tests { retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: true, }; - bob_store.save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, true), + (TrustRequirement::CrossSigned, false), + (TrustRequirement::VerifiedUserIdentity, false), + ], + ); // A session where we have the device keys but no cross-signing // information should be just like an unknown device @@ -5194,20 +5226,19 @@ pub(crate) mod tests { retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: false, }; - bob_store.save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, false), + (TrustRequirement::CrossSigned, false), + (TrustRequirement::VerifiedUserIdentity, false), + ], + ); } #[async_test] @@ -5216,57 +5247,12 @@ pub(crate) mod tests { get_machine_pair_with_setup_sessions_test_helper(alice_id(), user_id(), false).await; bob.bootstrap_cross_signing(false).await.unwrap(); let room_id = room_id!("!test:example.org"); - - // Alice encrypts a message for Bob - let to_device_requests = alice - .share_room_key(room_id, iter::once(bob.user_id()), EncryptionSettings::default()) - .await - .unwrap(); - - let event = ToDeviceEvent::new( - alice.user_id().to_owned(), - to_device_requests_to_content(to_device_requests), - ); - - let group_session = bob - .store() - .with_transaction(|mut tr| async { - let res = - bob.decrypt_to_device_event(&mut tr, &event, &mut Changes::default()).await?; - Ok((tr, res)) - }) - .await - .unwrap() - .inbound_group_session - .unwrap(); - bob.store().save_inbound_group_sessions(&[group_session.clone()]).await.unwrap(); - - let plaintext = "It is a secret to everybody"; - - let content = RoomMessageEventContent::text_plain(plaintext); - - let encrypted_content = alice - .encrypt_room_event(room_id, AnyMessageLikeEventContent::RoomMessage(content.clone())) - .await - .unwrap(); - - let event = json!({ - "event_id": "$xxxxx:example.org", - "origin_server_ts": MilliSecondsSinceUnixEpoch::now(), - "sender": alice.user_id(), - "type": "m.room.encrypted", - "content": encrypted_content, - }); - let event = json_convert(&event).unwrap(); + let (event, session_id) = encrypt_message(&alice, room_id, &bob, "Secret message").await; // Bob receives the message but Alice's keys are unknown at the time of // reception - let mut session = bob - .store() - .get_inbound_group_session(&room_id, group_session.session_id()) - .await - .unwrap() - .unwrap(); + let mut session = + bob.store().get_inbound_group_session(room_id, &session_id).await.unwrap().unwrap(); session.sender_data = SenderData::UnknownDevice { retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: false, @@ -5311,18 +5297,17 @@ pub(crate) mod tests { // Since the sending device is now cross-signed by Alice, it should be // decryptable in all modes except for verified. - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, true), + (TrustRequirement::CrossSigned, true), + (TrustRequirement::VerifiedUserIdentity, false), + ], + ); // If we verify Alice, the event should be decryptable in verified mode let mut alice_identity = @@ -5349,9 +5334,18 @@ pub(crate) mod tests { }) .await .unwrap(); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); + + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, true), + (TrustRequirement::CrossSigned, true), + (TrustRequirement::VerifiedUserIdentity, true), + ], + ); // If alice's cross-signing key changes, the event should not be decryptable (except for in unverified mode) let cross_signing_requests = alice.bootstrap_cross_signing(true).await.unwrap(); @@ -5388,18 +5382,17 @@ pub(crate) mod tests { .await .unwrap(); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, false), + (TrustRequirement::CrossSigned, false), + (TrustRequirement::VerifiedUserIdentity, false), + ], + ); // ... until we acknowledge the change, at which point it should be // decryptable in every mode except for verified @@ -5414,17 +5407,17 @@ pub(crate) mod tests { }) .await .unwrap(); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSignedOrLegacy }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::CrossSigned }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_ok()); - let decryption_settings = - DecryptionSettings { trust_requirement: TrustRequirement::VerifiedUserIdentity }; - assert!(bob.decrypt_room_event(&event, room_id, &decryption_settings).await.is_err()); + + check_decryption_trust_requirement( + &bob, + &event, + room_id, + &[ + (TrustRequirement::Untrusted, true), + (TrustRequirement::CrossSignedOrLegacy, true), + (TrustRequirement::CrossSigned, true), + (TrustRequirement::VerifiedUserIdentity, false), + ], + ); } } From b0994a5f7492256bdf62bb4527d13a7401d0c611 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 20:52:49 -0400 Subject: [PATCH 25/38] revert change to Cargo.lock --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index b7da79177b8..48376f542a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3903,7 +3903,7 @@ version = "5.0.0-alpha.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "098af5a5110b4deacf3200682963713b143ae9d28762b739bdb7b98429dfaf68" dependencies = [ - "base64 0.21.7", + "base64 0.22.1", "chrono", "getrandom", "http 1.1.0", From c53674b177d61d35bddf98b45d9522cc87c15e79 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 20:53:05 -0400 Subject: [PATCH 26/38] remove VerifiedUserIdentity requirement -- not used for now --- crates/matrix-sdk-crypto/src/machine/mod.rs | 4 ---- .../src/machine/tests/decryption_verification_state.rs | 7 ------- crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs | 3 --- 3 files changed, 14 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index 86f574a36f3..cd7ad44c6a7 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -1612,10 +1612,6 @@ impl OlmMachine { SenderData::SenderKnown { .. } => Ok(()), _ => self.check_sender_trusted(encryption_info, false), }, - TrustRequirement::VerifiedUserIdentity => match session.sender_data { - SenderData::SenderKnown { master_key_verified: true, .. } => Ok(()), - _ => self.check_sender_trusted(encryption_info, true), - }, } } 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 fefcf088ac2..8fce8d7788e 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 @@ -500,7 +500,6 @@ async fn test_decryption_trust_requirement() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, false), (TrustRequirement::CrossSigned, false), - (TrustRequirement::VerifiedUserIdentity, false), ], ) .await; @@ -519,7 +518,6 @@ async fn test_decryption_trust_requirement() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, true), (TrustRequirement::CrossSigned, false), - (TrustRequirement::VerifiedUserIdentity, false), ], ) .await; @@ -546,7 +544,6 @@ async fn test_decryption_trust_requirement() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, false), (TrustRequirement::CrossSigned, false), - (TrustRequirement::VerifiedUserIdentity, false), ], ) .await; @@ -617,7 +614,6 @@ async fn test_decryption_trust_with_identity_changes() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, true), (TrustRequirement::CrossSigned, true), - (TrustRequirement::VerifiedUserIdentity, false), ], ) .await; @@ -656,7 +652,6 @@ async fn test_decryption_trust_with_identity_changes() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, true), (TrustRequirement::CrossSigned, true), - (TrustRequirement::VerifiedUserIdentity, true), ], ) .await; @@ -703,7 +698,6 @@ async fn test_decryption_trust_with_identity_changes() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, false), (TrustRequirement::CrossSigned, false), - (TrustRequirement::VerifiedUserIdentity, false), ], ) .await; @@ -730,7 +724,6 @@ async fn test_decryption_trust_with_identity_changes() { (TrustRequirement::Untrusted, true), (TrustRequirement::CrossSignedOrLegacy, true), (TrustRequirement::CrossSigned, true), - (TrustRequirement::VerifiedUserIdentity, false), ], ) .await; diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index 97ca482e404..fa3bdfefd30 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -65,9 +65,6 @@ pub enum TrustRequirement { CrossSignedOrLegacy, /// Only decrypt events from cross-signed devices. CrossSigned, - /// Only decrypt events from cross-signed devices where we have verified the - /// user's identity. - VerifiedUserIdentity, } /// Settings for decrypting messages From 773c8feff8bccea5b49155785339159af64f77da Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 21:05:03 -0400 Subject: [PATCH 27/38] move decryption settings types into new file --- crates/matrix-sdk-crypto/src/machine/mod.rs | 7 ++-- .../tests/decryption_verification_state.rs | 5 ++- .../src/machine/tests/mod.rs | 6 ++-- .../src/olm/group_sessions/inbound.rs | 19 ----------- .../src/olm/group_sessions/mod.rs | 4 +-- crates/matrix-sdk-crypto/src/olm/mod.rs | 4 +-- .../matrix-sdk-crypto/src/types/decryption.rs | 34 +++++++++++++++++++ crates/matrix-sdk-crypto/src/types/mod.rs | 1 + 8 files changed, 46 insertions(+), 34 deletions(-) create mode 100644 crates/matrix-sdk-crypto/src/types/decryption.rs diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index cd7ad44c6a7..9d346de0689 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -65,9 +65,9 @@ use crate::{ gossiping::GossipMachine, identities::{user::UserIdentities, Device, IdentityManager, UserDevices}, olm::{ - Account, CrossSigningStatus, DecryptionSettings, EncryptionSettings, IdentityKeys, - InboundGroupSession, OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, - SenderDataFinder, SessionType, StaticAccountData, TrustRequirement, + Account, CrossSigningStatus, EncryptionSettings, IdentityKeys, InboundGroupSession, + OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, SenderDataFinder, SessionType, + StaticAccountData, }, requests::{IncomingResponse, OutgoingRequest, UploadSigningKeysRequest}, session_manager::{GroupSessionManager, SessionManager}, @@ -77,6 +77,7 @@ use crate::{ StoreCache, StoreTransaction, }, types::{ + decryption::{DecryptionSettings, TrustRequirement}, events::{ olm_v1::{AnyDecryptedOlmEvent, DecryptedRoomKeyEvent}, room::encrypted::{ 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 8fce8d7788e..074dc3c094b 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 @@ -40,11 +40,10 @@ use crate::{ testing::response_from_file, tests, }, - olm::{ - DecryptionSettings, InboundGroupSession, OutboundGroupSession, SenderData, TrustRequirement, - }, + olm::{InboundGroupSession, OutboundGroupSession, SenderData}, store::{Changes, IdentityChanges}, types::{ + decryption::{DecryptionSettings, TrustRequirement}, events::{ room::encrypted::{EncryptedEvent, RoomEventEncryptionScheme}, ToDeviceEvent, diff --git a/crates/matrix-sdk-crypto/src/machine/tests/mod.rs b/crates/matrix-sdk-crypto/src/machine/tests/mod.rs index d85a19bb0aa..051f4f07d41 100644 --- a/crates/matrix-sdk-crypto/src/machine/tests/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/tests/mod.rs @@ -61,13 +61,11 @@ use crate::{ }, EncryptionSyncChanges, OlmMachine, }, - olm::{ - BackedUpRoomKey, DecryptionSettings, ExportedRoomKey, SenderData, TrustRequirement, - VerifyJson, - }, + olm::{BackedUpRoomKey, ExportedRoomKey, SenderData, VerifyJson}, session_manager::CollectStrategy, store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore}, types::{ + decryption::{DecryptionSettings, TrustRequirement}, events::{ room::encrypted::{EncryptedToDeviceEvent, ToDeviceEncryptedEventContent}, room_key_withheld::{ diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index fa3bdfefd30..c46747a2c31 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -55,25 +55,6 @@ use crate::{ }, }; -/// The trust level required to decrypt an event. -#[derive(Clone, Copy, Debug, Deserialize, Serialize)] -pub enum TrustRequirement { - /// Decrypt events from everyone regardless of trust. - Untrusted, - /// Only decrypt events from cross-signed or legacy sessions (Megolm - /// sessions created before we started collecting trust information). - CrossSignedOrLegacy, - /// Only decrypt events from cross-signed devices. - CrossSigned, -} - -/// Settings for decrypting messages -#[derive(Clone, Debug, Deserialize, Serialize)] -pub struct DecryptionSettings { - /// The trust level required to decrypt the event - pub trust_requirement: TrustRequirement, -} - // TODO add creation times to the inbound group sessions so we can export // sessions that were created between some time period, this should only be set // for non-imported sessions. diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs index 2b6b691cdfa..33da6296414 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs @@ -20,9 +20,7 @@ mod outbound; mod sender_data; mod sender_data_finder; -pub use inbound::{ - DecryptionSettings, InboundGroupSession, PickledInboundGroupSession, TrustRequirement, -}; +pub use inbound::{InboundGroupSession, PickledInboundGroupSession}; pub(crate) use outbound::ShareState; pub use outbound::{ EncryptionSettings, OutboundGroupSession, PickledOutboundGroupSession, ShareInfo, diff --git a/crates/matrix-sdk-crypto/src/olm/mod.rs b/crates/matrix-sdk-crypto/src/olm/mod.rs index 5a29e5bc409..01bf6f2a1ff 100644 --- a/crates/matrix-sdk-crypto/src/olm/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/mod.rs @@ -26,9 +26,9 @@ mod utility; pub use account::{Account, OlmMessageHash, PickledAccount, StaticAccountData}; pub(crate) use account::{OlmDecryptionInfo, SessionType}; pub use group_sessions::{ - BackedUpRoomKey, DecryptionSettings, EncryptionSettings, ExportedRoomKey, InboundGroupSession, + BackedUpRoomKey, EncryptionSettings, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, PickledInboundGroupSession, PickledOutboundGroupSession, SenderData, - SessionCreationError, SessionExportError, SessionKey, ShareInfo, TrustRequirement, + SessionCreationError, SessionExportError, SessionKey, ShareInfo, }; pub(crate) use group_sessions::{SenderDataFinder, ShareState}; pub use session::{PickledSession, Session}; diff --git a/crates/matrix-sdk-crypto/src/types/decryption.rs b/crates/matrix-sdk-crypto/src/types/decryption.rs new file mode 100644 index 00000000000..28ca934d469 --- /dev/null +++ b/crates/matrix-sdk-crypto/src/types/decryption.rs @@ -0,0 +1,34 @@ +// Copyright 2024 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use serde::{Deserialize, Serialize}; + +/// The trust level required to decrypt an event. +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +pub enum TrustRequirement { + /// Decrypt events from everyone regardless of trust. + Untrusted, + /// Only decrypt events from cross-signed or legacy sessions (Megolm + /// sessions created before we started collecting trust information). + CrossSignedOrLegacy, + /// Only decrypt events from cross-signed devices. + CrossSigned, +} + +/// Settings for decrypting messages +#[derive(Clone, Debug, Deserialize, Serialize)] +pub struct DecryptionSettings { + /// The trust level required to decrypt the event + pub trust_requirement: TrustRequirement, +} diff --git a/crates/matrix-sdk-crypto/src/types/mod.rs b/crates/matrix-sdk-crypto/src/types/mod.rs index c76f0b472bc..f7828dbc2e1 100644 --- a/crates/matrix-sdk-crypto/src/types/mod.rs +++ b/crates/matrix-sdk-crypto/src/types/mod.rs @@ -43,6 +43,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; mod backup; mod cross_signing; +pub mod decryption; mod device_keys; pub mod events; mod one_time_keys; From 4cbfd6ba1347abbba0a24564225a5354e48693ab Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 21:19:22 -0400 Subject: [PATCH 28/38] add doc and fix compilation of other crates --- crates/matrix-sdk-base/src/client.rs | 2 +- crates/matrix-sdk-crypto/src/types/decryption.rs | 2 ++ crates/matrix-sdk-ui/src/timeline/traits.rs | 2 +- crates/matrix-sdk/src/room/mod.rs | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 8e0274a8359..18c86d894d7 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -28,7 +28,7 @@ use futures_util::Stream; use matrix_sdk_common::instant::Instant; #[cfg(feature = "e2e-encryption")] use matrix_sdk_crypto::{ - olm::{DecryptionSettings, TrustRequirement}, + types::decryption::{DecryptionSettings, TrustRequirement}, store::DynCryptoStore, EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine, ToDeviceRequest, }; diff --git a/crates/matrix-sdk-crypto/src/types/decryption.rs b/crates/matrix-sdk-crypto/src/types/decryption.rs index 28ca934d469..b587ff4af00 100644 --- a/crates/matrix-sdk-crypto/src/types/decryption.rs +++ b/crates/matrix-sdk-crypto/src/types/decryption.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +//! Data types for decrypting an event. + use serde::{Deserialize, Serialize}; /// The trust level required to decrypt an event. diff --git a/crates/matrix-sdk-ui/src/timeline/traits.rs b/crates/matrix-sdk-ui/src/timeline/traits.rs index 05ea7ad9fbc..34d3f915add 100644 --- a/crates/matrix-sdk-ui/src/timeline/traits.rs +++ b/crates/matrix-sdk-ui/src/timeline/traits.rs @@ -16,7 +16,7 @@ use async_trait::async_trait; use indexmap::IndexMap; #[cfg(feature = "e2e-encryption")] use matrix_sdk::{ - crypto::olm::{DecryptionSettings, TrustRequirement}, + crypto::types::decryption::{DecryptionSettings, TrustRequirement}, deserialized_responses::TimelineEvent, Result, }; diff --git a/crates/matrix-sdk/src/room/mod.rs b/crates/matrix-sdk/src/room/mod.rs index 4461af18ace..a62f080305e 100644 --- a/crates/matrix-sdk/src/room/mod.rs +++ b/crates/matrix-sdk/src/room/mod.rs @@ -15,7 +15,7 @@ use futures_util::{ stream::FuturesUnordered, }; #[cfg(feature = "e2e-encryption")] -use matrix_sdk_base::crypto::olm::{DecryptionSettings, TrustRequirement}; +use matrix_sdk_base::crypto::types::decryption::{DecryptionSettings, TrustRequirement}; use matrix_sdk_base::{ deserialized_responses::{ RawAnySyncOrStrippedState, RawSyncOrStrippedState, SyncOrStrippedState, TimelineEvent, From 65ba067537fa928e682dd5286841604eb5a7296d Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 21:30:00 -0400 Subject: [PATCH 29/38] lint --- crates/matrix-sdk-base/src/client.rs | 2 +- .../src/deserialized_responses.rs | 15 ++++++++------- .../tests/decryption_verification_state.rs | 6 ++++-- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 18c86d894d7..bc75aff4f6b 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -28,8 +28,8 @@ use futures_util::Stream; use matrix_sdk_common::instant::Instant; #[cfg(feature = "e2e-encryption")] use matrix_sdk_crypto::{ - types::decryption::{DecryptionSettings, TrustRequirement}, store::DynCryptoStore, + types::decryption::{DecryptionSettings, TrustRequirement}, EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine, ToDeviceRequest, }; #[cfg(feature = "e2e-encryption")] diff --git a/crates/matrix-sdk-common/src/deserialized_responses.rs b/crates/matrix-sdk-common/src/deserialized_responses.rs index f80bbd0e581..6f5b6b7ad14 100644 --- a/crates/matrix-sdk-common/src/deserialized_responses.rs +++ b/crates/matrix-sdk-common/src/deserialized_responses.rs @@ -89,12 +89,12 @@ impl VerificationState { match self { VerificationState::Verified => ShieldState::None, VerificationState::Unverified(level) => match level { - VerificationLevel::UnverifiedIdentity | VerificationLevel::ChangedIdentity | VerificationLevel::UnsignedDevice => { - ShieldState::Red { - code: ShieldStateCode::UnverifiedIdentity, - message: UNVERIFIED_IDENTITY, - } - } + VerificationLevel::UnverifiedIdentity + | VerificationLevel::ChangedIdentity + | VerificationLevel::UnsignedDevice => ShieldState::Red { + code: ShieldStateCode::UnverifiedIdentity, + message: UNVERIFIED_IDENTITY, + }, VerificationLevel::None(link) => match link { DeviceLinkProblem::MissingDevice => ShieldState::Red { code: ShieldStateCode::UnknownDevice, @@ -171,7 +171,8 @@ pub enum VerificationLevel { #[error("The sender's identity was not verified")] UnverifiedIdentity, - /// The message was sent by a user whose identity has changed and the change has not yet been confirmed + /// The message was sent by a user whose identity has changed and the change + /// has not yet been confirmed #[error("The sender's identity has changed and the change has not yet been confirmed")] ChangedIdentity, 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 074dc3c094b..314a16f762f 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 @@ -439,7 +439,8 @@ async fn encrypt_message( (event, group_session.session_id().to_string()) } -// Helper function that checks whether a message is decryptable under different trust requirements. +// Helper function that checks whether a message is decryptable under different +// trust requirements. // // `tests` is a list of tuples, where the first element of the tuple is the // trust requirement to check, and the second element indicates whether @@ -655,7 +656,8 @@ async fn test_decryption_trust_with_identity_changes() { ) .await; - // If alice's cross-signing key changes, the event should not be decryptable (except for in unverified mode) + // If alice's cross-signing key changes, the event should not be decryptable + // (except for in unverified mode) let cross_signing_requests = alice.bootstrap_cross_signing(true).await.unwrap(); let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); From e85bf3ba9a939748da3dfbdf2432e714dd1b4998 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 21:40:20 -0400 Subject: [PATCH 30/38] fix bindings and more linting --- bindings/matrix-sdk-crypto-ffi/src/machine.rs | 9 ++++++++- crates/matrix-sdk-crypto/src/machine/mod.rs | 2 +- .../src/machine/tests/decryption_verification_state.rs | 4 ++-- .../src/olm/group_sessions/sender_data_finder.rs | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/bindings/matrix-sdk-crypto-ffi/src/machine.rs b/bindings/matrix-sdk-crypto-ffi/src/machine.rs index 8094244724f..8963768aa37 100644 --- a/bindings/matrix-sdk-crypto-ffi/src/machine.rs +++ b/bindings/matrix-sdk-crypto-ffi/src/machine.rs @@ -17,6 +17,7 @@ use matrix_sdk_crypto::{ decrypt_room_key_export, encrypt_room_key_export, olm::ExportedRoomKey, store::{BackupDecryptionKey, Changes}, + types::decryption::{DecryptionSettings, TrustRequirement}, LocalTrust, OlmMachine as InnerMachine, ToDeviceRequest, UserIdentities, }; use ruma::{ @@ -884,7 +885,13 @@ impl OlmMachine { let event: Raw<_> = serde_json::from_str(&event)?; let room_id = RoomId::parse(room_id)?; - let decrypted = self.runtime.block_on(self.inner.decrypt_room_event(&event, &room_id))?; + let decryption_settings = + DecryptionSettings { trust_requirement: TrustRequirement::Untrusted }; + let decrypted = self.runtime.block_on(self.inner.decrypt_room_event( + &event, + &room_id, + &decryption_settings, + ))?; if handle_verification_events { if let Ok(AnyTimelineEvent::MessageLike(e)) = decrypted.event.deserialize() { diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index 9d346de0689..d99c3aecb83 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -1578,7 +1578,7 @@ impl OlmMachine { encryption_info: &EncryptionInfo, require_verified: bool, ) -> MegolmResult<()> { - match dbg!(&encryption_info.verification_state) { + match &encryption_info.verification_state { VerificationState::Verified => Ok(()), VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => { if require_verified { 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 314a16f762f..3f0f83a17bc 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 @@ -436,7 +436,7 @@ async fn encrypt_message( }); let event = json_convert(&event).unwrap(); - (event, group_session.session_id().to_string()) + (event, group_session.session_id().to_owned()) } // Helper function that checks whether a message is decryptable under different @@ -453,7 +453,7 @@ async fn check_decryption_trust_requirement( ) { for (trust_requirement, is_ok) in tests { let decryption_settings = - DecryptionSettings { trust_requirement: trust_requirement.clone() }; + DecryptionSettings { trust_requirement: *trust_requirement }; if *is_ok { assert!( bob.decrypt_room_event(event, room_id, &decryption_settings).await.is_ok(), 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 f86ecc0cd47..8edfd6855b2 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 @@ -269,7 +269,7 @@ impl<'a> SenderDataFinder<'a> { let master_key = Box::new(master_key); let master_key_verified = sender_device.is_cross_signing_trusted(); let identity_needs_user_approval = !master_key_verified - && owner_identity.other().and_then(|i| Some(i.has_pin_violation())).unwrap_or(true); + && owner_identity.other().map(|i| Some(i.has_pin_violation())).unwrap_or(true); SenderData::SenderKnown { user_id, device_id, From 14d1e0778e3161c3ba7c1a6a75db662236b55c01 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 21:43:09 -0400 Subject: [PATCH 31/38] fix type --- .../src/olm/group_sessions/sender_data_finder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8edfd6855b2..2e634e03cf4 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 @@ -269,7 +269,7 @@ impl<'a> SenderDataFinder<'a> { let master_key = Box::new(master_key); let master_key_verified = sender_device.is_cross_signing_trusted(); let identity_needs_user_approval = !master_key_verified - && owner_identity.other().map(|i| Some(i.has_pin_violation())).unwrap_or(true); + && owner_identity.other().map(|i| i.has_pin_violation()).unwrap_or(true); SenderData::SenderKnown { user_id, device_id, From e5c0c0626eefe2be461d1bcd6748b8de05e4f074 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 8 Aug 2024 21:47:19 -0400 Subject: [PATCH 32/38] more lint --- .../src/machine/tests/decryption_verification_state.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 3f0f83a17bc..8f7606e6424 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 @@ -452,8 +452,7 @@ async fn check_decryption_trust_requirement( tests: &[(TrustRequirement, bool)], ) { for (trust_requirement, is_ok) in tests { - let decryption_settings = - DecryptionSettings { trust_requirement: *trust_requirement }; + let decryption_settings = DecryptionSettings { trust_requirement: *trust_requirement }; if *is_ok { assert!( bob.decrypt_room_event(event, room_id, &decryption_settings).await.is_ok(), From 24a3ebbef776a9ca54c77c22a1878e641353e719 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 14 Aug 2024 13:35:59 -0400 Subject: [PATCH 33/38] only error on identity change if sender was previously verified --- .../src/deserialized_responses.rs | 26 ++++++++------ crates/matrix-sdk-crypto/src/machine/mod.rs | 4 +-- .../src/olm/group_sessions/sender_data.rs | 11 ++++-- .../olm/group_sessions/sender_data_finder.rs | 34 +++++++++---------- 4 files changed, 42 insertions(+), 33 deletions(-) diff --git a/crates/matrix-sdk-common/src/deserialized_responses.rs b/crates/matrix-sdk-common/src/deserialized_responses.rs index 6f5b6b7ad14..21fbbf529aa 100644 --- a/crates/matrix-sdk-common/src/deserialized_responses.rs +++ b/crates/matrix-sdk-common/src/deserialized_responses.rs @@ -31,6 +31,7 @@ const UNVERIFIED_IDENTITY: &str = "Encrypted by an unverified user."; const UNSIGNED_DEVICE: &str = "Encrypted by a device not verified by its owner."; const UNKNOWN_DEVICE: &str = "Encrypted by an unknown or deleted device."; pub const SENT_IN_CLEAR: &str = "Not encrypted."; +const PREVIOUSLY_VERIFIED: &str = "Encrypted by a previously-verified user."; /// Represents the state of verification for a decrypted message sent by a /// device. @@ -90,7 +91,7 @@ impl VerificationState { VerificationState::Verified => ShieldState::None, VerificationState::Unverified(level) => match level { VerificationLevel::UnverifiedIdentity - | VerificationLevel::ChangedIdentity + | VerificationLevel::PreviouslyVerified | VerificationLevel::UnsignedDevice => ShieldState::Red { code: ShieldStateCode::UnverifiedIdentity, message: UNVERIFIED_IDENTITY, @@ -123,14 +124,15 @@ impl VerificationState { VerificationLevel::UnverifiedIdentity => { // If you didn't show interest in verifying that user we don't // nag you with an error message. - // TODO: We should detect identity rotation of a previously trusted identity and - // then warn see https://github.com/matrix-org/matrix-rust-sdk/issues/1129 ShieldState::None } - VerificationLevel::ChangedIdentity => { - // As above, if you didn't show interest in verifying that - // user we don't nag you - ShieldState::None + VerificationLevel::PreviouslyVerified => { + // This is a high warning. The sender was previously + // verified, but changed their identity. + ShieldState::Red { + code: ShieldStateCode::PreviouslyVerified, + message: PREVIOUSLY_VERIFIED, + } } VerificationLevel::UnsignedDevice => { // This is a high warning. The sender hasn't verified his own device. @@ -171,10 +173,10 @@ pub enum VerificationLevel { #[error("The sender's identity was not verified")] UnverifiedIdentity, - /// The message was sent by a user whose identity has changed and the change - /// has not yet been confirmed - #[error("The sender's identity has changed and the change has not yet been confirmed")] - ChangedIdentity, + /// The message was sent by a user who was previously verified, but changed + /// their identity, and we have not yet verified the new identity. + #[error("The sender's identity was previously verified but has changed")] + PreviouslyVerified, /// The message was sent by a device not linked to (signed by) any user /// identity. @@ -241,6 +243,8 @@ pub enum ShieldStateCode { UnverifiedIdentity, /// An unencrypted event in an encrypted room. SentInClear, + /// The sender was previously verified but changed their identity. + PreviouslyVerified, } /// The algorithm specific information of a decrypted event. diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index d99c3aecb83..da653674fbd 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -2426,10 +2426,10 @@ fn sender_data_to_verification_state( ), SenderData::SenderKnown { master_key_verified: false, - identity_needs_user_approval: true, + previously_verified: true, device_id, .. - } => (VerificationState::Unverified(VerificationLevel::ChangedIdentity), device_id), + } => (VerificationState::Unverified(VerificationLevel::PreviouslyVerified), device_id), SenderData::SenderKnown { master_key_verified: false, device_id, .. } => { (VerificationState::Unverified(VerificationLevel::UnverifiedIdentity), device_id) } 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 8325a28d130..e6b1034821f 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 @@ -76,7 +76,12 @@ pub enum SenderData { master_key_verified: bool, #[serde(default)] - identity_needs_user_approval: bool, + /// Whether the user's master key was previously verified, but the + /// current master key is not. + /// + /// TODO: should this be merged with the master_key_verified field and + /// turned into an enum? + previously_verified: bool, }, } @@ -111,14 +116,14 @@ impl SenderData { device_id: &DeviceId, master_key: Ed25519PublicKey, master_key_verified: bool, - identity_needs_user_approval: bool, + previously_verified: bool, ) -> Self { Self::SenderKnown { user_id: user_id.to_owned(), device_id: Some(device_id.to_owned()), master_key: Box::new(master_key), master_key_verified, - identity_needs_user_approval, + previously_verified, } } 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 2e634e03cf4..b792dd96982 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 @@ -268,14 +268,14 @@ impl<'a> SenderDataFinder<'a> { // We have user_id and master_key for the user sending the to-device message. let master_key = Box::new(master_key); let master_key_verified = sender_device.is_cross_signing_trusted(); - let identity_needs_user_approval = !master_key_verified - && owner_identity.other().map(|i| i.has_pin_violation()).unwrap_or(true); + let previously_verified = !master_key_verified + && owner_identity.other().map(|i| i.was_previously_verified()).unwrap_or(false); SenderData::SenderKnown { user_id, device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } } else { // Surprisingly, there was no key in the MasterPubkey. We did not expect this: @@ -531,14 +531,14 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); assert_eq!(device_id.unwrap(), setup.sender_device.device_id()); assert_eq!(*master_key, setup.sender_master_key()); assert!(!master_key_verified); - assert!(identity_needs_user_approval); + assert!(!previously_verified); } #[async_test] @@ -566,14 +566,14 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); assert_eq!(device_id.unwrap(), setup.sender_device.device_id()); assert_eq!(*master_key, setup.sender_master_key()); assert!(!master_key_verified); - assert!(!identity_needs_user_approval); + assert!(!previously_verified); } #[async_test] @@ -602,14 +602,14 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); assert_eq!(device_id.unwrap(), setup.sender_device.device_id()); assert_eq!(*master_key, setup.sender_master_key()); assert!(!master_key_verified); - assert!(identity_needs_user_approval); + assert!(!previously_verified); } #[async_test] @@ -637,14 +637,14 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); assert_eq!(device_id.unwrap(), setup.sender_device.device_id()); assert_eq!(*master_key, setup.sender_master_key()); assert!(!master_key_verified); - assert!(!identity_needs_user_approval); + assert!(!previously_verified); } #[async_test] @@ -747,7 +747,7 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); @@ -755,7 +755,7 @@ mod tests { assert_eq!(*master_key, setup.sender_master_key()); // Including the fact that it was verified assert!(master_key_verified); - assert!(!identity_needs_user_approval); + assert!(!previously_verified); } #[async_test] @@ -786,7 +786,7 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); @@ -794,7 +794,7 @@ mod tests { assert_eq!(*master_key, setup.sender_master_key()); // Including the fact that it was verified assert!(master_key_verified); - assert!(!identity_needs_user_approval); + assert!(!previously_verified); } #[async_test] @@ -816,14 +816,14 @@ mod tests { device_id, master_key, master_key_verified, - identity_needs_user_approval, + previously_verified, } = sender_data ); assert_eq!(user_id, setup.sender.user_id); assert_eq!(device_id.unwrap(), setup.sender_device.device_id()); assert_eq!(*master_key, setup.sender_master_key()); assert!(!master_key_verified); - assert!(!identity_needs_user_approval); + assert!(!previously_verified); } struct TestOptions { From a119d79458ea18870c4b97f7cff7cdb73f51edfd Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 14 Aug 2024 16:49:39 -0400 Subject: [PATCH 34/38] fix logic and fix test --- crates/matrix-sdk-crypto/src/machine/mod.rs | 32 ++-- .../tests/decryption_verification_state.rs | 158 ++++-------------- 2 files changed, 49 insertions(+), 141 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index da653674fbd..8c4cce8644e 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -1573,20 +1573,10 @@ impl OlmMachine { /// cross-signed, and that the sender's identity is pinned. If /// `require_verified` is `true`, then also checks if we have verified the /// sender's identity - fn check_sender_trusted( - &self, - encryption_info: &EncryptionInfo, - require_verified: bool, - ) -> MegolmResult<()> { + fn check_sender_trusted(&self, encryption_info: &EncryptionInfo) -> MegolmResult<()> { match &encryption_info.verification_state { VerificationState::Verified => Ok(()), - VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => { - if require_verified { - Err(MegolmError::SenderIdentity(VerificationLevel::UnverifiedIdentity)) - } else { - Ok(()) - } - } + VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => Ok(()), VerificationState::Unverified(verification_level) => { Err(MegolmError::SenderIdentity(verification_level.clone())) } @@ -1603,15 +1593,25 @@ impl OlmMachine { ) -> MegolmResult<()> { match decryption_settings.trust_requirement { TrustRequirement::Untrusted => Ok(()), - TrustRequirement::CrossSignedOrLegacy => match session.sender_data { + TrustRequirement::CrossSignedOrLegacy => match &session.sender_data { + SenderData::SenderKnown { + master_key_verified: false, + previously_verified: true, + .. + } => Err(MegolmError::SenderIdentity(VerificationLevel::PreviouslyVerified)), SenderData::SenderKnown { .. } => Ok(()), SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), - _ => self.check_sender_trusted(encryption_info, false), + _ => self.check_sender_trusted(encryption_info), }, - TrustRequirement::CrossSigned => match session.sender_data { + TrustRequirement::CrossSigned => match &session.sender_data { + SenderData::SenderKnown { + master_key_verified: false, + previously_verified: true, + .. + } => Err(MegolmError::SenderIdentity(VerificationLevel::PreviouslyVerified)), SenderData::SenderKnown { .. } => Ok(()), - _ => self.check_sender_trusted(encryption_info, false), + _ => self.check_sender_trusted(encryption_info), }, } } 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 8f7606e6424..9288b10fd33 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 @@ -483,46 +483,12 @@ async fn test_decryption_trust_requirement() { // Set the sender data to various values, and test that we can or can't // decrypt, depending on what the trust requirement is. - // An unknown non-legacy session should be decryptable only when the trust - // requirement allows untrusted sessions let mut session = bob.store().get_inbound_group_session(room_id, &session_id).await.unwrap().unwrap(); - session.sender_data = - SenderData::UnknownDevice { legacy_session: false, owner_check_failed: false }; - bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); - - check_decryption_trust_requirement( - &bob, - &event, - room_id, - &[ - (TrustRequirement::Untrusted, true), - (TrustRequirement::CrossSignedOrLegacy, false), - (TrustRequirement::CrossSigned, false), - ], - ) - .await; - - // An unknown legacy session should be decryptable only when the trust - // requirement allows untrusted or legacy sessions - session.sender_data = - SenderData::UnknownDevice { legacy_session: true, owner_check_failed: false }; - bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); - - check_decryption_trust_requirement( - &bob, - &event, - room_id, - &[ - (TrustRequirement::Untrusted, true), - (TrustRequirement::CrossSignedOrLegacy, true), - (TrustRequirement::CrossSigned, false), - ], - ) - .await; - // A session where we have the device keys but no cross-signing - // information should be just like an unknown device + // A session where we have the device keys but no cross-signing information + // should be decryptable only when the trust requirement allows untrusted or + // legacy sessions session.sender_data = SenderData::DeviceInfo { device_keys: alice .get_device(alice.user_id(), alice.device_id(), None) @@ -546,29 +512,9 @@ async fn test_decryption_trust_requirement() { ], ) .await; -} -#[async_test] -async fn test_decryption_trust_with_identity_changes() { - let (alice, bob) = get_machine_pair_with_setup_sessions_test_helper( - tests::alice_id(), - tests::user_id(), - false, - ) - .await; + // Bob later gets Alice's identity and cross-signed device keys bob.bootstrap_cross_signing(false).await.unwrap(); - let room_id = room_id!("!test:example.org"); - let (event, session_id) = encrypt_message(&alice, room_id, &bob, "Secret message").await; - - // Bob receives the message but Alice's keys are unknown at the time of - // reception - let mut session = - bob.store().get_inbound_group_session(room_id, &session_id).await.unwrap().unwrap(); - session.sender_data = - SenderData::UnknownDevice { legacy_session: false, owner_check_failed: false }; - bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); - - // Bob later gets Alice's device keys and identity let cross_signing_requests = alice.bootstrap_cross_signing(false).await.unwrap(); let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); @@ -604,7 +550,7 @@ async fn test_decryption_trust_with_identity_changes() { .unwrap(); // Since the sending device is now cross-signed by Alice, it should be - // decryptable in all modes except for verified. + // decryptable in all modes check_decryption_trust_requirement( &bob, &event, @@ -616,48 +562,23 @@ async fn test_decryption_trust_with_identity_changes() { ], ) .await; +} - // If we verify Alice, the event should be decryptable in verified mode - let mut alice_identity = - bob.store().get_identity(alice.user_id()).await.unwrap().unwrap().other().unwrap(); - let signature_upload_req = alice_identity.verify().await.unwrap(); - let alice_read_only_identity = &mut alice_identity.inner; - let mut alice_msk_json = serde_json::to_value(alice_msk).unwrap(); - for (_, value) in signature_upload_req.signed_keys.get(alice.user_id()).unwrap().iter() { - let value: serde_json::Value = serde_json::from_str(value.get()).unwrap(); - alice_msk_json["signatures"] - .as_object_mut() - .unwrap() - .insert(bob.user_id().to_string(), value["signatures"][bob.user_id().as_str()].clone()); - } - let alice_msk: MasterPubkey = serde_json::from_value(alice_msk_json).unwrap(); - alice_read_only_identity.update(alice_msk.clone(), alice_ssk.clone(), None).unwrap(); - bob.store() - .save_changes(Changes { - identities: IdentityChanges { - new: vec![alice_read_only_identity.clone().into()], - ..Default::default() - }, - ..Default::default() - }) - .await - .unwrap(); - - check_decryption_trust_requirement( - &bob, - &event, - room_id, - &[ - (TrustRequirement::Untrusted, true), - (TrustRequirement::CrossSignedOrLegacy, true), - (TrustRequirement::CrossSigned, true), - ], +#[async_test] +async fn test_decryption_trust_with_identity_change() { + let (alice, bob) = get_machine_pair_with_setup_sessions_test_helper( + tests::alice_id(), + tests::user_id(), + false, ) .await; + bob.bootstrap_cross_signing(false).await.unwrap(); + let room_id = room_id!("!test:example.org"); + let (event, session_id) = encrypt_message(&alice, room_id, &bob, "Secret message").await; - // If alice's cross-signing key changes, the event should not be decryptable - // (except for in unverified mode) - let cross_signing_requests = alice.bootstrap_cross_signing(true).await.unwrap(); + // Simulate Alice's cross-signing key changing after having been verified by + // setting the `previously_verified` flag + let cross_signing_requests = alice.bootstrap_cross_signing(false).await.unwrap(); let upload_signing_keys_req = cross_signing_requests.upload_signing_keys_req; let alice_msk: MasterPubkey = upload_signing_keys_req.master_key.unwrap().try_into().unwrap(); let alice_ssk: SelfSigningPubkey = @@ -678,18 +599,31 @@ async fn test_decryption_trust_with_identity_changes() { .unwrap()]) .await .unwrap(); - alice_read_only_identity.update(alice_msk.clone(), alice_ssk.clone(), None).unwrap(); bob.store() .save_changes(Changes { identities: IdentityChanges { - new: vec![alice_read_only_identity.clone().into()], + new: vec![OtherUserIdentityData::new(alice_msk.clone(), alice_ssk.clone()) + .unwrap() + .into()], ..Default::default() }, ..Default::default() }) .await .unwrap(); + let alice_identity = + bob.store().get_identity(alice.user_id()).await.unwrap().unwrap().other().unwrap(); + alice_identity.mark_as_previously_verified().await.unwrap(); + // Bob receives the Megolm session and message + let mut session = + bob.store().get_inbound_group_session(room_id, &session_id).await.unwrap().unwrap(); + session.sender_data = + SenderData::UnknownDevice { legacy_session: false, owner_check_failed: false }; + bob.store().save_inbound_group_sessions(&[session.clone()]).await.unwrap(); + + // In this case, the message should not be decryptable except for when we + // accept untrusted devices check_decryption_trust_requirement( &bob, &event, @@ -701,30 +635,4 @@ async fn test_decryption_trust_with_identity_changes() { ], ) .await; - - // ... until we acknowledge the change, at which point it should be - // decryptable in every mode except for verified - alice_read_only_identity.pin(); - bob.store() - .save_changes(Changes { - identities: IdentityChanges { - new: vec![alice_read_only_identity.clone().into()], - ..Default::default() - }, - ..Default::default() - }) - .await - .unwrap(); - - check_decryption_trust_requirement( - &bob, - &event, - room_id, - &[ - (TrustRequirement::Untrusted, true), - (TrustRequirement::CrossSignedOrLegacy, true), - (TrustRequirement::CrossSigned, true), - ], - ) - .await; } From 368699acb0d574145942456c948832da36000a39 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 15 Aug 2024 13:12:06 -0400 Subject: [PATCH 35/38] fix clippy error --- crates/matrix-sdk-ui/src/timeline/traits.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/traits.rs b/crates/matrix-sdk-ui/src/timeline/traits.rs index 34d3f915add..9a67b2a9a79 100644 --- a/crates/matrix-sdk-ui/src/timeline/traits.rs +++ b/crates/matrix-sdk-ui/src/timeline/traits.rs @@ -16,10 +16,11 @@ use async_trait::async_trait; use indexmap::IndexMap; #[cfg(feature = "e2e-encryption")] use matrix_sdk::{ - crypto::types::decryption::{DecryptionSettings, TrustRequirement}, deserialized_responses::TimelineEvent, Result, }; +#[cfg(all(test, feature = "e2e-encryption"))] +use matrix_sdk::crypto::types::decryption::{DecryptionSettings, TrustRequirement}, use matrix_sdk::{event_cache::paginator::PaginableRoom, Room}; use matrix_sdk_base::latest_event::LatestEvent; #[cfg(feature = "e2e-encryption")] From 0a1f80bfc0abe81bbbcb3b56663c193a58609e40 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 15 Aug 2024 14:14:12 -0400 Subject: [PATCH 36/38] use the right punctuation --- crates/matrix-sdk-ui/src/timeline/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/traits.rs b/crates/matrix-sdk-ui/src/timeline/traits.rs index 9a67b2a9a79..714465f30a3 100644 --- a/crates/matrix-sdk-ui/src/timeline/traits.rs +++ b/crates/matrix-sdk-ui/src/timeline/traits.rs @@ -20,7 +20,7 @@ use matrix_sdk::{ Result, }; #[cfg(all(test, feature = "e2e-encryption"))] -use matrix_sdk::crypto::types::decryption::{DecryptionSettings, TrustRequirement}, +use matrix_sdk::crypto::types::decryption::{DecryptionSettings, TrustRequirement}; use matrix_sdk::{event_cache::paginator::PaginableRoom, Room}; use matrix_sdk_base::latest_event::LatestEvent; #[cfg(feature = "e2e-encryption")] From 04c81e3cb2a6a46f7dd50fd5d288db092ea74515 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 15 Aug 2024 14:41:07 -0400 Subject: [PATCH 37/38] fmt --- crates/matrix-sdk-ui/src/timeline/traits.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/traits.rs b/crates/matrix-sdk-ui/src/timeline/traits.rs index 714465f30a3..19827c104a9 100644 --- a/crates/matrix-sdk-ui/src/timeline/traits.rs +++ b/crates/matrix-sdk-ui/src/timeline/traits.rs @@ -14,13 +14,10 @@ use async_trait::async_trait; use indexmap::IndexMap; -#[cfg(feature = "e2e-encryption")] -use matrix_sdk::{ - deserialized_responses::TimelineEvent, - Result, -}; #[cfg(all(test, feature = "e2e-encryption"))] use matrix_sdk::crypto::types::decryption::{DecryptionSettings, TrustRequirement}; +#[cfg(feature = "e2e-encryption")] +use matrix_sdk::{deserialized_responses::TimelineEvent, Result}; use matrix_sdk::{event_cache::paginator::PaginableRoom, Room}; use matrix_sdk_base::latest_event::LatestEvent; #[cfg(feature = "e2e-encryption")] From 0e9b5811a10abfcbe443d6cf104bb95fe401bd7b Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 15 Aug 2024 16:36:51 -0400 Subject: [PATCH 38/38] add/fix comments and add changelog --- crates/matrix-sdk-crypto/CHANGELOG.md | 3 +++ crates/matrix-sdk-crypto/src/machine/mod.rs | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index 6e36dc038b7..b9daff888c7 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -32,6 +32,9 @@ Changes: Breaking changes: +- `OlmMachine::decrypt_room_event` now takes an `EncryptionSettings` argument. + ([#3701](https://github.com/matrix-org/matrix-rust-sdk/pull/3701)) + - Add a new `error_on_verified_user_problem` property to `CollectStrategy::DeviceBasedStrategy`, which, when set, causes `OlmMachine::share_room_key` to fail with an error if any verified users on diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index b96a0d5b277..c2b378713cb 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -1567,16 +1567,18 @@ impl OlmMachine { self.get_encryption_info(&session, &event.sender).await } - /// Check whether the sender of a Megolm session is trusted. + /// Check whether the sender of a Megolm session is trusted, based on the + /// verification state. /// - /// Checks that the device is cross-signed, that the sender's identity is - /// cross-signed, and that the sender's identity is pinned. If - /// `require_verified` is `true`, then also checks if we have verified the - /// sender's identity + /// This is used by `check_sender_trust_requirement`, and ensures that the + /// sending device is cross-signed. fn check_sender_trusted(&self, encryption_info: &EncryptionInfo) -> MegolmResult<()> { match &encryption_info.verification_state { + // Device is cross-signed, and identity is verified VerificationState::Verified => Ok(()), + // Device is cross-signed, but identity is not verified VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => Ok(()), + // Device is not cross-signed VerificationState::Unverified(verification_level) => { Err(MegolmError::SenderIdentity(verification_level.clone())) } @@ -1593,7 +1595,10 @@ impl OlmMachine { ) -> MegolmResult<()> { match decryption_settings.trust_requirement { TrustRequirement::Untrusted => Ok(()), + TrustRequirement::CrossSignedOrLegacy => match &session.sender_data { + // Reject if the sender was previously verified, but changed + // their identity and is not verified any more. SenderData::SenderKnown { master_key_verified: false, previously_verified: true, @@ -1604,7 +1609,10 @@ impl OlmMachine { SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), _ => self.check_sender_trusted(encryption_info), }, + TrustRequirement::CrossSigned => match &session.sender_data { + // Reject if the sender was previously verified, but changed + // their identity and is not verified any more. SenderData::SenderKnown { master_key_verified: false, previously_verified: true,