From fd705b7d5e5efb4535f834b9218e5b08423f8449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Tue, 8 Dec 2020 16:01:42 +0100 Subject: [PATCH] crypto: Canonicalize the start event content before calculating the commitment This fixes: #117. --- matrix_sdk_common/src/lib.rs | 2 +- .../src/verification/sas/helpers.rs | 59 +++++++++++++++++-- .../src/verification/sas/sas_state.rs | 32 +++++----- 3 files changed, 71 insertions(+), 22 deletions(-) diff --git a/matrix_sdk_common/src/lib.rs b/matrix_sdk_common/src/lib.rs index 92002cfc..13655d0a 100644 --- a/matrix_sdk_common/src/lib.rs +++ b/matrix_sdk_common/src/lib.rs @@ -8,7 +8,7 @@ pub use ruma::{ AuthScheme, EndpointError, OutgoingRequest, }, directory, encryption, events, identifiers, presence, push, - serde::Raw, + serde::{CanonicalJsonValue, Raw}, thirdparty, Outgoing, }; diff --git a/matrix_sdk_crypto/src/verification/sas/helpers.rs b/matrix_sdk_crypto/src/verification/sas/helpers.rs index 106727b8..5a85e958 100644 --- a/matrix_sdk_crypto/src/verification/sas/helpers.rs +++ b/matrix_sdk_crypto/src/verification/sas/helpers.rs @@ -16,16 +16,19 @@ use std::{collections::BTreeMap, convert::TryInto}; use tracing::{trace, warn}; -use olm_rs::sas::OlmSas; +use olm_rs::{sas::OlmSas, utility::OlmUtility}; use matrix_sdk_common::{ api::r0::to_device::DeviceIdOrAllDevices, events::{ - key::verification::{cancel::CancelCode, mac::MacToDeviceEventContent}, + key::verification::{ + cancel::CancelCode, mac::MacToDeviceEventContent, start::StartToDeviceEventContent, + }, AnyToDeviceEventContent, EventType, ToDeviceEvent, }, identifiers::{DeviceId, DeviceKeyAlgorithm, DeviceKeyId, UserId}, uuid::Uuid, + CanonicalJsonValue, }; use crate::{ @@ -40,6 +43,26 @@ pub struct SasIds { pub other_identity: Option, } +/// Calculate the commitment for a accept event from the public key and the +/// start event. +/// +/// # Arguments +/// +/// * `public_key` - Our own ephemeral public key that is used for the +/// interactive verification. +/// +/// * `content` - The `m.key.verification.start` event content that started the +/// interactive verification process. +pub fn calculate_commitment(public_key: &str, content: &StartToDeviceEventContent) -> String { + let json_content: CanonicalJsonValue = serde_json::to_value(content) + .expect("Can't serialize content") + .try_into() + .expect("Can't canonicalize content"); + + let utility = OlmUtility::new(); + utility.sha256_utf8_msg(&format!("{}{}", public_key, json_content)) +} + /// Get a tuple of an emoji and a description of the emoji using a number. /// /// This is taken directly from the [spec] @@ -493,12 +516,38 @@ pub fn content_to_request( #[cfg(test)] mod test { + use matrix_sdk_common::events::key::verification::start::StartToDeviceEventContent; use proptest::prelude::*; + use serde_json::json; - use super::{bytes_to_decimal, bytes_to_emoji, bytes_to_emoji_index, emoji_from_index}; + use super::{ + bytes_to_decimal, bytes_to_emoji, bytes_to_emoji_index, calculate_commitment, + emoji_from_index, + }; #[test] - fn test_emoji_generation() { + fn commitment_calculation() { + let commitment = "CCQmB4JCdB0FW21FdAnHj/Hu8+W9+Nb0vgwPEnZZQ4g"; + + let public_key = "Q/NmNFEUS1fS+YeEmiZkjjblKTitrKOAk7cPEumcMlg"; + let content = json!({ + "from_device":"XOWLHHFSWM", + "transaction_id":"bYxBsirjUJO9osar6ST4i2M2NjrYLA7l", + "method":"m.sas.v1", + "key_agreement_protocols":["curve25519-hkdf-sha256","curve25519"], + "hashes":["sha256"], + "message_authentication_codes":["hkdf-hmac-sha256","hmac-sha256"], + "short_authentication_string":["decimal","emoji"] + }); + + let content: StartToDeviceEventContent = serde_json::from_value(content).unwrap(); + let calculated_commitment = calculate_commitment(public_key, &content); + + assert_eq!(commitment, &calculated_commitment); + } + + #[test] + fn emoji_generation() { let bytes = vec![0, 0, 0, 0, 0, 0]; let index: Vec<(&'static str, &'static str)> = vec![0, 0, 0, 0, 0, 0, 0] .into_iter() @@ -516,7 +565,7 @@ mod test { } #[test] - fn test_decimal_generation() { + fn decimal_generation() { let bytes = vec![0, 0, 0, 0, 0]; let result = bytes_to_decimal(bytes); diff --git a/matrix_sdk_crypto/src/verification/sas/sas_state.rs b/matrix_sdk_crypto/src/verification/sas/sas_state.rs index 90966008..ab136cae 100644 --- a/matrix_sdk_crypto/src/verification/sas/sas_state.rs +++ b/matrix_sdk_crypto/src/verification/sas/sas_state.rs @@ -19,7 +19,7 @@ use std::{ time::{Duration, Instant}, }; -use olm_rs::{sas::OlmSas, utility::OlmUtility}; +use olm_rs::sas::OlmSas; use matrix_sdk_common::{ events::{ @@ -40,8 +40,11 @@ use matrix_sdk_common::{ identifiers::{DeviceId, UserId}, uuid::Uuid, }; +use tracing::error; -use super::helpers::{get_decimal, get_emoji, get_mac_content, receive_mac_event, SasIds}; +use super::helpers::{ + calculate_commitment, get_decimal, get_emoji, get_mac_content, receive_mac_event, SasIds, +}; use crate::{ identities::{ReadOnlyDevice, UserIdentities}, @@ -176,7 +179,7 @@ pub struct Started { #[derive(Clone, Debug)] pub struct Accepted { accepted_protocols: Arc, - json_start_content: String, + start_content: Arc, commitment: String, } @@ -348,8 +351,7 @@ impl SasState { let accepted_protocols = AcceptedProtocols::try_from(content.clone()).map_err(|c| self.clone().cancel(c))?; - let json_start_content = serde_json::to_string(&self.as_content()) - .expect("Can't deserialize start event content"); + let start_content = self.as_content().into(); Ok(SasState { inner: self.inner, @@ -358,9 +360,9 @@ impl SasState { creation_time: self.creation_time, last_event_time: self.last_event_time, state: Arc::new(Accepted { - json_start_content, + start_content, commitment: content.commitment.clone(), - accepted_protocols: Arc::new(accepted_protocols), + accepted_protocols: accepted_protocols.into(), }), }) } else { @@ -391,12 +393,14 @@ impl SasState { ) -> Result, SasState> { if let StartMethod::MSasV1(content) = &event.content.method { let sas = OlmSas::new(); - let utility = OlmUtility::new(); - let json_content = - serde_json::to_string(&event.content).expect("Can't serialize content"); let pubkey = sas.public_key(); - let commitment = utility.sha256_utf8_msg(&format!("{}{}", pubkey, json_content)); + let commitment = calculate_commitment(&pubkey, &event.content); + + error!( + "Calculated commitment for pubkey {} and content {:?} {}", + pubkey, event.content, commitment + ); let sas = SasState { inner: Arc::new(Mutex::new(sas)), @@ -540,11 +544,7 @@ impl SasState { self.check_event(&event.sender, &event.content.transaction_id) .map_err(|c| self.clone().cancel(c))?; - let utility = OlmUtility::new(); - let commitment = utility.sha256_utf8_msg(&format!( - "{}{}", - event.content.key, self.state.json_start_content - )); + let commitment = calculate_commitment(&event.content.key, &self.state.start_content); if self.state.commitment != commitment { Err(self.cancel(CancelCode::InvalidMessage))