From eab35c1289d9312ee55044f840ff6bf9057872d3 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 22 Jan 2024 11:44:23 +0100 Subject: [PATCH 1/2] chore: Update Ruma to 684ffc. This patch updates Ruma to the latest commit on its `main` branch. This is useful for https://github.com/matrix-org/matrix-rust-sdk/issues/2932. --- Cargo.lock | 19 +++--- Cargo.toml | 4 +- .../matrix-sdk-crypto/src/secret_storage.rs | 58 +++++++++++-------- 3 files changed, 46 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e7b924df..3a4742336 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4734,7 +4734,7 @@ dependencies = [ [[package]] name = "ruma" version = "0.9.4" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "assign", "js_int", @@ -4750,8 +4750,9 @@ dependencies = [ [[package]] name = "ruma-client-api" version = "0.17.4" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ + "as_variant", "assign", "bytes", "http", @@ -4768,7 +4769,7 @@ dependencies = [ [[package]] name = "ruma-common" version = "0.12.1" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "as_variant", "base64 0.21.5", @@ -4798,7 +4799,7 @@ dependencies = [ [[package]] name = "ruma-events" version = "0.27.11" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "as_variant", "indexmap 2.1.0", @@ -4822,7 +4823,7 @@ dependencies = [ [[package]] name = "ruma-federation-api" version = "0.8.0" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "js_int", "ruma-common", @@ -4834,7 +4835,7 @@ dependencies = [ [[package]] name = "ruma-html" version = "0.1.0" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "as_variant", "html5ever", @@ -4846,7 +4847,7 @@ dependencies = [ [[package]] name = "ruma-identifiers-validation" version = "0.9.3" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "js_int", "thiserror", @@ -4855,7 +4856,7 @@ dependencies = [ [[package]] name = "ruma-macros" version = "0.12.0" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "once_cell", "proc-macro-crate 2.0.1", @@ -4870,7 +4871,7 @@ dependencies = [ [[package]] name = "ruma-push-gateway-api" version = "0.8.0" -source = "git+https://github.com/ruma/ruma?rev=a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4#a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4" +source = "git+https://github.com/ruma/ruma?rev=684ffc789877355bd25269451b2356817c17cc3f#684ffc789877355bd25269451b2356817c17cc3f" dependencies = [ "js_int", "ruma-common", diff --git a/Cargo.toml b/Cargo.toml index db231d420..bff1526c8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,8 +36,8 @@ futures-executor = "0.3.21" futures-util = { version = "0.3.26", default-features = false, features = ["alloc"] } http = "0.2.6" itertools = "0.12.0" -ruma = { git = "https://github.com/ruma/ruma", rev = "a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4", features = ["client-api-c", "compat-upload-signatures", "compat-user-id", "compat-arbitrary-length-ids", "unstable-msc3401"] } -ruma-common = { git = "https://github.com/ruma/ruma", rev = "a7e1d7fa49d93f4d9cbc369aa28378a89b3dc9e4"} +ruma = { git = "https://github.com/ruma/ruma", rev = "684ffc789877355bd25269451b2356817c17cc3f", features = ["client-api-c", "compat-upload-signatures", "compat-user-id", "compat-arbitrary-length-ids", "unstable-msc3401"] } +ruma-common = { git = "https://github.com/ruma/ruma", rev = "684ffc789877355bd25269451b2356817c17cc3f"} once_cell = "1.16.0" rand = "0.8.5" serde = "1.0.151" diff --git a/crates/matrix-sdk-crypto/src/secret_storage.rs b/crates/matrix-sdk-crypto/src/secret_storage.rs index 9bb195f88..b848d1583 100644 --- a/crates/matrix-sdk-crypto/src/secret_storage.rs +++ b/crates/matrix-sdk-crypto/src/secret_storage.rs @@ -245,29 +245,39 @@ impl SecretStorageKey { fn check_zero_message(&self) -> Result<(), DecodeError> { match &self.storage_key_info.algorithm { SecretStorageEncryptionAlgorithm::V1AesHmacSha2(properties) => { - if properties.iv.as_bytes().len() != IV_SIZE { - Err(DecodeError::IvLength(IV_SIZE, properties.iv.as_bytes().len())) - } else { - let mut iv_array = [0u8; 16]; - iv_array.copy_from_slice(properties.iv.as_bytes()); + let Some(iv) = &properties.iv else { + return Err(DecodeError::IvLength(IV_SIZE, 0)); + }; - // I'm not particularly convinced that this couldn't have been done simpler. Why - // do we need to reproduce the ciphertext? Couldn't we just generate the MAC tag - // using the `ZERO_MESSAGE`? - // - // If someone is reading this and is designing a new secret encryption - // algorithm, please consider the above suggestion. - let key = AesHmacSha2Key::from_secret_storage_key(&self.secret_key, ""); - let ciphertext = key.apply_keystream(Self::ZERO_MESSAGE.to_vec(), &iv_array); - let expected_mac = HmacSha256Mac::from_slice(properties.mac.as_bytes()) - .ok_or_else(|| { - DecodeError::MacLength(MAC_SIZE, properties.mac.as_bytes().len()) - })?; + let Some(mac) = &properties.mac else { + return Err(DecodeError::MacLength(MAC_SIZE, 0)); + }; - key.verify_mac(&ciphertext, expected_mac.as_bytes())?; + let iv = iv.as_bytes(); + let iv_length = iv.len(); - Ok(()) + if iv_length != IV_SIZE { + return Err(DecodeError::IvLength(IV_SIZE, iv_length)); } + + let mut iv_array = [0u8; 16]; + iv_array.copy_from_slice(iv); + + // I'm not particularly convinced that this couldn't have been done simpler. + // Why do we need to reproduce the ciphertext? + // Couldn't we just generate the MAC tag + // using the `ZERO_MESSAGE`? + // + // If someone is reading this and is designing a new secret encryption + // algorithm, please consider the above suggestion. + let key = AesHmacSha2Key::from_secret_storage_key(&self.secret_key, ""); + let ciphertext = key.apply_keystream(Self::ZERO_MESSAGE.to_vec(), &iv_array); + let expected_mac = HmacSha256Mac::from_slice(mac.as_bytes()) + .ok_or_else(|| DecodeError::MacLength(MAC_SIZE, mac.as_bytes().len()))?; + + key.verify_mac(&ciphertext, expected_mac.as_bytes())?; + + Ok(()) } custom => Err(DecodeError::UnsupportedAlgorithm(custom.algorithm().to_owned())), } @@ -283,7 +293,7 @@ impl SecretStorageKey { SecretStorageKeyEventContent::new( key_id, SecretStorageEncryptionAlgorithm::V1AesHmacSha2( - SecretStorageV1AesHmacSha2Properties::new(iv, mac), + SecretStorageV1AesHmacSha2Properties::new(Some(iv), Some(mac)), ), ) } @@ -862,8 +872,8 @@ mod test { "bmur2d9ypPUH1msSwCxQOJkuKRmJI55e".to_owned(), SecretStorageEncryptionAlgorithm::V1AesHmacSha2( SecretStorageV1AesHmacSha2Properties::new( - Base64::new(vec![0u8; 14]), - Base64::new(vec![0u8; 32]), + Some(Base64::new(vec![0u8; 14])), + Some(Base64::new(vec![0u8; 32])), ), ), ); @@ -878,8 +888,8 @@ mod test { "bmur2d9ypPUH1msSwCxQOJkuKRmJI55e".to_owned(), SecretStorageEncryptionAlgorithm::V1AesHmacSha2( SecretStorageV1AesHmacSha2Properties::new( - Base64::new(vec![0u8; 16]), - Base64::new(vec![0u8; 10]), + Some(Base64::new(vec![0u8; 16])), + Some(Base64::new(vec![0u8; 10])), ), ), ); From 8515def61b59cb735be4d8837d4a7e8e6ccc2701 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 22 Jan 2024 15:00:23 +0100 Subject: [PATCH 2/2] fix(crypto): It's OK to have `iv` and `mac` missing. This patch updates `SecretStorageKey::check_zero_message` to assume that a missing `iv` and/or `mac` is valid, instead of an error, as the spec suggests. --- crates/matrix-sdk-crypto/src/secret_storage.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/secret_storage.rs b/crates/matrix-sdk-crypto/src/secret_storage.rs index b848d1583..bf94f559f 100644 --- a/crates/matrix-sdk-crypto/src/secret_storage.rs +++ b/crates/matrix-sdk-crypto/src/secret_storage.rs @@ -245,12 +245,13 @@ impl SecretStorageKey { fn check_zero_message(&self) -> Result<(), DecodeError> { match &self.storage_key_info.algorithm { SecretStorageEncryptionAlgorithm::V1AesHmacSha2(properties) => { - let Some(iv) = &properties.iv else { - return Err(DecodeError::IvLength(IV_SIZE, 0)); - }; - - let Some(mac) = &properties.mac else { - return Err(DecodeError::MacLength(MAC_SIZE, 0)); + let (Some(iv), Some(mac)) = (&properties.iv, &properties.mac) else { + // The IV and/or MAC are missing from the account data + // content. As the [spec] says, we have to assume that the + // key is valid. + // + // [spec]: https://spec.matrix.org/unstable/client-server-api/#msecret_storagev1aes-hmac-sha2 + return Ok(()); }; let iv = iv.as_bytes();