From 356506060c7ef17ab986f32863d1137dac71f82a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Thu, 12 Aug 2021 14:06:45 +0200 Subject: [PATCH] crypto: Remove private cross signing keys if we detect that they changed --- matrix_sdk_crypto/src/identities/manager.rs | 54 +++++--- matrix_sdk_crypto/src/olm/signing/mod.rs | 129 ++++++++++++++++---- matrix_sdk_crypto/src/store/mod.rs | 4 + 3 files changed, 148 insertions(+), 39 deletions(-) diff --git a/matrix_sdk_crypto/src/identities/manager.rs b/matrix_sdk_crypto/src/identities/manager.rs index 60d83899..d8563383 100644 --- a/matrix_sdk_crypto/src/identities/manager.rs +++ b/matrix_sdk_crypto/src/identities/manager.rs @@ -24,7 +24,7 @@ use ruma::{ api::client::r0::keys::get_keys::Response as KeysQueryResponse, encryption::DeviceKeys, DeviceId, DeviceIdBox, UserId, }; -use tracing::{trace, warn}; +use tracing::{info, trace, warn}; use crate::{ error::OlmResult, @@ -32,6 +32,7 @@ use crate::{ MasterPubkey, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, ReadOnlyUserIdentity, SelfSigningPubkey, UserSigningPubkey, }, + olm::PrivateCrossSigningIdentity, requests::KeysQueryRequest, store::{Changes, DeviceChanges, IdentityChanges, Result as StoreResult, Store}, }; @@ -75,11 +76,13 @@ impl IdentityManager { ) -> OlmResult<(DeviceChanges, IdentityChanges)> { let changed_devices = self.handle_devices_from_key_query(response.device_keys.clone()).await?; - let changed_identities = self.handle_cross_singing_keys(response).await?; + let (changed_identities, cross_signing_identity) = + self.handle_cross_singing_keys(response).await?; let changes = Changes { identities: changed_identities.clone(), devices: changed_devices.clone(), + private_identity: cross_signing_identity, ..Default::default() }; @@ -234,8 +237,9 @@ impl IdentityManager { async fn handle_cross_singing_keys( &self, response: &KeysQueryResponse, - ) -> StoreResult { + ) -> StoreResult<(IdentityChanges, Option)> { let mut changes = IdentityChanges::default(); + let mut changed_identity = None; for (user_id, master_key) in &response.master_keys { let master_key = MasterPubkey::from(master_key); @@ -243,7 +247,10 @@ impl IdentityManager { let self_signing = if let Some(s) = response.self_signing_keys.get(user_id) { SelfSigningPubkey::from(s) } else { - warn!("User identity for user {} didn't contain a self signing pubkey", user_id); + warn!( + user_id = user_id.as_str(), + "A user identity didn't contain a self signing pubkey" + ); continue; }; @@ -255,9 +262,9 @@ impl IdentityManager { UserSigningPubkey::from(s) } else { warn!( - "User identity for our own user {} didn't \ - contain a user signing pubkey", - user_id + user_id = user_id.as_str(), + "User identity for our own user didn't \ + contain a user signing pubkey", ); continue; }; @@ -277,8 +284,8 @@ impl IdentityManager { || user_signing.user_id() != user_id { warn!( - "User id mismatch in one of the cross signing keys for user {}", - user_id + user_id = user_id.as_str(), + "User ID mismatch in one of the cross signing keys", ); continue; } @@ -287,14 +294,14 @@ impl IdentityManager { .map(|i| (ReadOnlyUserIdentities::Own(i), true)) } else { warn!( - "User identity for our own user {} didn't contain a \ + user_id = user_id.as_str(), + "User identity for our own user didn't contain a \ user signing pubkey", - user_id ); continue; } } else if master_key.user_id() != user_id || self_signing.user_id() != user_id { - warn!("User id mismatch in one of the cross signing keys for user {}", user_id); + warn!(user = user_id.as_str(), "User ID mismatch in one of the cross signing keys",); continue; } else { ReadOnlyUserIdentity::new(master_key, self_signing) @@ -303,21 +310,38 @@ impl IdentityManager { match result { Ok((i, new)) => { - trace!("Updated or created new user identity for {}: {:?}", user_id, i); + if let Some(identity) = i.own() { + let private_identity = self.store.private_identity(); + let private_identity = private_identity.lock().await; + + let result = private_identity.clear_if_differs(identity).await; + + if result.any_cleared() { + changed_identity = Some((&*private_identity).clone()); + info!(cleared =? result, "Removed some or all of our private cross signing keys"); + } + } + if new { + trace!(user_id = user_id.as_str(), identity =? i, "Created new user identity"); changes.new.push(i); } else { + trace!(user_id = user_id.as_str(), identity =? i, "Updated a user identity"); changes.changed.push(i); } } Err(e) => { - warn!("Couldn't update or create new user identity for {}: {:?}", user_id, e); + warn!( + user_id = user_id.as_str(), + error =? e, + "Couldn't update or create new user identity for" + ); continue; } } } - Ok(changes) + Ok((changes, changed_identity)) } /// Get a key query request if one is needed. diff --git a/matrix_sdk_crypto/src/olm/signing/mod.rs b/matrix_sdk_crypto/src/olm/signing/mod.rs index bb67c4cc..dcd8fd18 100644 --- a/matrix_sdk_crypto/src/olm/signing/mod.rs +++ b/matrix_sdk_crypto/src/olm/signing/mod.rs @@ -34,9 +34,13 @@ use serde::{Deserialize, Serialize}; use serde_json::Error as JsonError; use crate::{ - error::SignatureError, identities::MasterPubkey, requests::UploadSigningKeysRequest, - store::SecretImportError, utilities::decode, OwnUserIdentity, ReadOnlyAccount, ReadOnlyDevice, - ReadOnlyOwnUserIdentity, ReadOnlyUserIdentity, + error::SignatureError, + identities::{MasterPubkey, SelfSigningPubkey, UserSigningPubkey}, + requests::UploadSigningKeysRequest, + store::SecretImportError, + utilities::decode, + OwnUserIdentity, ReadOnlyAccount, ReadOnlyDevice, ReadOnlyOwnUserIdentity, + ReadOnlyUserIdentity, }; /// Private cross signing identity. @@ -57,6 +61,26 @@ pub struct PrivateCrossSigningIdentity { pub(crate) self_signing_key: Arc>>, } +/// A struct containing information if any of our cross signing keys were +/// cleared because the public keys differ from the keys that are uploaded to +/// the server. +#[derive(Debug, Clone)] +pub struct ClearResult { + /// Was the master key cleared. + master_cleared: bool, + /// Was the self-signing key cleared. + self_signing_cleared: bool, + /// Was the user-signing key cleared. + user_signing_cleared: bool, +} + +impl ClearResult { + /// Did we clear any of the private cross signing keys. + pub fn any_cleared(&self) -> bool { + self.master_cleared || self.self_signing_cleared || self.user_signing_cleared + } +} + /// The pickled version of a `PrivateCrossSigningIdentity`. /// /// Can be used to store the identity. @@ -137,6 +161,16 @@ impl PrivateCrossSigningIdentity { self.master_key.lock().await.as_ref().map(|m| m.public_key.to_owned()) } + /// Get the public part of the self-signing key, if we have one. + pub async fn self_signing_public_key(&self) -> Option { + self.self_signing_key.lock().await.as_ref().map(|k| k.public_key.to_owned()) + } + + /// Get the public part of the user-signing key, if we have one. + pub async fn user_signing_public_key(&self) -> Option { + self.user_signing_key.lock().await.as_ref().map(|k| k.public_key.to_owned()) + } + /// Export the seed of the private cross signing key /// /// The exported seed will be encoded as unpadded base64. @@ -190,7 +224,6 @@ impl PrivateCrossSigningIdentity { if public_identity.master_key() == &master.public_key { Ok(Some(master)) - // *self.master_key.lock().await = Some(master); } else { Err(SecretImportError::MissmatchedPublicKeys) } @@ -203,7 +236,6 @@ impl PrivateCrossSigningIdentity { let subkey = UserSigning::from_seed(self.user_id().clone(), seed); if public_identity.user_signing_key() == &subkey.public_key { - // *self.user_signing_key.lock().await = Some(subkey); Ok(Some(subkey)) } else { Err(SecretImportError::MissmatchedPublicKeys) @@ -217,7 +249,6 @@ impl PrivateCrossSigningIdentity { let subkey = SelfSigning::from_seed(self.user_id().clone(), seed); if public_identity.self_signing_key() == &subkey.public_key { - // *self.self_signing_key.lock().await = Some(subkey); Ok(Some(subkey)) } else { Err(SecretImportError::MissmatchedPublicKeys) @@ -241,6 +272,58 @@ impl PrivateCrossSigningIdentity { Ok(()) } + /// Remove our private cross signing key if the public keys differ from + /// what's found in the `ReadOnlyOwnUserIdentity`. + pub(crate) async fn clear_if_differs( + &self, + public_identity: &ReadOnlyOwnUserIdentity, + ) -> ClearResult { + let result = self.get_public_identity_diff(public_identity).await; + + if result.master_cleared { + *self.master_key.lock().await = None; + } + + if result.user_signing_cleared { + *self.user_signing_key.lock().await = None; + } + + if result.self_signing_cleared { + *self.self_signing_key.lock().await = None; + } + + result + } + + async fn get_public_identity_diff( + &self, + public_identity: &ReadOnlyOwnUserIdentity, + ) -> ClearResult { + let master_differs = self + .master_public_key() + .await + .map(|master| &master != public_identity.master_key()) + .unwrap_or(false); + + let user_signing_differs = self + .user_signing_public_key() + .await + .map(|subkey| &subkey != public_identity.user_signing_key()) + .unwrap_or(false); + + let self_signing_differs = self + .self_signing_public_key() + .await + .map(|subkey| &subkey != public_identity.self_signing_key()) + .unwrap_or(false); + + ClearResult { + master_cleared: master_differs, + user_signing_cleared: user_signing_differs, + self_signing_cleared: self_signing_differs, + } + } + /// Get the names of the secrets we are missing. pub(crate) async fn get_missing_secrets(&self) -> Vec { let mut missing = Vec::new(); @@ -282,6 +365,7 @@ impl PrivateCrossSigningIdentity { .ok_or(SignatureError::MissingSigningKey)? .public_key .clone(); + let self_signing = self .self_signing_key .lock() @@ -290,6 +374,7 @@ impl PrivateCrossSigningIdentity { .ok_or(SignatureError::MissingSigningKey)? .public_key .clone(); + let user_signing = self .user_signing_key .lock() @@ -298,6 +383,7 @@ impl PrivateCrossSigningIdentity { .ok_or(SignatureError::MissingSigningKey)? .public_key .clone(); + let identity = ReadOnlyOwnUserIdentity::new(master, self_signing, user_signing)?; identity.mark_as_verified(); @@ -506,23 +592,18 @@ impl PrivateCrossSigningIdentity { ) -> Result { let signings: PickledSignings = serde_json::from_str(&pickle.pickle)?; - let master = if let Some(m) = signings.master_key { - Some(MasterSigning::from_pickle(m, pickle_key)?) - } else { - None - }; + let master = + signings.master_key.map(|m| MasterSigning::from_pickle(m, pickle_key)).transpose()?; - let self_signing = if let Some(s) = signings.self_signing_key { - Some(SelfSigning::from_pickle(s, pickle_key)?) - } else { - None - }; + let self_signing = signings + .self_signing_key + .map(|s| SelfSigning::from_pickle(s, pickle_key)) + .transpose()?; - let user_signing = if let Some(u) = signings.user_signing_key { - Some(UserSigning::from_pickle(u, pickle_key)?) - } else { - None - }; + let user_signing = signings + .user_signing_key + .map(|s| UserSigning::from_pickle(s, pickle_key)) + .transpose()?; Ok(Self { user_id: Arc::new(pickle.user_id), @@ -537,13 +618,13 @@ impl PrivateCrossSigningIdentity { /// identity. pub(crate) async fn as_upload_request(&self) -> UploadSigningKeysRequest { let master_key = - self.master_key.lock().await.as_ref().cloned().map(|k| k.public_key.into()); + self.master_key.lock().await.as_ref().map(|k| k.public_key.to_owned().into()); let user_signing_key = - self.user_signing_key.lock().await.as_ref().cloned().map(|k| k.public_key.into()); + self.user_signing_key.lock().await.as_ref().map(|k| k.public_key.to_owned().into()); let self_signing_key = - self.self_signing_key.lock().await.as_ref().cloned().map(|k| k.public_key.into()); + self.self_signing_key.lock().await.as_ref().map(|k| k.public_key.to_owned().into()); UploadSigningKeysRequest { master_key, self_signing_key, user_signing_key } } diff --git a/matrix_sdk_crypto/src/store/mod.rs b/matrix_sdk_crypto/src/store/mod.rs index aca23877..57d79c69 100644 --- a/matrix_sdk_crypto/src/store/mod.rs +++ b/matrix_sdk_crypto/src/store/mod.rs @@ -202,6 +202,10 @@ impl Store { self.identity.lock().await.reset().await; } + pub fn private_identity(&self) -> Arc> { + self.identity.clone() + } + pub async fn save_sessions(&self, sessions: &[Session]) -> Result<()> { let changes = Changes { sessions: sessions.to_vec(), ..Default::default() };