diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index 7e0adc2aca2..c355c69ff33 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -70,6 +70,35 @@ pub enum OlmError { have a valid Olm session with us" )] MissingSession, + + #[error(transparent)] + /// The room key that was to be shared was not shared because the sharing + /// strategy could not be fulfilled. + RoomKeySharingStrategyError(RoomKeySharingStrategyError), +} + +/// Depending on the sharing strategy for room keys, the distribution of the +/// room key could fail. +#[derive(Error, Debug)] +pub enum RoomKeySharingStrategyError { + /// When encrypting using the IdentityBased strategy, and there are changed + /// user identities that have not been confirmed by the user. The + /// application should display identity changes to the user as soon as + /// possible to avoid hitting this case. If this happens the app might just + /// retry automatically after the identity change has been notified, or + /// offer option to cancel. + #[error("Encryption failed because changed user identities have not been confirmed, please confirm or verify the problematic user identities")] + UnconfirmedIdentities(Vec), + + /// Cross-signing is required for encryption with invisible crypto + #[error("Encryption failed because cross-signing is not setup 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 because your device is not verified")] + SendingFromUnverifiedDevice, } /// Error representing a failure during a group encryption operation. 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..64f6dda09a3 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, RoomKeySharingStrategyError}, + 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 @@ -98,8 +100,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,75 +125,119 @@ 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<(DeviceData, 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_device_data_for_user_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()); + + let own_verified_identity = match maybe_own_identity { + None => { + return Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::CrossSigningNotSetup, + )) + } + Some(identity) if !identity.is_verified() => { + return Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::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 device_owner_identity = store.get_user_identity(user_id).await?; - split_recipients_withhelds_for_user_based_on_identity( + + if let Some(other_identity) = device_owner_identity.as_ref().and_then(|i| i.other()) + { + if other_identity.has_pin_violation() + && other_identity.was_previously_verified() + && 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, - ) - } - }; - - 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 { - // 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, + ); + + // 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(recipients); - withheld_devices.extend(withheld_recipients); - } + devices + .entry(user_id.to_owned()) + .or_default() + .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::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::UnconfirmedIdentities( + users_with_identity_mismatch, + ), + )); + } + } + }; if should_rotate { debug!( @@ -209,6 +253,44 @@ 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: &[DeviceData], + 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<&DeviceId> = shared.keys().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)>, @@ -281,16 +363,24 @@ mod tests { use std::sync::Arc; - use matrix_sdk_test::{async_test, test_json::keys_query_sets::KeyDistributionTestData}; + use assert_matches::assert_matches; + use assert_matches2::assert_let; + 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::{ + error::RoomKeySharingStrategyError, olm::OutboundGroupSession, session_manager::{ 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 { @@ -528,6 +618,187 @@ mod tests { assert_eq!(code, &WithheldCode::Unauthorised); } + #[async_test] + async fn test_share_identity_strategy_no_cross_signing() { + // Test key sharing with the identity-based strategy with different + // states of our own verification. + + // Starting off, we have not yet set up our own cross-signing, so + // sharing with the identity-based strategy should fail. + let machine: OlmMachine = OlmMachine::new( + KeyDistributionTestData::me_id(), + KeyDistributionTestData::me_device_id(), + ) + .await; + + let keys_query = KeyDistributionTestData::dan_keys_query_response(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let encryption_settings = EncryptionSettings { + sharing_strategy: CollectStrategy::new_identity_based(), + ..Default::default() + }; + + let request_result = machine + .share_room_key( + fake_room_id, + std::iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await; + + assert_matches!( + request_result, + Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::CrossSigningNotSetup + )) + ); + + // We now get our public cross-signing keys, but we don't trust them + // yet. In this case, sharing the keys should still fail since our own + // device is still unverified. + let keys_query = KeyDistributionTestData::me_keys_query_response(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + let request_result = machine + .share_room_key( + fake_room_id, + std::iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await; + + assert_matches!( + request_result, + Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::SendingFromUnverifiedDevice + )) + ); + + // Finally, after we trust our own cross-signing keys, key sharing + // should succeed. + 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 requests = machine + .share_room_key( + fake_room_id, + std::iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await + .unwrap(); + + // Dan has two devices, but only one is cross-signed, so there should + // only be one key share. + assert_eq!(requests.len(), 1); + } + + #[async_test] + async fn test_share_identity_strategy_report_pinning_violation() { + // Test that identity-based key sharing gives an error when a verified + // user changes their identity. + 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(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + let keys_query = MaloIdentityChangeDataSet::initial_key_query(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + // Simulate pinning violation, in which case the key sharing should fail. + let keys_query = IdentityChangeDataSet::key_query_with_identity_b(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + machine + .get_identity(IdentityChangeDataSet::user_id(), None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .mark_as_previously_verified() + .await + .unwrap(); + + let keys_query = MaloIdentityChangeDataSet::updated_key_query(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + machine + .get_identity(MaloIdentityChangeDataSet::user_id(), None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .mark_as_previously_verified() + .await + .unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let encryption_settings = EncryptionSettings { + sharing_strategy: CollectStrategy::new_identity_based(), + ..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_let!( + Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::UnconfirmedIdentities(affected_users) + )) = request_result + ); + assert_eq!(2, affected_users.len()); + + // This can be resolved by pinning the new identities. + for user_id in affected_users { + machine + .get_identity(&user_id, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .pin_current_master_key() + .await + .unwrap() + } + + // And now the key share should succeed. + machine + .share_room_key( + fake_room_id, + std::iter::once(IdentityChangeDataSet::user_id()), + encryption_settings.clone(), + ) + .await + .unwrap(); + } + #[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 cfa290427dc..e39b5ea33a8 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 @@ -1088,3 +1088,149 @@ impl PreviouslyVerifiedTestData { .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") + } +}