crypto: Canonicalize the start event content before calculating the commitment

This fixes: #117.

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,
 };
 

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<UserIdentities>,
 }
 
+/// 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);
 

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<AcceptedProtocols>,
-    json_start_content: String,
+    start_content: Arc<StartToDeviceEventContent>,
     commitment: String,
 }
 
@@ -348,8 +351,7 @@ impl SasState<Created> {
             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())
+            let start_content = self.as_content().into();
-                .expect("Can't deserialize start event content");
 
             Ok(SasState {
                 inner: self.inner,
@@ -358,9 +360,9 @@ impl SasState<Created> {
                 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<Started> {
     ) -> Result<SasState<Started>, SasState<Canceled>> {
         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<Accepted> {
         self.check_event(&event.sender, &event.content.transaction_id)
             .map_err(|c| self.clone().cancel(c))?;
 
-        let utility = OlmUtility::new();
+        let commitment = calculate_commitment(&event.content.key, &self.state.start_content);
-        let commitment = utility.sha256_utf8_msg(&format!(
-            "{}{}",
-            event.content.key, self.state.json_start_content
-        ));
 
         if self.state.commitment != commitment {
             Err(self.cancel(CancelCode::InvalidMessage))