From 408fe5da4b81db5cd0e79daa01357a6002d54ef2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Tue, 4 Aug 2020 12:14:19 +0200 Subject: [PATCH] crypto: Check that the other device had a valid MAC. --- .../src/verification/sas/helpers.rs | 6 +-- matrix_sdk_crypto/src/verification/sas/mod.rs | 42 ++++++++++++------- .../src/verification/sas/sas_state.rs | 14 ++++--- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/matrix_sdk_crypto/src/verification/sas/helpers.rs b/matrix_sdk_crypto/src/verification/sas/helpers.rs index bdd4e920..33e01a53 100644 --- a/matrix_sdk_crypto/src/verification/sas/helpers.rs +++ b/matrix_sdk_crypto/src/verification/sas/helpers.rs @@ -142,9 +142,9 @@ pub fn receive_mac_event( ids: &SasIds, flow_id: &str, event: &ToDeviceEvent, -) -> (Vec>, Vec) { +) -> (Vec, Vec) { // TODO check the event and cancel if it isn't ok (sender, transaction id) - let mut verified_devices: Vec> = Vec::new(); + let mut verified_devices = Vec::new(); let info = extra_mac_info_receive(&ids, flow_id); @@ -180,7 +180,7 @@ pub fn receive_mac_event( .calculate_mac(key, &format!("{}{}", info, key_id)) .expect("Can't calculate SAS MAC") { - verified_devices.push(ids.other_device.device_id().into()); + verified_devices.push(ids.other_device.clone()); } else { // TODO cancel here } diff --git a/matrix_sdk_crypto/src/verification/sas/mod.rs b/matrix_sdk_crypto/src/verification/sas/mod.rs index f2717cae..c6e1b526 100644 --- a/matrix_sdk_crypto/src/verification/sas/mod.rs +++ b/matrix_sdk_crypto/src/verification/sas/mod.rs @@ -182,16 +182,30 @@ impl Sas { if let Some(device) = device { if device.keys() == self.other_device.keys() { - trace!( - "Marking device {} {} as verified.", - device.user_id(), - device.device_id() - ); + if self + .verified_devices() + .map_or(false, |v| v.contains(&device)) + { + trace!( + "Marking device {} {} as verified.", + device.user_id(), + device.device_id() + ); - device.set_trust_state(TrustState::Verified); - self.store.read().await.save_devices(&[device]).await?; + device.set_trust_state(TrustState::Verified); + self.store.read().await.save_devices(&[device]).await?; - Ok(true) + Ok(true) + } else { + info!( + "The interactive verification process didn't contain a \ + MAC for the device {} {}", + device.user_id(), + device.device_id() + ); + + Ok(false) + } } else { warn!( "The device keys of {} {} have changed while an interactive \ @@ -273,7 +287,7 @@ impl Sas { content } - pub(crate) fn verified_devices(&self) -> Option>>> { + pub(crate) fn verified_devices(&self) -> Option>> { self.inner.lock().unwrap().verified_devices() } @@ -472,7 +486,7 @@ impl InnerSas { } } - fn verified_devices(&self) -> Option>>> { + fn verified_devices(&self) -> Option>> { if let InnerSas::Done(s) = self { Some(s.verified_devices()) } else { @@ -609,8 +623,8 @@ mod test { let event = wrap_to_device_event(alice.user_id(), alice.as_content()); let bob = bob.into_done(&event).unwrap(); - assert!(bob.verified_devices().contains(&alice.device_id().into())); - assert!(alice.verified_devices().contains(&bob.device_id().into())); + assert!(bob.verified_devices().contains(&bob.other_device())); + assert!(alice.verified_devices().contains(&alice.other_device())); } #[tokio::test] @@ -674,10 +688,10 @@ mod test { assert!(alice .verified_devices() .unwrap() - .contains(&bob.device_id().into())); + .contains(&alice.other_device())); assert!(bob .verified_devices() .unwrap() - .contains(&alice.device_id().into())); + .contains(&bob.other_device())); } } diff --git a/matrix_sdk_crypto/src/verification/sas/sas_state.rs b/matrix_sdk_crypto/src/verification/sas/sas_state.rs index 868ae0a3..ec09f8b7 100644 --- a/matrix_sdk_crypto/src/verification/sas/sas_state.rs +++ b/matrix_sdk_crypto/src/verification/sas/sas_state.rs @@ -165,7 +165,7 @@ pub struct Confirmed { #[derive(Clone, Debug)] pub struct MacReceived { we_started: bool, - verified_devices: Arc>>, + verified_devices: Arc>, verified_master_keys: Arc>, } @@ -175,7 +175,7 @@ pub struct MacReceived { /// the master keys in the verified devices list. #[derive(Clone, Debug)] pub struct Done { - verified_devices: Arc>>, + verified_devices: Arc>, verified_master_keys: Arc>, } @@ -196,6 +196,10 @@ impl SasState { &self.ids.account.device_id() } + pub fn other_device(&self) -> Device { + self.ids.other_device.clone() + } + pub fn cancel(self, cancel_code: CancelCode) -> SasState { SasState { inner: self.inner, @@ -691,7 +695,7 @@ impl SasState { } /// Get the list of verified devices. - pub fn verified_devices(&self) -> Arc>> { + pub fn verified_devices(&self) -> Arc> { self.state.verified_devices.clone() } @@ -853,7 +857,7 @@ mod test { let event = wrap_to_device_event(alice.user_id(), alice.as_content()); let bob = bob.into_done(&event).unwrap(); - assert!(bob.verified_devices().contains(&alice.device_id().into())); - assert!(alice.verified_devices().contains(&bob.device_id().into())); + assert!(bob.verified_devices().contains(&bob.other_device())); + assert!(alice.verified_devices().contains(&alice.other_device())); } }