diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index f11051d95db..48704801976 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -391,7 +391,7 @@ pub enum SessionRecipientCollectionError { /// /// Happens only with [`CollectStrategy::DeviceBasedStrategy`] when /// [`error_on_verified_user_problem`](`CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem`) - /// is true. + /// is true, or with [`CollectStrategy::IdentityBasedStrategy`]. /// /// In order to resolve this, the user can: /// @@ -407,4 +407,15 @@ pub enum SessionRecipientCollectionError { /// The caller can then retry the encryption operation. #[error("one or more users that were verified have changed their identity")] VerifiedUserChangedIdentity(Vec), + + /// Cross-signing is required for encryption when using + /// [`CollectStrategy::IdentityBased`]. + #[error("Encryption failed because cross-signing is not setup on your account")] + CrossSigningNotSetup, + + /// The current device needs to be verified when encrypting using + /// [`CollectStrategy::IdentityBased`]. Apps should prevent sending in the + /// UI to avoid hitting this case. + #[error("Encryption failed because your device is not verified")] + SendingFromUnverifiedDevice, } 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 5c426d0d425..bab3af7a24a 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 @@ -120,6 +120,7 @@ pub(crate) async fn collect_session_recipients( let users: BTreeSet<&UserId> = users.collect(); let mut devices: BTreeMap> = Default::default(); let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); + let mut verified_users_with_new_identities: Vec = Default::default(); trace!(?users, ?settings, "Calculating group session recipients"); @@ -145,6 +146,8 @@ 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()); + // Get the recipient and withheld devices, based on the collection strategy. match settings.sharing_strategy { CollectStrategy::DeviceBasedStrategy { @@ -153,10 +156,6 @@ pub(crate) async fn collect_session_recipients( } => { let mut unsigned_devices_of_verified_users: BTreeMap> = Default::default(); - let mut verified_users_with_new_identities: Vec = Default::default(); - - let own_identity = - store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); for user_id in users { trace!("Considering recipient devices for user {}", user_id); @@ -233,24 +232,39 @@ pub(crate) async fn collect_session_recipients( ), )); } - - // Alternatively, we may have encountered previously-verified users who have - // changed their identities. We bail out for that, too. - if !verified_users_with_new_identities.is_empty() { - return Err(OlmError::SessionRecipientCollectionError( - SessionRecipientCollectionError::VerifiedUserChangedIdentity( - verified_users_with_new_identities, - ), - )); - } } CollectStrategy::IdentityBasedStrategy => { + // We require our own cross-signing to be properly set-up for the + // identity-based strategy, so return an error if it isn't. + match &own_identity { + None => { + return Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::CrossSigningNotSetup, + )) + } + Some(identity) if !identity.is_verified() => { + return Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::SendingFromUnverifiedDevice, + )) + } + Some(_) => (), + } + for user_id in users { trace!("Considering recipient devices for user {}", user_id); let user_devices = store.get_device_data_for_user_filtered(user_id).await?; let device_owner_identity = store.get_user_identity(user_id).await?; + if has_identity_verification_violation( + own_identity.as_ref(), + device_owner_identity.as_ref(), + ) { + verified_users_with_new_identities.push(user_id.to_owned()); + // No point considering the individual devices of this user. + continue; + } + let recipient_devices = split_recipients_withhelds_for_user_based_on_identity( user_devices, &device_owner_identity, @@ -277,6 +291,16 @@ pub(crate) async fn collect_session_recipients( } } + // We may have encountered previously-verified users who have changed their + // identities. If so, we bail out with an error. + if !verified_users_with_new_identities.is_empty() { + return Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::VerifiedUserChangedIdentity( + verified_users_with_new_identities, + ), + )); + } + if should_rotate { debug!( should_rotate, @@ -491,10 +515,14 @@ fn is_user_verified( mod tests { use std::{collections::BTreeMap, iter, sync::Arc}; + use assert_matches::assert_matches; use assert_matches2::assert_let; use matrix_sdk_test::{ async_test, test_json, - test_json::keys_query_sets::{KeyDistributionTestData, PreviouslyVerifiedTestData}, + test_json::keys_query_sets::{ + IdentityChangeDataSet, KeyDistributionTestData, MaloIdentityChangeDataSet, + PreviouslyVerifiedTestData, + }, }; use ruma::{ device_id, events::room::history_visibility::HistoryVisibility, room_id, TransactionId, @@ -1122,6 +1150,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, + iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await; + + assert_matches!( + request_result, + Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::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, + iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await; + + assert_matches!( + request_result, + Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::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, + 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::SessionRecipientCollectionError( + SessionRecipientCollectionError::VerifiedUserChangedIdentity(affected_users) + )) = request_result + ); + assert_eq!(2, affected_users.len()); + + // This can be resolved by withdrawing verification. + for user_id in affected_users { + machine + .get_identity(&user_id, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .withdraw_verification() + .await + .unwrap() + } + + // And now the key share should succeed. + machine + .share_room_key( + fake_room_id, + 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 ea12ff899ef..4fa38e100a6 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 @@ -1263,3 +1263,151 @@ impl PreviouslyVerifiedTestData { ruma_response_from_json(&data) } } + +/// 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") + } + + /// @malo's keys before their identity change + pub fn initial_key_query() -> KeyQueryResponse { + let data = 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": {}, + }); + + ruma_response_from_json(&data) + } + + /// @malo's keys after their identity change + pub fn updated_key_query() -> KeyQueryResponse { + let data = 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": {} + }); + + ruma_response_from_json(&data) + } +}