crypto: Error when sending keys to previously-verified users with identity-based strategy

bindings/matrix-sdk-ffi/CHANGELOG.md

@@ -2,10 +2,14 @@
 
 Breaking changes:
 
-- `EventSendState` now has two additional variants: `VerifiedUserHasUnsignedDevice` and
-  `VerifiedUserChangedIdentity`. These reflect problems with verified users in the room
-  and as such can only be returned when the room key recipient strategy has
-  `error_on_verified_user_problem` set.
+- `EventSendState` now has four additional variants: `VerifiedUserHasUnsignedDevice`,
+  `VerifiedUserChangedIdentity`, `CrossSigningNotSetup`, and
+  `SendingFromUnverifiedDevice`. The first two reflect problems with verified users in
+  the room and as such can only be returned when the room key recipient strategy has
+  `error_on_verified_user_problem` set, or when using the identity-based strategy. The
+  last two indicate that your own device is not properly cross-signed, which is a
+  requirement when using the identity-based strategy, and can only be returned when
+  using the identity-based strategy.
 
 - The `AuthenticationService` has been removed:
     - Instead of calling `configure_homeserver`, build your own client with the `serverNameOrHomeserverUrl` builder

bindings/matrix-sdk-ffi/src/timeline/mod.rs

@@ -918,12 +918,21 @@ pub enum EventSendState {
     ///
     /// Happens only when the room key recipient strategy (as set by
     /// [`ClientBuilder::room_key_recipient_strategy`]) has
-    /// [`error_on_verified_user_problem`](CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem) set.
+    /// [`error_on_verified_user_problem`](CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem)
+    /// set, or when using [`CollectStrategy::IdentityBasedStrategy`].
     VerifiedUserChangedIdentity {
         /// The users that were previously verified, but are no longer
         users: Vec<String>,
     },
 
+    /// The user does not have cross-signing set up, but
+    /// [`CollectStrategy::IdentityBasedStrategy`] was used.
+    CrossSigningNotSetup,
+
+    /// The current device is not verified, but
+    /// [`CollectStrategy::IdentityBasedStrategy`] was used.
+    SendingFromUnverifiedDevice,
+
     /// The local event has been sent to the server, but unsuccessfully: The
     /// sending has failed.
     SendingFailed {
@@ -977,6 +986,10 @@ fn event_send_state_from_sending_failed(error: &Error, is_recoverable: bool) ->
             VerifiedUserChangedIdentity(bad_users) => EventSendState::VerifiedUserChangedIdentity {
                 users: bad_users.iter().map(|user_id| user_id.to_string()).collect(),
             },
+
+            CrossSigningNotSetup => EventSendState::CrossSigningNotSetup,
+
+            SendingFromUnverifiedDevice => EventSendState::SendingFromUnverifiedDevice,
         },
 
         _ => EventSendState::SendingFailed { error: error.to_string(), is_recoverable },

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -56,13 +56,19 @@ Breaking changes:
   `OlmMachine::share_room_key` to fail with an error if any verified users on
   the recipient list have unsigned devices, or are no lonver verified.
 
+  When `CallectStrategy::IdentityBasedStrategy` is used,
+  `OlmMachine::share_room_key` will fail with an error if any verified users on
+  the recipient list are no longer verified, or if our own device is not
+  properly cross-signed.
+
   Also remove `CollectStrategy::new_device_based`: callers should construct a
   `CollectStrategy::DeviceBasedStrategy` directly.
 
   `EncryptionSettings::new` now takes a `CollectStrategy` argument, instead of
   a list of booleans.
   ([#3810](https://github.com/matrix-org/matrix-rust-sdk/pull/3810))
   ([#3816](https://github.com/matrix-org/matrix-rust-sdk/pull/3816))
+  ([#3896](https://github.com/matrix-org/matrix-rust-sdk/pull/3896))
 
 - Remove the method `OlmMachine::clear_crypto_cache()`, crypto stores are not
   supposed to have any caches anymore.

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<OwnedUserId>),
+
+    /// Cross-signing is required for encryption when using
+    /// [`CollectStrategy::IdentityBasedStrategy`].
+    #[error("Encryption failed because cross-signing is not set up on your account")]
+    CrossSigningNotSetup,
+
+    /// The current device needs to be verified when encrypting using
+    /// [`CollectStrategy::IdentityBasedStrategy`]. Apps should prevent sending
+    /// in the UI to avoid hitting this case.
+    #[error("Encryption failed because your device is not verified")]
+    SendingFromUnverifiedDevice,
 }

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<OwnedUserId, Vec<DeviceData>> = Default::default();
     let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default();
+    let mut verified_users_with_new_identities: Vec<OwnedUserId> = 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<OwnedUserId, Vec<OwnedDeviceId>> =
                 Default::default();
-            let mut verified_users_with_new_identities: Vec<OwnedUserId> = 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;