From 680f77beb9b712e71dcba543bfab0fb5a306423f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Thu, 12 Aug 2021 14:13:22 +0200 Subject: [PATCH] crypto: Add the master key to the SAS MAC if we trust it --- matrix_sdk_crypto/src/identities/user.rs | 7 +++++ matrix_sdk_crypto/src/verification/machine.rs | 10 +++++++ .../src/verification/requests.rs | 19 ++++++++++-- .../src/verification/sas/helpers.rs | 17 ++++++++++- .../src/verification/sas/inner_sas.rs | 15 ++++++++-- matrix_sdk_crypto/src/verification/sas/mod.rs | 11 ++++++- .../src/verification/sas/sas_state.rs | 29 ++++++++++++++----- 7 files changed, 95 insertions(+), 13 deletions(-) diff --git a/matrix_sdk_crypto/src/identities/user.rs b/matrix_sdk_crypto/src/identities/user.rs index 839deb25..11c21105 100644 --- a/matrix_sdk_crypto/src/identities/user.rs +++ b/matrix_sdk_crypto/src/identities/user.rs @@ -666,6 +666,13 @@ impl ReadOnlyUserIdentities { } } + pub(crate) fn into_own(self) -> Option { + match self { + ReadOnlyUserIdentities::Own(i) => Some(i), + _ => None, + } + } + /// Destructure the enum into an `UserIdentity` if it's of the correct /// type. pub fn other(&self) -> Option<&ReadOnlyUserIdentity> { diff --git a/matrix_sdk_crypto/src/verification/machine.rs b/matrix_sdk_crypto/src/verification/machine.rs index 075bcd29..d3cb603b 100644 --- a/matrix_sdk_crypto/src/verification/machine.rs +++ b/matrix_sdk_crypto/src/verification/machine.rs @@ -128,12 +128,15 @@ impl VerificationMachine { device: ReadOnlyDevice, ) -> Result<(Sas, OutgoingVerificationRequest), CryptoStoreError> { let identity = self.store.get_user_identity(device.user_id()).await?; + let own_identity = + self.store.get_user_identity(self.own_user_id()).await?.and_then(|i| i.into_own()); let private_identity = self.private_identity.lock().await.clone(); let (sas, content) = Sas::start( private_identity, device.clone(), self.store.clone(), + own_identity, identity, None, true, @@ -411,6 +414,11 @@ impl VerificationMachine { self.store.get_device(event.sender(), c.from_device()).await? { let private_identity = self.private_identity.lock().await.clone(); + let own_identity = self + .store + .get_user_identity(self.own_user_id()) + .await? + .and_then(|i| i.into_own()); let identity = self.store.get_user_identity(event.sender()).await?; match Sas::from_start_event( @@ -419,6 +427,7 @@ impl VerificationMachine { self.store.clone(), private_identity, device, + own_identity, identity, None, false, @@ -559,6 +568,7 @@ mod test { bob_store, None, None, + None, true, None, ); diff --git a/matrix_sdk_crypto/src/verification/requests.rs b/matrix_sdk_crypto/src/verification/requests.rs index 6767fcf3..63a3a7e7 100644 --- a/matrix_sdk_crypto/src/verification/requests.rs +++ b/matrix_sdk_crypto/src/verification/requests.rs @@ -46,8 +46,8 @@ use super::{ }; use crate::{ olm::{PrivateCrossSigningIdentity, ReadOnlyAccount}, - CryptoStoreError, OutgoingVerificationRequest, ReadOnlyDevice, ReadOnlyUserIdentities, - RoomMessageRequest, Sas, ToDeviceRequest, + CryptoStoreError, OutgoingVerificationRequest, ReadOnlyDevice, ReadOnlyOwnUserIdentity, + ReadOnlyUserIdentities, RoomMessageRequest, Sas, ToDeviceRequest, }; const SUPPORTED_METHODS: &[VerificationMethod] = &[ @@ -920,6 +920,7 @@ impl RequestState { &self, content: &StartContent<'a>, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, we_started: bool, request_handle: RequestHandle, @@ -930,6 +931,7 @@ impl RequestState { self.store.clone(), self.private_cross_signing_identity.clone(), other_device, + own_identity, other_identity, Some(request_handle), we_started, @@ -1100,12 +1102,18 @@ impl RequestState { }; let identity = self.store.get_user_identity(sender).await?; + let own_identity = self + .store + .get_user_identity(self.store.account.user_id()) + .await? + .and_then(|i| i.into_own()); match content.method() { StartMethod::SasV1(_) => { match self.to_started_sas( content, device.clone(), + own_identity, identity, we_started, request_handle, @@ -1169,6 +1177,11 @@ impl RequestState { // TODO signal why starting the sas flow doesn't work? let other_identity = store.get_user_identity(&self.other_user_id).await?; + let own_identity = self + .store + .get_user_identity(self.store.account.user_id()) + .await? + .and_then(|i| i.into_own()); let device = if let Some(device) = self.store.get_device(&self.other_user_id, &self.state.other_device_id).await? @@ -1190,6 +1203,7 @@ impl RequestState { private_identity, device, store, + own_identity, other_identity, Some(t.to_owned()), we_started, @@ -1204,6 +1218,7 @@ impl RequestState { private_identity, device, store, + own_identity, other_identity, we_started, request_handle, diff --git a/matrix_sdk_crypto/src/verification/sas/helpers.rs b/matrix_sdk_crypto/src/verification/sas/helpers.rs index f39869b0..c752d888 100644 --- a/matrix_sdk_crypto/src/verification/sas/helpers.rs +++ b/matrix_sdk_crypto/src/verification/sas/helpers.rs @@ -34,12 +34,13 @@ use crate::{ identities::{ReadOnlyDevice, ReadOnlyUserIdentities}, utilities::encode, verification::event_enums::{MacContent, StartContent}, - ReadOnlyAccount, + ReadOnlyAccount, ReadOnlyOwnUserIdentity, }; #[derive(Clone, Debug)] pub struct SasIds { pub account: ReadOnlyAccount, + pub own_identity: Option, pub other_device: ReadOnlyDevice, pub other_identity: Option, } @@ -304,6 +305,20 @@ pub fn get_mac_content(sas: &OlmSas, ids: &SasIds, flow_id: &FlowId) -> Outgoing sas.calculate_mac(key, &format!("{}{}", info, key_id)).expect("Can't calculate SAS MAC"), ); + if let Some(own_identity) = &ids.own_identity { + if own_identity.is_verified() { + if let Some(key) = own_identity.master_key().get_first_key() { + let key_id = format!("{}:{}", DeviceKeyAlgorithm::Ed25519, &key); + + let calculated_mac = sas + .calculate_mac(key, &format!("{}{}", info, &key_id)) + .expect("Can't calculate SAS Master key MAC"); + + mac.insert(key_id, calculated_mac); + } + } + } + // TODO Add the cross signing master key here if we trust/have it. let mut keys = mac.keys().cloned().collect::>(); diff --git a/matrix_sdk_crypto/src/verification/sas/inner_sas.rs b/matrix_sdk_crypto/src/verification/sas/inner_sas.rs index 53677e33..9679b824 100644 --- a/matrix_sdk_crypto/src/verification/sas/inner_sas.rs +++ b/matrix_sdk_crypto/src/verification/sas/inner_sas.rs @@ -34,7 +34,7 @@ use crate::{ event_enums::{AnyVerificationContent, OutgoingContent, OwnedAcceptContent, StartContent}, Cancelled, Done, }, - ReadOnlyAccount, + ReadOnlyAccount, ReadOnlyOwnUserIdentity, }; #[derive(Clone, Debug)] @@ -55,10 +55,17 @@ impl InnerSas { pub fn start( account: ReadOnlyAccount, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, transaction_id: Option, ) -> (InnerSas, OutgoingContent) { - let sas = SasState::::new(account, other_device, other_identity, transaction_id); + let sas = SasState::::new( + account, + other_device, + own_identity, + other_identity, + transaction_id, + ); let content = sas.as_content(); (InnerSas::Created(sas), content.into()) } @@ -131,6 +138,7 @@ impl InnerSas { room_id: RoomId, account: ReadOnlyAccount, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, ) -> (InnerSas, OutgoingContent) { let sas = SasState::::new_in_room( @@ -138,6 +146,7 @@ impl InnerSas { event_id, account, other_device, + own_identity, other_identity, ); let content = sas.as_content(); @@ -149,12 +158,14 @@ impl InnerSas { other_device: ReadOnlyDevice, flow_id: FlowId, content: &StartContent, + own_identity: Option, other_identity: Option, started_from_request: bool, ) -> Result { match SasState::::from_start_event( account, other_device, + own_identity, other_identity, flow_id, content, diff --git a/matrix_sdk_crypto/src/verification/sas/mod.rs b/matrix_sdk_crypto/src/verification/sas/mod.rs index 231d7e6a..051858d7 100644 --- a/matrix_sdk_crypto/src/verification/sas/mod.rs +++ b/matrix_sdk_crypto/src/verification/sas/mod.rs @@ -42,7 +42,7 @@ use crate::{ olm::PrivateCrossSigningIdentity, requests::{OutgoingVerificationRequest, RoomMessageRequest}, store::CryptoStoreError, - ReadOnlyAccount, ToDeviceRequest, + ReadOnlyAccount, ReadOnlyOwnUserIdentity, ToDeviceRequest, }; /// Short authentication string object. @@ -183,10 +183,12 @@ impl Sas { /// /// Returns the new `Sas` object and a `StartEventContent` that needs to be /// sent out through the server to the other device. + #[allow(clippy::too_many_arguments)] pub(crate) fn start( private_identity: PrivateCrossSigningIdentity, other_device: ReadOnlyDevice, store: VerificationStore, + own_identity: Option, other_identity: Option, transaction_id: Option, we_started: bool, @@ -195,6 +197,7 @@ impl Sas { let (inner, content) = InnerSas::start( store.account.clone(), other_device.clone(), + own_identity, other_identity.clone(), transaction_id, ); @@ -230,6 +233,7 @@ impl Sas { private_identity: PrivateCrossSigningIdentity, other_device: ReadOnlyDevice, store: VerificationStore, + own_identity: Option, other_identity: Option, we_started: bool, request_handle: RequestHandle, @@ -239,6 +243,7 @@ impl Sas { room_id, store.account.clone(), other_device.clone(), + own_identity, other_identity.clone(), ); @@ -273,6 +278,7 @@ impl Sas { store: VerificationStore, private_identity: PrivateCrossSigningIdentity, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, request_handle: Option, we_started: bool, @@ -282,6 +288,7 @@ impl Sas { other_device.clone(), flow_id, content, + own_identity, other_identity.clone(), request_handle.is_some(), )?; @@ -601,6 +608,7 @@ mod test { alice_store, None, None, + None, true, None, ); @@ -616,6 +624,7 @@ mod test { alice_device, None, None, + None, false, ) .unwrap(); diff --git a/matrix_sdk_crypto/src/verification/sas/sas_state.rs b/matrix_sdk_crypto/src/verification/sas/sas_state.rs index 7aa9bc2f..78bfd541 100644 --- a/matrix_sdk_crypto/src/verification/sas/sas_state.rs +++ b/matrix_sdk_crypto/src/verification/sas/sas_state.rs @@ -60,7 +60,7 @@ use crate::{ }, Cancelled, Done, FlowId, }, - ReadOnlyAccount, + ReadOnlyAccount, ReadOnlyOwnUserIdentity, }; const KEY_AGREEMENT_PROTOCOLS: &[KeyAgreementProtocol] = @@ -362,13 +362,21 @@ impl SasState { pub fn new( account: ReadOnlyAccount, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, transaction_id: Option, ) -> SasState { let started_from_request = transaction_id.is_some(); let flow_id = FlowId::ToDevice(transaction_id.unwrap_or_else(|| Uuid::new_v4().to_string())); - Self::new_helper(flow_id, account, other_device, other_identity, started_from_request) + Self::new_helper( + flow_id, + account, + other_device, + own_identity, + other_identity, + started_from_request, + ) } /// Create a new SAS in-room verification flow. @@ -388,22 +396,24 @@ impl SasState { event_id: EventId, account: ReadOnlyAccount, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, ) -> SasState { let flow_id = FlowId::InRoom(room_id, event_id); - Self::new_helper(flow_id, account, other_device, other_identity, false) + Self::new_helper(flow_id, account, other_device, own_identity, other_identity, false) } fn new_helper( flow_id: FlowId, account: ReadOnlyAccount, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, started_from_request: bool, ) -> SasState { SasState { inner: Arc::new(Mutex::new(OlmSas::new())), - ids: SasIds { account, other_device, other_identity }, + ids: SasIds { account, other_device, other_identity, own_identity }, verification_flow_id: flow_id.into(), creation_time: Arc::new(Instant::now()), @@ -497,6 +507,7 @@ impl SasState { pub fn from_start_event( account: ReadOnlyAccount, other_device: ReadOnlyDevice, + own_identity: Option, other_identity: Option, flow_id: FlowId, content: &StartContent, @@ -514,6 +525,7 @@ impl SasState { ids: SasIds { account: account.clone(), other_device: other_device.clone(), + own_identity: own_identity.clone(), other_identity: other_identity.clone(), }, @@ -536,7 +548,7 @@ impl SasState { Ok(SasState { inner: Arc::new(Mutex::new(sas)), - ids: SasIds { account, other_device, other_identity }, + ids: SasIds { account, other_device, other_identity, own_identity }, creation_time: Arc::new(Instant::now()), last_event_time: Arc::new(Instant::now()), @@ -1158,7 +1170,7 @@ mod test { let bob = ReadOnlyAccount::new(&bob_id(), &bob_device_id()); let bob_device = ReadOnlyDevice::from_account(&bob).await; - let alice_sas = SasState::::new(alice.clone(), bob_device, None, None); + let alice_sas = SasState::::new(alice.clone(), bob_device, None, None, None); let start_content = alice_sas.as_content(); let flow_id = start_content.flow_id(); @@ -1167,6 +1179,7 @@ mod test { bob.clone(), alice_device, None, + None, flow_id, &start_content.as_start_content(), false, @@ -1339,7 +1352,7 @@ mod test { let bob = ReadOnlyAccount::new(&bob_id(), &bob_device_id()); let bob_device = ReadOnlyDevice::from_account(&bob).await; - let alice_sas = SasState::::new(alice.clone(), bob_device, None, None); + let alice_sas = SasState::::new(alice.clone(), bob_device, None, None, None); let mut start_content = alice_sas.as_content(); let method = start_content.method_mut(); @@ -1358,6 +1371,7 @@ mod test { bob.clone(), alice_device.clone(), None, + None, flow_id, &content, false, @@ -1379,6 +1393,7 @@ mod test { bob.clone(), alice_device, None, + None, flow_id.into(), &content, false,