From 0e4057f3a3fdd75d594c7f5b68355014540d23a8 Mon Sep 17 00:00:00 2001 From: ftilde Date: Sun, 12 Dec 2021 00:28:11 +0100 Subject: [PATCH 1/4] Do not drop requests after receiving mac if sas is not complete When during verification we are the first party to complete the verification workflow and then receive a (correct) MAC from the other party, we need to send out the Done message even if we have not received the Done message from the other party ourselves and are thus still "WaitingForDone". --- crates/matrix-sdk-crypto/src/verification/machine.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/matrix-sdk-crypto/src/verification/machine.rs b/crates/matrix-sdk-crypto/src/verification/machine.rs index 6aa91f100..6a29d9f90 100644 --- a/crates/matrix-sdk-crypto/src/verification/machine.rs +++ b/crates/matrix-sdk-crypto/src/verification/machine.rs @@ -471,6 +471,18 @@ impl VerificationMachine { if s.is_done() { self.mark_sas_as_done(s, content).await?; + } else { + // Even if we are not done (yet), there might be content to send + // out, e.g. in the case where we are done with our side of the + // verification process, but the other side has not yet sent their + // "done". + if let Some(content) = content { + self.queue_up_content( + s.other_user_id(), + s.other_device_id(), + content, + ); + } } } else { flow_id_mismatch(); From 1c72472706ebcbc8da63fe73076b909a5dbf8ea0 Mon Sep 17 00:00:00 2001 From: ftilde Date: Sat, 18 Dec 2021 22:41:20 +0100 Subject: [PATCH 2/4] Send Done message when confirming in MacReceived state Being in MacReceived state means that the other party has confirmed that emojis/numbers match and the sent mac messages has been successfully checked. When now confirming ourselves this means that the verification was successful and thus the Done message can be sent to the other party. --- crates/matrix-sdk-crypto/src/machine.rs | 20 +++----- .../src/verification/machine.rs | 8 +++- .../src/verification/sas/inner_sas.rs | 12 ++--- .../src/verification/sas/mod.rs | 47 +++++++++++-------- .../src/encryption/verification/sas.rs | 4 +- 5 files changed, 47 insertions(+), 44 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index d25015877..60659b333 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -2147,25 +2147,17 @@ pub(crate) mod test { assert_eq!(alice_sas.emoji(), bob_sas.emoji()); assert_eq!(alice_sas.decimals(), bob_sas.decimals()); - let event = bob_sas - .confirm() - .await - .unwrap() - .0 - .map(|r| request_to_event(bob.user_id(), &r)) - .unwrap(); + let contents = bob_sas.confirm().await.unwrap().0; + assert!(contents.len() == 1); + let event = request_to_event(bob.user_id(), &contents[0]); alice.handle_verification_event(&event).await; assert!(!alice_sas.is_done()); assert!(!bob_sas.is_done()); - let event = alice_sas - .confirm() - .await - .unwrap() - .0 - .map(|r| request_to_event(alice.user_id(), &r)) - .unwrap(); + let contents = alice_sas.confirm().await.unwrap().0; + assert!(contents.len() == 1); + let event = request_to_event(alice.user_id(), &contents[0]); assert!(alice_sas.is_done()); assert!(bob_device.verified()); diff --git a/crates/matrix-sdk-crypto/src/verification/machine.rs b/crates/matrix-sdk-crypto/src/verification/machine.rs index 6a29d9f90..4ec6d6269 100644 --- a/crates/matrix-sdk-crypto/src/verification/machine.rs +++ b/crates/matrix-sdk-crypto/src/verification/machine.rs @@ -642,12 +642,16 @@ mod test { assert!(bob.emoji().is_some()); assert_eq!(alice.emoji(), bob.emoji()); - let request = alice.confirm().await.unwrap().0.unwrap(); + let mut requests = alice.confirm().await.unwrap().0; + assert!(requests.len() == 1); + let request = requests.pop().unwrap(); let content = OutgoingContent::try_from(request).unwrap(); let content = MacContent::try_from(&content).unwrap().into(); bob.receive_any_event(alice.user_id(), &content); - let request = bob.confirm().await.unwrap().0.unwrap(); + let mut requests = bob.confirm().await.unwrap().0; + assert!(requests.len() == 1); + let request = requests.pop().unwrap(); let content = OutgoingContent::try_from(request).unwrap(); let content = MacContent::try_from(&content).unwrap().into(); alice.receive_any_event(bob.user_id(), &content); diff --git a/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs b/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs index 27cbbd9e6..b7e9258c3 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs @@ -228,27 +228,27 @@ impl InnerSas { (InnerSas::Cancelled(sas), Some(content)) } - pub fn confirm(self) -> (InnerSas, Option) { + pub fn confirm(self) -> (InnerSas, Vec) { match self { InnerSas::KeyReceived(s) => { let sas = s.confirm(); let content = sas.as_content(); - (InnerSas::Confirmed(sas), Some(content)) + (InnerSas::Confirmed(sas), vec![content]) } InnerSas::MacReceived(s) => { if s.started_from_request { let sas = s.confirm_and_wait_for_done(); - let content = sas.as_content(); + let contents = vec![sas.as_content(), sas.done_content()]; - (InnerSas::WaitingForDone(sas), Some(content)) + (InnerSas::WaitingForDone(sas), contents) } else { let sas = s.confirm(); let content = sas.as_content(); - (InnerSas::Done(sas), Some(content)) + (InnerSas::Done(sas), vec![content]) } } - _ => (self, None), + _ => (self, Vec::new()), } } diff --git a/crates/matrix-sdk-crypto/src/verification/sas/mod.rs b/crates/matrix-sdk-crypto/src/verification/sas/mod.rs index f8391f80d..15cdb24f5 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/mod.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/mod.rs @@ -355,27 +355,28 @@ impl Sas { /// the server. pub async fn confirm( &self, - ) -> Result< - (Option, Option), - CryptoStoreError, - > { - let (content, done) = { + ) -> Result<(Vec, Option), CryptoStoreError> + { + let (contents, done) = { let mut guard = self.inner.lock().unwrap(); let sas: InnerSas = (*guard).clone(); - let (sas, content) = sas.confirm(); + let (sas, contents) = sas.confirm(); *guard = sas; - (content, guard.is_done()) + (contents, guard.is_done()) }; - let mac_request = content.map(|c| match c { - OutgoingContent::ToDevice(c) => self.content_to_request(c).into(), - OutgoingContent::Room(r, c) => { - RoomMessageRequest { room_id: r, txn_id: Uuid::new_v4(), content: c }.into() - } - }); + let mac_requests = contents + .into_iter() + .map(|c| match c { + OutgoingContent::ToDevice(c) => self.content_to_request(c).into(), + OutgoingContent::Room(r, c) => { + RoomMessageRequest { room_id: r, txn_id: Uuid::new_v4(), content: c }.into() + } + }) + .collect::>(); - if mac_request.is_some() { + if !mac_requests.is_empty() { trace!( user_id = self.other_user_id().as_str(), device_id = self.other_device_id().as_str(), @@ -385,12 +386,14 @@ impl Sas { if done { match self.mark_as_done().await? { - VerificationResult::Cancel(c) => Ok((self.cancel_with_code(c), None)), - VerificationResult::Ok => Ok((mac_request, None)), - VerificationResult::SignatureUpload(r) => Ok((mac_request, Some(r))), + VerificationResult::Cancel(c) => { + Ok((self.cancel_with_code(c).into_iter().collect(), None)) + } + VerificationResult::Ok => Ok((mac_requests, None)), + VerificationResult::SignatureUpload(r) => Ok((mac_requests, Some(r))), } } else { - Ok((mac_request, None)) + Ok((mac_requests, None)) } } @@ -650,12 +653,16 @@ mod test { assert_eq!(alice.emoji().unwrap(), bob.emoji().unwrap()); assert_eq!(alice.decimals().unwrap(), bob.decimals().unwrap()); - let request = alice.confirm().await.unwrap().0.unwrap(); + let mut requests = alice.confirm().await.unwrap().0; + assert!(requests.len() == 1); + let request = requests.pop().unwrap(); let content = OutgoingContent::try_from(request).unwrap(); let content = MacContent::try_from(&content).unwrap(); bob.receive_any_event(alice.user_id(), &content.into()); - let request = bob.confirm().await.unwrap().0.unwrap(); + let mut requests = bob.confirm().await.unwrap().0; + assert!(requests.len() == 1); + let request = requests.pop().unwrap(); let content = OutgoingContent::try_from(request).unwrap(); let content = MacContent::try_from(&content).unwrap(); alice.receive_any_event(bob.user_id(), &content.into()); diff --git a/crates/matrix-sdk/src/encryption/verification/sas.rs b/crates/matrix-sdk/src/encryption/verification/sas.rs index ef4791f1e..b87db5a93 100644 --- a/crates/matrix-sdk/src/encryption/verification/sas.rs +++ b/crates/matrix-sdk/src/encryption/verification/sas.rs @@ -76,9 +76,9 @@ impl SasVerification { /// Confirm that the short auth strings match on both sides. pub async fn confirm(&self) -> Result<()> { - let (request, signature) = self.inner.confirm().await?; + let (requests, signature) = self.inner.confirm().await?; - if let Some(request) = request { + for request in requests { self.client.send_verification_request(request).await?; } From b877d15616ed55eab9deeaa83e1519d6aaa32918 Mon Sep 17 00:00:00 2001 From: ftilde Date: Sat, 18 Dec 2021 22:49:12 +0100 Subject: [PATCH 3/4] Do not send Done message after receiving a Done Done messages are supposed to indicate that the verification was successful and _can be_ finalized. The correct time to send Done events is after the user has confirmed the verification (after checking the emoji or similar) and after receiving and checking a mac from the other party (whatever happens later). Waiting for the other side to send Done before sending it ourselves does not make sense. --- crates/matrix-sdk-crypto/src/verification/mod.rs | 6 +++++- crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs | 5 +---- crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs | 4 ---- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/verification/mod.rs b/crates/matrix-sdk-crypto/src/verification/mod.rs index 0d93347ed..d5b069a9b 100644 --- a/crates/matrix-sdk-crypto/src/verification/mod.rs +++ b/crates/matrix-sdk-crypto/src/verification/mod.rs @@ -31,6 +31,10 @@ use matrix_sdk_common::locks::Mutex; #[cfg(feature = "qrcode")] pub use qrcode::{QrVerification, ScanError}; pub use requests::VerificationRequest; +#[cfg(feature = "qrcode")] +use ruma::events::key::verification::done::{ + KeyVerificationDoneEventContent, ToDeviceKeyVerificationDoneEventContent, +}; use ruma::{ api::client::r0::keys::upload_signatures::Request as SignatureUploadRequest, events::{ @@ -39,7 +43,6 @@ use ruma::{ CancelCode, KeyVerificationCancelEventContent, ToDeviceKeyVerificationCancelEventContent, }, - done::{KeyVerificationDoneEventContent, ToDeviceKeyVerificationDoneEventContent}, Relation, }, AnyMessageEventContent, AnyToDeviceEventContent, @@ -243,6 +246,7 @@ pub struct Done { } impl Done { + #[cfg(feature = "qrcode")] pub fn as_content(&self, flow_id: &FlowId) -> OutgoingContent { match flow_id { FlowId::ToDevice(t) => AnyToDeviceEventContent::KeyVerificationDone( diff --git a/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs b/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs index b7e9258c3..be68e930e 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs @@ -335,10 +335,7 @@ impl InnerSas { }, AnyVerificationContent::Done(c) => match self { InnerSas::WaitingForDone(s) => match s.into_done(sender, c) { - Ok(s) => { - let content = s.done_content(); - (InnerSas::Done(s), Some(content)) - } + Ok(s) => (InnerSas::Done(s), None), Err(s) => { let content = s.as_content(); (InnerSas::Cancelled(s), Some(content)) diff --git a/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs b/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs index b4a64c300..499304044 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs @@ -1177,10 +1177,6 @@ impl SasState { get_mac_content(&self.inner.lock().unwrap(), &self.ids, &self.verification_flow_id) } - pub fn done_content(&self) -> OutgoingContent { - self.state.as_content(self.verification_flow_id.as_ref()) - } - /// Get the list of verified devices. pub fn verified_devices(&self) -> Arc<[ReadOnlyDevice]> { self.state.verified_devices.clone() From b1501fcb1d13a98b1b9ba69e5d33b140cf3cb6d9 Mon Sep 17 00:00:00 2001 From: ftilde Date: Thu, 23 Dec 2021 15:42:58 +0100 Subject: [PATCH 4/4] Add test for verification started from a request --- crates/matrix-sdk-crypto/src/machine.rs | 169 ++++++++++++++++++ .../matrix-sdk-crypto/src/verification/mod.rs | 9 + 2 files changed, 178 insertions(+) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 60659b333..5ad96a789 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1572,6 +1572,7 @@ pub(crate) mod test { event_id, events::{ dummy::ToDeviceDummyEventContent, + key::verification::VerificationMethod, room::{ encrypted::ToDeviceRoomEncryptedEventContent, message::{MessageType, RoomMessageEventContent}, @@ -2170,4 +2171,172 @@ pub(crate) mod test { assert!(bob_sas.is_done()); assert!(alice_device.verified()); } + + #[tokio::test] + async fn interactive_verification_started_from_request() { + let (alice, bob) = get_machine_pair_with_setup_sessions().await; + + // ---------------------------------------------------------------------------- + // On Alice's device: + let bob_device = alice.get_device(bob.user_id(), bob.device_id()).await.unwrap().unwrap(); + + assert!(!bob_device.verified()); + + // Alice sends a verification request with her desired methods to Bob + let (alice_ver_req, request) = + bob_device.request_verification_with_methods(vec![VerificationMethod::SasV1]).await; + + // ---------------------------------------------------------------------------- + // On Bobs's device: + let event = request_to_event(alice.user_id(), &request); + bob.handle_verification_event(&event).await; + let flow_id = alice_ver_req.flow_id().as_str(); + + let verification_request = bob.get_verification_request(alice.user_id(), flow_id).unwrap(); + + // Bob accepts the request, sending a Ready request + let accept_request = + verification_request.accept_with_methods(vec![VerificationMethod::SasV1]).unwrap(); + // And also immediately sends a start request + let (_, start_request_from_bob) = verification_request.start_sas().await.unwrap().unwrap(); + + // ---------------------------------------------------------------------------- + // On Alice's device: + + // Alice receives the Ready + let event = request_to_event(bob.user_id(), &accept_request); + alice.handle_verification_event(&event).await; + + let verification_request = alice.get_verification_request(bob.user_id(), flow_id).unwrap(); + + // And also immediately sends a start request + let (alice_sas, start_request_from_alice) = + verification_request.start_sas().await.unwrap().unwrap(); + + // Now alice receives Bob's start: + let event = request_to_event(bob.user_id(), &start_request_from_bob); + alice.handle_verification_event(&event).await; + + // Since Alice's user id is lexicographically smaller than Bob's, Alice does not + // do anything with the request, however. + assert!(alice.user_id() < bob.user_id()); + + // ---------------------------------------------------------------------------- + // On Bob's device: + + // Bob receives Alice's start: + let event = request_to_event(alice.user_id(), &start_request_from_alice); + bob.handle_verification_event(&event).await; + + let bob_sas = bob + .get_verification(alice.user_id(), alice_sas.flow_id().as_str()) + .unwrap() + .sas_v1() + .unwrap(); + + assert!(alice_sas.emoji().is_none()); + assert!(bob_sas.emoji().is_none()); + + // ... and accepts it + let event = bob_sas.accept().map(|r| request_to_event(bob.user_id(), &r)).unwrap(); + + // ---------------------------------------------------------------------------- + // On Alice's device: + + // Alice receives the Accept request: + alice.handle_verification_event(&event).await; + + // Alice sends a key + let msgs = alice.verification_machine.outgoing_messages(); + assert!(msgs.len() == 1); + let msg = msgs.first().unwrap(); + let event = outgoing_request_to_event(alice.user_id(), msg); + alice.verification_machine.mark_request_as_sent(&msg.request_id); + + // ---------------------------------------------------------------------------- + // On Bob's device: + + // And bob receive's it: + bob.handle_verification_event(&event).await; + + // Now bob sends a key + let msgs = bob.verification_machine.outgoing_messages(); + assert!(msgs.len() == 1); + let msg = msgs.first().unwrap(); + let event = outgoing_request_to_event(bob.user_id(), msg); + bob.verification_machine.mark_request_as_sent(&msg.request_id); + + // ---------------------------------------------------------------------------- + // On Alice's device: + + // And alice receives it + alice.handle_verification_event(&event).await; + + // As a result, both devices now can show emojis/decimals + assert!(alice_sas.emoji().is_some()); + assert!(bob_sas.emoji().is_some()); + + // ---------------------------------------------------------------------------- + // On Bob's device: + + assert_eq!(alice_sas.emoji(), bob_sas.emoji()); + assert_eq!(alice_sas.decimals(), bob_sas.decimals()); + + // Bob first confirms that the emojis match and sends the MAC... + let contents = bob_sas.confirm().await.unwrap().0; + assert!(contents.len() == 1); + let event = request_to_event(bob.user_id(), &contents[0]); + + // ---------------------------------------------------------------------------- + // On Alice's device: + + // ...which alice receives + alice.handle_verification_event(&event).await; + + assert!(!alice_sas.is_done()); + assert!(!bob_sas.is_done()); + + // Now alice confirms that the emojis match and sends... + let contents = alice_sas.confirm().await.unwrap().0; + assert!(contents.len() == 2); + // ... her own MAC... + let event_mac = request_to_event(alice.user_id(), &contents[0]); + // ... and a Done message + let event_done = request_to_event(alice.user_id(), &contents[1]); + + // ---------------------------------------------------------------------------- + // On Bob's device: + + // Bob receives the MAC message + bob.handle_verification_event(&event_mac).await; + + // Bob verifies that the MAC is valid and also sends a "done" message. + let msgs = bob.verification_machine.outgoing_messages(); + eprintln!("{:?}", msgs); + assert!(msgs.len() == 1); + let event = msgs.first().map(|r| outgoing_request_to_event(bob.user_id(), r)).unwrap(); + + let alice_device = + bob.get_device(alice.user_id(), alice.device_id()).await.unwrap().unwrap(); + + assert!(!bob_sas.is_done()); + assert!(!alice_device.verified()); + // And Bob receives the Done message of alice. + bob.handle_verification_event(&event_done).await; + + assert!(bob_sas.is_done()); + assert!(alice_device.verified()); + + // ---------------------------------------------------------------------------- + // On Alice's device: + + assert!(!alice_sas.is_done()); + assert!(!bob_device.verified()); + // Alices receives the done message + eprintln!("{:?}", event); + alice.handle_verification_event(&event).await; + + assert!(alice_sas.is_done()); + assert!(bob_device.verified()); + } } diff --git a/crates/matrix-sdk-crypto/src/verification/mod.rs b/crates/matrix-sdk-crypto/src/verification/mod.rs index d5b069a9b..6332143bf 100644 --- a/crates/matrix-sdk-crypto/src/verification/mod.rs +++ b/crates/matrix-sdk-crypto/src/verification/mod.rs @@ -719,6 +719,12 @@ pub(crate) mod test { let sender = sender.to_owned(); match content { + AnyToDeviceEventContent::KeyVerificationRequest(c) => { + AnyToDeviceEvent::KeyVerificationRequest(ToDeviceEvent { sender, content: c }) + } + AnyToDeviceEventContent::KeyVerificationReady(c) => { + AnyToDeviceEvent::KeyVerificationReady(ToDeviceEvent { sender, content: c }) + } AnyToDeviceEventContent::KeyVerificationKey(c) => { AnyToDeviceEvent::KeyVerificationKey(ToDeviceEvent { sender, content: c }) } @@ -731,6 +737,9 @@ pub(crate) mod test { AnyToDeviceEventContent::KeyVerificationMac(c) => { AnyToDeviceEvent::KeyVerificationMac(ToDeviceEvent { sender, content: c }) } + AnyToDeviceEventContent::KeyVerificationDone(c) => { + AnyToDeviceEvent::KeyVerificationDone(ToDeviceEvent { sender, content: c }) + } _ => unreachable!(), }