From 8fbd9a4a342badd77441f0a5be112f9fd1525657 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 8 Jul 2024 14:45:04 +0200 Subject: [PATCH] feat(crypto): Key distribution errors for Pin violations --- crates/matrix-sdk-crypto/src/error.rs | 26 ++ .../group_sessions/share_strategy.rs | 224 +++++++++++++++++- .../src/test_json/keys_query_sets.rs | 146 ++++++++++++ 3 files changed, 389 insertions(+), 7 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index cedb9712da5..d08f1601a1f 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 5ecd361abbf..f9f986b5b72 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, - EncryptionSettings, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, + error::{OlmResult, RoomKeyDistributionError}, + store::Store, + types::events::room_key_withheld::WithheldCode, + EncryptionSettings, OlmError, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, }; /// Strategy to collect the devices that should receive room keys for the @@ -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_readonly_devices_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: &[ReadOnlyDevice], outbound: &OutboundGroupSession, ) -> bool { // Device IDs that should receive this session @@ -324,7 +363,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::{ @@ -333,7 +377,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 { @@ -571,6 +615,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 755369902bf..21684038262 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 @@ -669,3 +669,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") + } +}