Repository: matrix-org/matrix-rust-sdk (rust)

Issue:
`UserIdentity::has_verification_violation` does not work for our own identity
https://github.com/matrix-org/matrix-rust-sdk/pull/3795 added a new method `UserIdentity::has_verification_violation`.

However, it appears to have omitted support for tracking verification of the user's *own* identity. This means that if our own identity was rotated, and a new device added, signed by the new identity, we would share messages to that device despite there being no evidence it is not controlled by an attacker.

This will become less of a problem in future, when we refuse to do any operations without our own identity being verified, but in the meantime it is a security hole, and, for as long as matrix-sdk-crypto has support for sending messages without verifying our identity, it will remain a footgun. I think it therefore needs fixing as a prerequisite to https://github.com/matrix-org/matrix-rust-sdk/issues/3793.

Pull request:
crypto: Remember whether our own identity has been verified
A follow-up to #3795, which extends the same principle to `OwnUserIdentity`.

Fixes #3845.

Patch:
diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md
index 6e36dc038..84fdf3e90 100644
--- a/crates/matrix-sdk-crypto/CHANGELOG.md
+++ b/crates/matrix-sdk-crypto/CHANGELOG.md
@@ -32,6 +32,16 @@ Changes:
 
 Breaking changes:
 
+- Expose new methods `OwnUserIdentity::was_previously_verified`,
+  `OwnUserIdentity::withdraw_verification`, and
+  `OwnUserIdentity::has_verification_violation`, which track whether our own
+  identity was previously verified.
+  ([#3846](https://github.com/matrix-org/matrix-rust-sdk/pull/3846))
+
+  **NOTE**: this causes changes to the format of the serialised data in the
+  CryptoStore, meaning that, once upgraded, it will not be possible to roll
+  back applications to earlier versions without breaking user sessions.
+
 - 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/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs
index 112401ce6..34473ee97 100644
--- a/crates/matrix-sdk-crypto/src/identities/user.rs
+++ b/crates/matrix-sdk-crypto/src/identities/user.rs
@@ -29,11 +29,10 @@ use ruma::{
     },
     DeviceId, EventId, OwnedDeviceId, OwnedUserId, RoomId, UserId,
 };
-use serde::{Deserialize, Serialize};
+use serde::{Deserialize, Deserializer, Serialize};
 use serde_json::Value;
 use tracing::error;
 
-use super::{atomic_bool_deserializer, atomic_bool_serializer};
 use crate::{
     error::SignatureError,
     store::{Changes, IdentityChanges, Store},
@@ -200,6 +199,18 @@ impl OwnUserIdentity {
             methods,
         ))
     }
+
+    /// Remove the requirement for this identity to be verified.
+    pub async fn withdraw_verification(&self) -> Result<(), CryptoStoreError> {
+        self.inner.withdraw_verification();
+        let to_save = UserIdentityData::Own(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(())
+    }
 }
 
 /// Struct representing a cross signing identity of a user.
@@ -781,11 +792,21 @@ pub struct OwnUserIdentityData {
     master_key: Arc<MasterPubkey>,
     self_signing_key: Arc<SelfSigningPubkey>,
     user_signing_key: Arc<UserSigningPubkey>,
-    #[serde(
-        serialize_with = "atomic_bool_serializer",
-        deserialize_with = "atomic_bool_deserializer"
-    )]
-    verified: Arc<AtomicBool>,
+    #[serde(deserialize_with = "deserialize_own_user_identity_data_verified")]
+    verified: Arc<RwLock<OwnUserIdentityVerifiedState>>,
+}
+
+#[derive(Default, Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+enum OwnUserIdentityVerifiedState {
+    /// We have never verified our own identity
+    #[default]
+    NeverVerified,
+
+    /// We previously verified this identity, but it has changed.
+    PreviouslyVerifiedButNoLonger,
+
+    /// We have verified the current identity.
+    Verified,
 }
 
 impl PartialEq for OwnUserIdentityData {
@@ -806,7 +827,7 @@ impl PartialEq for OwnUserIdentityData {
             && self.master_key == other.master_key
             && self.self_signing_key == other.self_signing_key
             && self.user_signing_key == other.user_signing_key
-            && self.is_verified() == other.is_verified()
+            && *self.verified.read().unwrap() == *other.verified.read().unwrap()
             && self.master_key.signatures() == other.master_key.signatures()
     }
 }
@@ -838,7 +859,7 @@ impl OwnUserIdentityData {
             master_key: master_key.into(),
             self_signing_key: self_signing_key.into(),
             user_signing_key: user_signing_key.into(),
-            verified: Arc::new(AtomicBool::new(false)),
+            verified: Default::default(),
         })
     }
 
@@ -855,7 +876,7 @@ impl OwnUserIdentityData {
             master_key: master_key.into(),
             self_signing_key: self_signing_key.into(),
             user_signing_key: user_signing_key.into(),
-            verified: Arc::new(AtomicBool::new(false)),
+            verified: Default::default(),
         }
     }
 
@@ -926,17 +947,58 @@ impl OwnUserIdentityData {
 
     /// Mark our identity as verified.
     pub fn mark_as_verified(&self) {
-        self.verified.store(true, Ordering::SeqCst)
+        *self.verified.write().unwrap() = OwnUserIdentityVerifiedState::Verified;
     }
 
-    #[cfg(test)]
-    pub fn mark_as_unverified(&self) {
-        self.verified.store(false, Ordering::SeqCst)
+    /// Mark our identity as unverified.
+    pub(crate) fn mark_as_unverified(&self) {
+        let mut guard = self.verified.write().unwrap();
+        if *guard == OwnUserIdentityVerifiedState::Verified {
+            *guard = OwnUserIdentityVerifiedState::PreviouslyVerifiedButNoLonger;
+        }
     }
 
     /// Check if our identity is verified.
     pub fn is_verified(&self) -> bool {
-        self.verified.load(Ordering::SeqCst)
+        *self.verified.read().unwrap() == OwnUserIdentityVerifiedState::Verified
+    }
+
+    /// True if we verified our own identity at some point in the past.
+    ///
+    /// To reset this latch back to `false`, one must call
+    /// [`OwnUserIdentityData::withdraw_verification()`].
+    pub fn was_previously_verified(&self) -> bool {
+        matches!(
+            *self.verified.read().unwrap(),
+            OwnUserIdentityVerifiedState::Verified
+                | OwnUserIdentityVerifiedState::PreviouslyVerifiedButNoLonger
+        )
+    }
+
+    /// Remove the requirement for this identity to be verified.
+    ///
+    /// If an identity was previously verified and is not any more it will be
+    /// reported to the user. In order to remove this notice users have to
+    /// verify again or to withdraw the verification requirement.
+    pub fn withdraw_verification(&self) {
+        let mut guard = self.verified.write().unwrap();
+        if *guard == OwnUserIdentityVerifiedState::PreviouslyVerifiedButNoLonger {
+            *guard = OwnUserIdentityVerifiedState::NeverVerified;
+        }
+    }
+
+    /// Was this identity previously verified, and is no longer?
+    ///
+    /// Such a violation should be reported to the local user by the
+    /// application, and resolved by
+    ///
+    /// - Verifying the new identity with
+    ///   [`OwnUserIdentity::request_verification`]
+    /// - Or by withdrawing the verification requirement
+    ///   [`OwnUserIdentity::withdraw_verification`].
+    pub fn has_verification_violation(&self) -> bool {
+        *self.verified.read().unwrap()
+            == OwnUserIdentityVerifiedState::PreviouslyVerifiedButNoLonger
     }
 
     /// Update the identity with a new master key and self signing key.
@@ -969,7 +1031,7 @@ impl OwnUserIdentityData {
         self.user_signing_key = user_signing_key.into();
 
         if self.master_key.as_ref() != &master_key {
-            self.verified.store(false, Ordering::SeqCst);
+            self.mark_as_unverified()
         }
 
         self.master_key = master_key.into();
@@ -991,6 +1053,31 @@ impl OwnUserIdentityData {
     }
 }
 
+/// Custom deserializer for [`OwnUserIdentityData::verified`].
+///
+/// This used to be a bool, so we need to handle that.
+fn deserialize_own_user_identity_data_verified<'de, D>(
+    de: D,
+) -> Result<Arc<RwLock<OwnUserIdentityVerifiedState>>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    #[derive(Deserialize)]
+    #[serde(untagged)]
+    enum VerifiedStateOrBool {
+        VerifiedState(OwnUserIdentityVerifiedState),
+        Bool(bool),
+    }
+
+    let verified_state = match VerifiedStateOrBool::deserialize(de)? {
+        VerifiedStateOrBool::Bool(true) => OwnUserIdentityVerifiedState::Verified,
+        VerifiedStateOrBool::Bool(false) => OwnUserIdentityVerifiedState::NeverVerified,
+        VerifiedStateOrBool::VerifiedState(x) => x,
+    };
+
+    Ok(Arc::new(RwLock::new(verified_state)))
+}
+
 /// Testing Facilities
 #[cfg(any(test, feature = "testing"))]
 #[allow(dead_code)]
@@ -1079,7 +1166,8 @@ pub(crate) mod tests {
 
     use super::{
         testing::{device, get_other_identity, get_own_identity},
-        OtherUserIdentityDataSerializerV2, OwnUserIdentityData, UserIdentityData,
+        OtherUserIdentityDataSerializerV2, OwnUserIdentityData, OwnUserIdentityVerifiedState,
+        UserIdentityData,
     };
     use crate::{
         identities::{
@@ -1200,6 +1288,39 @@ pub(crate) mod tests {
         assert_eq!("2", with_serializer.version.unwrap());
     }
 
+    /// [`OwnUserIdentityData::verified`] was previously an AtomicBool. Check
+    /// that we can deserialize boolean values.
+    #[test]
+    fn test_deserialize_own_user_identity_bool_verified() {
+        let mut json = json!({
+            "user_id": "@example:localhost",
+            "master_key": {
+                "user_id":"@example:localhost",
+                "usage":["master"],
+                "keys":{"ed25519:rJ2TAGkEOP6dX41Ksll6cl8K3J48l8s/59zaXyvl2p0":"rJ2TAGkEOP6dX41Ksll6cl8K3J48l8s/59zaXyvl2p0"},
+            },
+            "self_signing_key": {
+                "user_id":"@example:localhost",
+                "usage":["self_signing"],
+                "keys":{"ed25519:0C8lCBxrvrv/O7BQfsKnkYogHZX3zAgw3RfJuyiq210":"0C8lCBxrvrv/O7BQfsKnkYogHZX3zAgw3RfJuyiq210"}
+            },
+            "user_signing_key": {
+                "user_id":"@example:localhost",
+                "usage":["user_signing"],
+                "keys":{"ed25519:DU9z4gBFKFKCk7a13sW9wjT0Iyg7Hqv5f0BPM7DEhPo":"DU9z4gBFKFKCk7a13sW9wjT0Iyg7Hqv5f0BPM7DEhPo"}
+            },
+            "verified": false
+        });
+
+        let id: OwnUserIdentityData = serde_json::from_value(json.clone()).unwrap();
+        assert_eq!(*id.verified.read().unwrap(), OwnUserIdentityVerifiedState::NeverVerified);
+
+        // Tweak the json to have `"verified": true`, and repeat
+        *json.get_mut("verified").unwrap() = true.into();
+        let id: OwnUserIdentityData = serde_json::from_value(json.clone()).unwrap();
+        assert_eq!(*id.verified.read().unwrap(), OwnUserIdentityVerifiedState::Verified);
+    }
+
     #[test]
     fn own_identity_check_signatures() {
         let response = own_key_query();
@@ -1509,4 +1630,53 @@ pub(crate) mod tests {
 
         assert!(bob_identity.has_verification_violation());
     }
+
+    /// Test that receiving new public keys for our own identity causes a
+    /// verification violation on our own identity.
+    #[async_test]
+    async fn own_keys_update_creates_own_identity_verification_violation() {
+        use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet;
+
+        let machine = OlmMachine::new(DataSet::own_id(), device_id!("LOCAL")).await;
+
+        // Start with our own identity verified
+        let own_keys = DataSet::own_keys_query_response_1();
+        machine.mark_request_as_sent(&TransactionId::new(), &own_keys).await.unwrap();
+
+        machine
+            .import_cross_signing_keys(CrossSigningKeyExport {
+                master_key: DataSet::MASTER_KEY_PRIVATE_EXPORT.to_owned().into(),
+                self_signing_key: DataSet::SELF_SIGNING_KEY_PRIVATE_EXPORT.to_owned().into(),
+                user_signing_key: DataSet::USER_SIGNING_KEY_PRIVATE_EXPORT.to_owned().into(),
+            })
+            .await
+            .unwrap();
+
+        // Double-check that we have a verified identity
+        let own_identity = machine.get_identity(DataSet::own_id(), None).await.unwrap().unwrap();
+        let own_identity = own_identity.own().unwrap();
+        assert!(own_identity.is_verified());
+        assert!(own_identity.was_previously_verified());
+        assert!(!own_identity.has_verification_violation());
+
+        // Now, we receive a *different* set of public keys
+        let own_keys = DataSet::own_keys_query_response_2();
+        machine.mark_request_as_sent(&TransactionId::new(), &own_keys).await.unwrap();
+
+        // That should give an identity that is no longer verified, with a verification
+        // violation.
+        let own_identity = machine.get_identity(DataSet::own_id(), None).await.unwrap().unwrap();
+        let own_identity = own_identity.own().unwrap();
+        assert!(!own_identity.is_verified());
+        assert!(own_identity.was_previously_verified());
+        assert!(own_identity.has_verification_violation());
+
+        // Now check that we can withdraw verification for our own identity, and that it
+        // becomes valid again.
+        own_identity.withdraw_verification().await.unwrap();
+
+        assert!(!own_identity.is_verified());
+        assert!(!own_identity.was_previously_verified());
+        assert!(!own_identity.has_verification_violation());
+    }
 }


Test patch:
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 cc17f1929..ea12ff899 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
@@ -774,6 +774,48 @@ impl PreviouslyVerifiedTestData {
         ruma_response_from_json(&data)
     }
 
+    /// A second `/keys/query` response for Alice, containing a *different* set
+    /// of public cross-signing keys.
+    ///
+    /// This response was lifted from the test data set from `matrix-js-sdk`.
+    pub fn own_keys_query_response_2() -> KeyQueryResponse {
+        let data = json!({
+            "master_keys": {
+                "@alice:localhost": {
+                    "keys": { "ed25519:J+5An10v1vzZpAXTYFokD1/PEVccFnLC61EfRXit0UY": "J+5An10v1vzZpAXTYFokD1/PEVccFnLC61EfRXit0UY" },
+                    "user_id": "@alice:localhost",
+                    "usage": [ "master" ]
+                }
+            },
+            "self_signing_keys": {
+                "@alice:localhost": {
+                    "keys": { "ed25519:aU2+2CyXQTCuDcmWW0EL2bhJ6PdjFW2LbAsbHqf02AY": "aU2+2CyXQTCuDcmWW0EL2bhJ6PdjFW2LbAsbHqf02AY" },
+                    "user_id": "@alice:localhost",
+                    "usage": [ "self_signing" ],
+                    "signatures": {
+                        "@alice:localhost": {
+                            "ed25519:J+5An10v1vzZpAXTYFokD1/PEVccFnLC61EfRXit0UY": "XfhYEhZmOs8BJdb3viatILBZ/bElsHXEW28V4tIaY5CxrBR0YOym3yZHWmRmypXessHZAKOhZn3yBMXzdajyCw"
+                        }
+                    }
+                }
+            },
+            "user_signing_keys": {
+                "@alice:localhost": {
+                    "keys": { "ed25519:g5TC/zjQXyZYuDLZv7a41z5fFVrXpYPypG//AFQj8hY": "g5TC/zjQXyZYuDLZv7a41z5fFVrXpYPypG//AFQj8hY" },
+                    "user_id": "@alice:localhost",
+                    "usage": [ "user_signing" ],
+                    "signatures": {
+                        "@alice:localhost": {
+                            "ed25519:J+5An10v1vzZpAXTYFokD1/PEVccFnLC61EfRXit0UY": "6AkD1XM2H0/ebgP9oBdMKNeft7uxsrb0XN1CsjjHgeZCvCTMmv3BHlLiT/Hzy4fe8H+S1tr484dcXN/PIdnfDA"
+                        }
+                    }
+                }
+            }
+        });
+
+        ruma_response_from_json(&data)
+    }
+
     /// Device ID of the device returned by [`Self::own_unsigned_device_keys`].
     pub fn own_unsigned_device_id() -> OwnedDeviceId {
         Self::own_unsigned_device_keys().0