From d8d9dae9d77bee48a2591b9aad9bd2fa466354cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Tue, 16 Jul 2024 16:02:39 +0200 Subject: [PATCH] crypto: Fix UserIdentity::is_verified to take into account our own identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `UserIdentity::is_verified()` method in the matrix-sdk-crypto crate before version 0.7.2 doesn't take into account the verification status of the user's own identity while performing the check and may as a result return a value contrary to what is implied by its name and documentation. This patch fixes this and adds a regression test. The method itself is not used internally and as such has not a larger impact. Co-authored-by: Denis Kasak Signed-off-by: Damir Jelić --- .../matrix-sdk-crypto/src/identities/user.rs | 95 ++++++++++++++++++- crates/matrix-sdk-crypto/src/machine.rs | 4 +- 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 406c906808a..9f56a97c878 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -226,7 +226,11 @@ impl Deref for UserIdentity { impl UserIdentity { /// Is this user identity verified. pub fn is_verified(&self) -> bool { - self.own_identity.as_ref().is_some_and(|o| o.is_identity_signed(&self.inner).is_ok()) + self.own_identity.as_ref().is_some_and(|own_identity| { + // The identity of another user is verified iff our own identity is verified and + // if our own identity has signed the other user's identity. + own_identity.is_verified() && own_identity.is_identity_signed(&self.inner).is_ok() + }) } /// Manually verify this user. @@ -778,7 +782,7 @@ pub(crate) mod tests { use assert_matches::assert_matches; use matrix_sdk_test::async_test; - use ruma::{device_id, user_id}; + use ruma::{device_id, user_id, UserId}; use serde_json::{json, Value}; use tokio::sync::Mutex; @@ -788,10 +792,15 @@ pub(crate) mod tests { }; use crate::{ identities::{manager::testing::own_key_query, Device}, + machine::tests::{ + get_machine_pair, mark_alice_identity_as_verified_test_helper, + setup_cross_signing_for_machine_test_helper, + }, olm::{Account, PrivateCrossSigningIdentity}, - store::{CryptoStoreWrapper, MemoryStore}, + store::{Changes, CryptoStoreWrapper, MemoryStore}, types::{CrossSigningKey, MasterPubkey, SelfSigningPubkey, Signatures, UserSigningPubkey}, verification::VerificationMachine, + OlmMachine, }; #[test] @@ -1006,4 +1015,84 @@ pub(crate) mod tests { [second_device_id] ); } + + async fn get_machine_pair_with_signed_identities( + alice: &UserId, + bob: &UserId, + ) -> (OlmMachine, OlmMachine) { + let (alice, bob, _) = get_machine_pair(alice, bob, false).await; + setup_cross_signing_for_machine_test_helper(&alice, &bob).await; + mark_alice_identity_as_verified_test_helper(&alice, &bob).await; + + (alice, bob) + } + + #[async_test] + async fn test_other_user_is_verified_if_my_identity_is_verified_and_they_are_cross_signed() { + let alice_user_id = user_id!("@alice:localhost"); + let bob_user_id = user_id!("@bob:localhost"); + let (alice, bob) = + get_machine_pair_with_signed_identities(alice_user_id, bob_user_id).await; + + let bobs_own_identity = + bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); + let bobs_alice_identity = + bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + + assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); + assert!(bobs_alice_identity.is_verified(), "Alice's identity should be verified as well."); + } + + #[async_test] + async fn test_other_user_is_not_verified_if_they_are_not_cross_signed() { + let alice_user_id = user_id!("@alice:localhost"); + let bob_user_id = user_id!("@bob:localhost"); + let (alice, bob, _) = get_machine_pair(alice_user_id, bob_user_id, false).await; + setup_cross_signing_for_machine_test_helper(&alice, &bob).await; + + let bobs_own_identity = + bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); + let bobs_alice_identity = + bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + + assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); + assert!(!bobs_alice_identity.is_verified(), "Alice's identity should not be considered verified since Bob has not signed it."); + } + + #[async_test] + async fn test_other_user_is_not_verified_if_my_identity_is_not_verified() { + let alice_user_id = user_id!("@alice:localhost"); + let bob_user_id = user_id!("@bob:localhost"); + + let (alice, bob, _) = get_machine_pair(alice_user_id, bob_user_id, false).await; + setup_cross_signing_for_machine_test_helper(&alice, &bob).await; + mark_alice_identity_as_verified_test_helper(&alice, &bob).await; + + let bobs_own_identity = + bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); + let bobs_alice_identity = + bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + + assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); + assert!(bobs_alice_identity.is_verified(), "Alice's identity should be verified as well."); + + bobs_own_identity.mark_as_unverified(); + + bob.store() + .save_changes(Changes { + identities: crate::store::IdentityChanges { + changed: vec![bobs_own_identity.inner.clone().into()], + ..Default::default() + }, + ..Default::default() + }) + .await + .unwrap(); + + assert!(!bobs_own_identity.is_verified(), "Bob's identity should not be verified anymore."); + assert!( + !bobs_alice_identity.is_verified(), + "Alice's identity should not be verified either." + ); + } } diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index a84745ec4f6..98ad6b218fa 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -3223,7 +3223,7 @@ pub(crate) mod tests { ); } - async fn setup_cross_signing_for_machine_test_helper(alice: &OlmMachine, bob: &OlmMachine) { + pub async fn setup_cross_signing_for_machine_test_helper(alice: &OlmMachine, bob: &OlmMachine) { let CrossSigningBootstrapRequests { upload_signing_keys_req: alice_upload_signing, .. } = alice.bootstrap_cross_signing(false).await.expect("Expect Alice x-signing key request"); @@ -3344,7 +3344,7 @@ pub(crate) mod tests { bob.receive_keys_query_response(&TransactionId::new(), &kq_response).await.unwrap(); } - async fn mark_alice_identity_as_verified_test_helper(alice: &OlmMachine, bob: &OlmMachine) { + pub async fn mark_alice_identity_as_verified_test_helper(alice: &OlmMachine, bob: &OlmMachine) { let alice_device = bob.get_device(alice.user_id(), alice.device_id(), None).await.unwrap().unwrap();