From 0bf99d5c731aca3707e4e7e98183f213a22dde13 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 7 Jul 2023 17:55:13 +0100 Subject: [PATCH] Fix handling of SAS start events once we have shown a QR code (#2242) Fixes #2237 Also a couple of other cleanups while I'm in the area. --- .../matrix-sdk-crypto-js/src/verification.rs | 2 + crates/matrix-sdk-crypto/CHANGELOG.md | 2 + .../src/verification/qrcode.rs | 39 +- .../src/verification/requests.rs | 387 ++++++++++-------- 4 files changed, 265 insertions(+), 165 deletions(-) diff --git a/bindings/matrix-sdk-crypto-js/src/verification.rs b/bindings/matrix-sdk-crypto-js/src/verification.rs index 5888d8f9a..654c35a76 100644 --- a/bindings/matrix-sdk-crypto-js/src/verification.rs +++ b/bindings/matrix-sdk-crypto-js/src/verification.rs @@ -1116,6 +1116,8 @@ impl VerificationRequest { /// Generate a QR code that can be used by another client to start /// a QR code based verification. + /// + /// Returns a `Qr`. #[cfg(feature = "qrcode")] #[wasm_bindgen(js_name = "generateQrCode")] pub fn generate_qr_code(&self) -> Promise { diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index 510afefb0..be9f8b2f2 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -36,3 +36,5 @@ - For verification-via-emojis, return the word "Aeroplane" rather than "Airplane", for consistency with the Matrix spec. + +- Fix handling of SAS verification start events once we have shown a QR code. diff --git a/crates/matrix-sdk-crypto/src/verification/qrcode.rs b/crates/matrix-sdk-crypto/src/verification/qrcode.rs index aacd0adda..74e8c383b 100644 --- a/crates/matrix-sdk-crypto/src/verification/qrcode.rs +++ b/crates/matrix-sdk-crypto/src/verification/qrcode.rs @@ -95,13 +95,26 @@ pub enum ScanError { #[derive(Debug, Clone)] pub enum QrVerificationState { /// The QR verification has been started. + /// + /// We have received the other device's details (from the + /// `m.key.verification.request` or `m.key.verification.ready`) and + /// established the shared secret, so can + /// display the QR code. + /// + /// Note that despite the name of this state, we have not yet sent or + /// received an `m.key.verification.start` message. Started, /// The QR verification has been scanned by the other side. Scanned, - /// The scanning of the QR code has been confirmed by us. + /// We have confirmed the other side's scan of the QR code. Confirmed, /// We have successfully scanned the QR code and are able to send a /// reciprocation event. + /// + /// Call `QrVerification::reciprocate` to build the reciprocation message. + /// + /// Note that, despite the name of this state, we have not necessarily + /// yet sent the `m.reciprocate.v1` message. Reciprocated, /// The verification process has been successfully concluded. Done { @@ -676,11 +689,35 @@ impl QrVerification { #[derive(Debug, Clone)] enum InnerState { + /// We have received the other device's details (from the + /// `m.key.verification.request` or `m.key.verification.ready`) and + /// established the shared secret, so can + /// display the QR code. Created(QrState), + + /// The other side has scanned our QR code and sent an + /// `m.key.verification.start` message with `method: m.reciprocate.v1` with + /// matching shared secret. Scanned(QrState), + + /// Our user has confirmed that the other device scanned successfully. We + /// have sent an `m.key.verification.done`. Confirmed(QrState), + + /// We have scanned the other side's QR code and are able to send a + /// `m.key.verification.start` message with `method: m.reciprocate.v1`. + /// + /// Call `QrVerification::reciprocate` to build the start message. + /// + /// Note that, despite the name of this state, we have not necessarily + /// yet sent the `m.reciprocate.v1` message. Reciprocated(QrState), + + /// Verification complete: we have received an `m.key.verification.done` + /// from the other side. Done(QrState), + + /// Verification cancelled or failed. Cancelled(QrState), } diff --git a/crates/matrix-sdk-crypto/src/verification/requests.rs b/crates/matrix-sdk-crypto/src/verification/requests.rs index e10baa162..ea3ce406d 100644 --- a/crates/matrix-sdk-crypto/src/verification/requests.rs +++ b/crates/matrix-sdk-crypto/src/verification/requests.rs @@ -42,7 +42,7 @@ use tracing::debug; use tracing::{info, trace, warn}; #[cfg(feature = "qrcode")] -use super::qrcode::{QrVerification, ScanError}; +use super::qrcode::{QrVerification, QrVerificationState, ScanError}; use super::{ cache::VerificationCache, event_enums::{ @@ -1379,30 +1379,51 @@ async fn receive_start( we_started, ) { Ok(new) => { - if let Some(Verification::SasV1(_old)) = - request_state.verification_cache.get(sender, request_state.flow_id.as_str()) - { - // If there is already a SAS verification, i.e. we already started one - // before the other side tried to do the same; ignore it if we did and - // we're the lexicographically smaller user ID (or device ID if equal). - use std::cmp::Ordering; - if !matches!( - (sender.cmp(own_user_id), device.device_id().cmp(own_device_id)), - (Ordering::Greater, _) | (Ordering::Equal, Ordering::Greater) - ) { - info!( - "Started a new SAS verification, replacing an already started one." - ); - request_state.verification_cache.replace_sas(new.to_owned()); - - Ok(Some(state.to_transitioned(request_state, new.into()))) - } else { - Ok(None) + let old_verification = request_state + .verification_cache + .get(sender, request_state.flow_id.as_str()); + match old_verification { + Some(Verification::SasV1(_old)) => { + // If there is already a SAS verification, i.e. we already started one + // before the other side tried to do the same; ignore it if we did and + // we're the lexicographically smaller user ID (or device ID if equal). + use std::cmp::Ordering; + if !matches!( + (sender.cmp(own_user_id), device.device_id().cmp(own_device_id)), + (Ordering::Greater, _) | (Ordering::Equal, Ordering::Greater) + ) { + info!("Started a new SAS verification, replacing an already started one."); + request_state.verification_cache.replace_sas(new.to_owned()); + Ok(Some(state.to_transitioned(request_state, new.into()))) + } else { + info!("Ignored incoming SAS verification from lexicographically larger user/device ID."); + Ok(None) + } + } + #[cfg(feature = "qrcode")] + Some(Verification::QrV1(old)) => { + // If there is already a QR verification, our ability to transition to + // SAS depends on how far we got through the QR flow. + if let QrVerificationState::Started = old.state() { + // it is legit to transition from QR display to SAS + info!("Transitioned from QR display to SAS"); + request_state.verification_cache.replace_sas(new.to_owned()); + Ok(Some(state.to_transitioned(request_state, new.into()))) + } else { + // otherwise, we've either scanned their QR code, or they have + // scanned ours -- i.e., an `m.key.verification.start` with method + // `m.reciprocate.v1` has already been sent/received and, per the + // spec, it is too late to switch to SAS. + warn!(qr_state = ?old.state(), "Invalid transition from QR to SAS"); + request_state.verification_cache.insert_sas(new.to_owned()); + Ok(Some(state.to_transitioned(request_state, new.into()))) + } + } + None => { + info!("Started a new SAS verification."); + request_state.verification_cache.insert_sas(new.to_owned()); + Ok(Some(state.to_transitioned(request_state, new.into()))) } - } else { - info!("Started a new SAS verification."); - request_state.verification_cache.insert_sas(new.to_owned()); - Ok(Some(state.to_transitioned(request_state, new.into()))) } } Err(c) => { @@ -1614,9 +1635,10 @@ mod tests { #[cfg(feature = "qrcode")] use matrix_sdk_qrcode::QrVerificationData; use matrix_sdk_test::async_test; - #[cfg(feature = "qrcode")] - use ruma::events::key::verification::VerificationMethod; - use ruma::{event_id, room_id, to_device::DeviceIdOrAllDevices}; + use ruma::{ + event_id, events::key::verification::VerificationMethod, room_id, + to_device::DeviceIdOrAllDevices, UserId, + }; use super::VerificationRequest; use crate::{ @@ -1626,7 +1648,7 @@ mod tests { CancelContent, OutgoingContent, ReadyContent, RequestContent, StartContent, }, test::{alice_id, bob_id, setup_stores}, - FlowId, Verification, + FlowId, Verification, VerificationStore, }, OutgoingVerificationRequest, ReadOnlyDevice, VerificationRequestState, }; @@ -1689,29 +1711,9 @@ mod tests { let (alice_store, bob_store) = setup_stores().await; let bob_device = ReadOnlyDevice::from_account(&bob_store.account).await; - let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); - - let bob_request = VerificationRequest::new( - VerificationCache::new(), - bob_store, - flow_id, - alice_id(), - vec![], - None, - ); - - let request = bob_request.request_to_device(); - let content: OutgoingContent = request.try_into().unwrap(); - let content = RequestContent::try_from(&content).unwrap(); - let flow_id = bob_request.flow_id().to_owned(); - - let alice_request = VerificationRequest::from_request( - VerificationCache::new(), - alice_store, - bob_id(), - flow_id, - &content, - ); + // Set up the pair of verification requests + let bob_request = build_test_request(&bob_store, alice_id(), None); + let alice_request = build_incoming_verification_request(&alice_store, &bob_request); let outgoing_request = alice_request.cancel().unwrap(); @@ -1772,13 +1774,7 @@ mod tests { &(&content).into(), ); - let content: OutgoingContent = alice_request.accept().unwrap().try_into().unwrap(); - let content = ReadyContent::try_from(&content).unwrap(); - - bob_request.receive_ready(alice_id(), &content); - - assert!(bob_request.is_ready()); - assert!(alice_request.is_ready()); + do_accept_request(&alice_request, &bob_request, None); let (bob_sas, request) = bob_request.start_sas().await.unwrap().unwrap(); @@ -1807,37 +1803,10 @@ mod tests { let (alice_store, bob_store) = setup_stores().await; let bob_device = ReadOnlyDevice::from_account(&bob_store.account).await; - let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); - - let bob_request = VerificationRequest::new( - VerificationCache::new(), - bob_store, - flow_id, - alice_id(), - vec![], - None, - ); - - let request = bob_request.request_to_device(); - let content: OutgoingContent = request.try_into().unwrap(); - let content = RequestContent::try_from(&content).unwrap(); - let flow_id = bob_request.flow_id().to_owned(); - - let alice_request = VerificationRequest::from_request( - VerificationCache::new(), - alice_store, - bob_id(), - flow_id, - &content, - ); - - let content: OutgoingContent = alice_request.accept().unwrap().try_into().unwrap(); - let content = ReadyContent::try_from(&content).unwrap(); - - bob_request.receive_ready(alice_id(), &content); - - assert!(bob_request.is_ready()); - assert!(alice_request.is_ready()); + // Set up the pair of verification requests + let bob_request = build_test_request(&bob_store, alice_id(), None); + let alice_request = build_incoming_verification_request(&alice_store, &bob_request); + do_accept_request(&alice_request, &bob_request, None); let (bob_sas, request) = bob_request.start_sas().await.unwrap().unwrap(); @@ -1868,44 +1837,19 @@ mod tests { async fn can_scan_another_qr_after_creating_mine() { let (alice_store, bob_store) = setup_stores().await; - let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); - - // We setup the initial verification request - let bob_request = VerificationRequest::new( - VerificationCache::new(), - bob_store, - flow_id.clone(), + // Set up the pair of verification requests + let bob_request = build_test_request( + &bob_store, alice_id(), - vec![], Some(vec![VerificationMethod::QrCodeScanV1, VerificationMethod::QrCodeShowV1]), ); - - let request = bob_request.request_to_device(); - let content: OutgoingContent = request.try_into().unwrap(); - let content = RequestContent::try_from(&content).unwrap(); - - let alice_request = VerificationRequest::from_request( - VerificationCache::new(), - alice_store, - bob_id(), - flow_id, - &content, + let alice_request = build_incoming_verification_request(&alice_store, &bob_request); + do_accept_request( + &alice_request, + &bob_request, + Some(vec![VerificationMethod::QrCodeScanV1, VerificationMethod::QrCodeShowV1]), ); - let content: OutgoingContent = alice_request - .accept_with_methods(vec![ - VerificationMethod::QrCodeScanV1, - VerificationMethod::QrCodeShowV1, - ]) - .unwrap() - .try_into() - .unwrap(); - let content = ReadyContent::try_from(&content).unwrap(); - bob_request.receive_ready(alice_id(), &content); - - assert!(bob_request.is_ready()); - assert!(alice_request.is_ready()); - // Each side can start its own QR verification flow by generating QR code let alice_verification = alice_request.generate_qr_code().await.unwrap(); let bob_verification = bob_request.generate_qr_code().await.unwrap(); @@ -1945,47 +1889,10 @@ mod tests { async fn can_start_sas_after_generating_qr_code() { let (alice_store, bob_store) = setup_stores().await; - let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); - - // We setup the initial verification request - let bob_request = VerificationRequest::new( - VerificationCache::new(), - bob_store, - flow_id.clone(), - alice_id(), - vec![], - Some(vec![ - VerificationMethod::QrCodeScanV1, - VerificationMethod::QrCodeShowV1, - VerificationMethod::SasV1, - ]), - ); - - let request = bob_request.request_to_device(); - let content: OutgoingContent = request.try_into().unwrap(); - let content = RequestContent::try_from(&content).unwrap(); - - let alice_request = VerificationRequest::from_request( - VerificationCache::new(), - alice_store, - bob_id(), - flow_id, - &content, - ); - - let content: OutgoingContent = alice_request - .accept_with_methods(vec![ - VerificationMethod::QrCodeScanV1, - VerificationMethod::QrCodeShowV1, - ]) - .unwrap() - .try_into() - .unwrap(); - let content = ReadyContent::try_from(&content).unwrap(); - bob_request.receive_ready(alice_id(), &content); - - assert!(bob_request.is_ready()); - assert!(alice_request.is_ready()); + // Set up the pair of verification requests + let bob_request = build_test_request(&bob_store, alice_id(), Some(all_methods())); + let alice_request = build_incoming_verification_request(&alice_store, &bob_request); + do_accept_request(&alice_request, &bob_request, Some(all_methods())); // Each side can start its own QR verification flow by generating QR code let alice_verification = alice_request.generate_qr_code().await.unwrap(); @@ -2001,11 +1908,163 @@ mod tests { // Alice can now start SAS verification flow instead of QR without cancelling // the request - let (sas, _) = alice_request.start_sas().await.unwrap().unwrap(); + let (sas, request) = alice_request.start_sas().await.unwrap().unwrap(); assert_matches!( alice_request.state(), VerificationRequestState::Transitioned { verification: Verification::SasV1(_) } ); assert!(!sas.is_cancelled()); + + // Bob receives the SAS start + let content: OutgoingContent = request.try_into().unwrap(); + let content = StartContent::try_from(&content).unwrap(); + bob_request.receive_start(alice_id(), &content).await.unwrap(); + + // Bob should now have transitioned to SAS... + let bob_sas = assert_matches!( + bob_request.state(), + VerificationRequestState::Transitioned { verification: Verification::SasV1(sas) } => sas + ); + // ... and, more to the point, it should not be cancelled. + assert!(!bob_sas.is_cancelled()); + } + + #[async_test] + #[cfg(feature = "qrcode")] + async fn start_sas_after_scan_cancels_request() { + let (alice_store, bob_store) = setup_stores().await; + + // Set up the pair of verification requests + let bob_request = build_test_request(&bob_store, alice_id(), Some(all_methods())); + let alice_request = build_incoming_verification_request(&alice_store, &bob_request); + do_accept_request(&alice_request, &bob_request, Some(all_methods())); + + // Bob generates a QR code + let bob_verification = bob_request.generate_qr_code().await.unwrap().unwrap(); + assert_matches!( + bob_request.state(), + VerificationRequestState::Transitioned { verification: Verification::QrV1(_) } + ); + + // Now Alice scans Bob's code + let bob_qr_code = bob_verification.to_bytes().unwrap(); + let bob_qr_code = QrVerificationData::from_bytes(bob_qr_code).unwrap(); + let _ = alice_request.scan_qr_code(bob_qr_code).await.unwrap().unwrap(); + + let alice_qr = assert_matches!( + alice_request.state(), + VerificationRequestState::Transitioned { + verification: Verification::QrV1(v) + } => v + ); + assert!(alice_qr.reciprocated()); + + // But Bob wants to do an SAS verification! + let (_, request) = bob_request.start_sas().await.unwrap().unwrap(); + assert_matches!( + bob_request.state(), + VerificationRequestState::Transitioned { verification: Verification::SasV1(_) } + ); + + // Alice receives the SAS start + let content: OutgoingContent = request.try_into().unwrap(); + let content = StartContent::try_from(&content).unwrap(); + alice_request.receive_start(bob_id(), &content).await.unwrap(); + + // ... which should mean that her Qr is cancelled + assert!(alice_qr.is_cancelled()); + + // and she should now have a *cancelled* SAS verification + let alice_sas = assert_matches!( + alice_request.state(), + VerificationRequestState::Transitioned { verification: Verification::SasV1(sas) } => sas + ); + assert!(alice_sas.is_cancelled()); + } + + /// Build an outgoing Verification request + /// + /// # Arguments + /// + /// * `verification_store` - The `VerificationStore` for the user making the + /// request. + /// * `other_user_id` - The ID of the user we want to verify + /// * `methods` - A list of `VerificationMethods` to say we support. If + /// `None`, will use the default list. + fn build_test_request( + verification_store: &VerificationStore, + other_user_id: &UserId, + methods: Option>, + ) -> VerificationRequest { + VerificationRequest::new( + VerificationCache::new(), + verification_store.clone(), + FlowId::ToDevice("TEST_FLOW_ID".into()), + other_user_id, + vec![], + methods, + ) + } + + /// Given an outgoing `VerificationRequest`, create an incoming + /// `VerificationRequest` for the other side. + /// + /// Tells the outgoing request to generate an `m.key.verification.request` + /// to-device message, and uses it to build a new request for the incoming + /// side. + fn build_incoming_verification_request( + verification_store: &VerificationStore, + outgoing_request: &VerificationRequest, + ) -> VerificationRequest { + let request = outgoing_request.request_to_device(); + let content: OutgoingContent = request.try_into().unwrap(); + let content = RequestContent::try_from(&content).unwrap(); + + VerificationRequest::from_request( + VerificationCache::new(), + verification_store.clone(), + outgoing_request.own_user_id(), + outgoing_request.flow_id().clone(), + &content, + ) + } + + /// Have a `VerificationRequest` generate an `m.key.verification.ready` and + /// feed it into another `VerificationRequest`. + /// + /// # Arguments + /// + /// * `accepting_request` - The request which should send the acceptance. + /// * `initiating_request` - The request which initiated the flow -- i.e., + /// the one that should *receive* the acceptance. + /// * `methods` - The list of methods to say we support. If `None`, the + /// default list of methods will be used. + fn do_accept_request( + accepting_request: &VerificationRequest, + initiating_request: &VerificationRequest, + methods: Option>, + ) { + let request = match methods { + Some(methods) => accepting_request.accept_with_methods(methods), + None => accepting_request.accept(), + }; + let content: OutgoingContent = request.unwrap().try_into().unwrap(); + let content = ReadyContent::try_from(&content).unwrap(); + initiating_request.receive_ready(accepting_request.own_user_id(), &content); + + assert!(initiating_request.is_ready()); + assert!(accepting_request.is_ready()); + } + + /// Get a list of all the verification methods, including those used for QR + /// codes. + #[cfg(feature = "qrcode")] + fn all_methods() -> Vec { + vec![ + VerificationMethod::SasV1, + VerificationMethod::QrCodeScanV1, + VerificationMethod::QrCodeShowV1, + VerificationMethod::ReciprocateV1, + ] } }