From 149606e148ab7461115dd41269f5319b5f07aa5c Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 23 Apr 2024 17:46:53 +0200 Subject: [PATCH 1/7] Avoid incorrect usage of private backup key This fixes instances of key backup corruption and prevents inadvertently logging the private backup key to the logs. --- crates/matrix-sdk-crypto/src/machine.rs | 74 +++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 99a3c7455..f40efcb73 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -317,10 +317,9 @@ impl OlmMachine { let saved_keys = store.load_backup_keys().await?; let maybe_backup_key = saved_keys.decryption_key.and_then(|k| { if let Some(version) = saved_keys.backup_version { - MegolmV1BackupKey::from_base64(&k.to_base64()).ok().map(|k| { - k.set_version(version); - k - }) + let megolm_v1_backup_key = k.megolm_v1_public_key(); + megolm_v1_backup_key.set_version(version); + Some(megolm_v1_backup_key) } else { None } @@ -2241,8 +2240,10 @@ pub(crate) mod tests { use crate::{ error::{EventError, SetRoomSettingsError}, machine::{EncryptionSyncChanges, OlmMachine}, - olm::{InboundGroupSession, OutboundGroupSession, VerifyJson}, - store::{Changes, RoomSettings}, + olm::{ + BackedUpRoomKey, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, VerifyJson, + }, + store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore, RoomSettings}, types::{ events::{ room::encrypted::{EncryptedToDeviceEvent, ToDeviceEncryptedEventContent}, @@ -4336,4 +4337,65 @@ pub(crate) mod tests { assert_matches!(encryption_result, Err(OlmError::MissingSession)); } + + #[async_test] + async fn test_fix_incorrect_usage_of_backup_key_causing_decryption_errors() { + let store = MemoryStore::new(); + + let backup_decryption_key = BackupDecryptionKey::new().unwrap(); + + store + .save_changes(Changes { + backup_decryption_key: Some(backup_decryption_key.clone()), + backup_version: Some("1".to_owned()), + ..Default::default() + }) + .await + .unwrap(); + + // Some valid key data + let data = json!({ + "algorithm": "m.megolm.v1.aes-sha2", + "room_id": "!room:id", + "sender_key": "FOvlmz18LLI3k/llCpqRoKT90+gFF8YhuL+v1YBXHlw", + "session_id": "/2K+V777vipCxPZ0gpY9qcpz1DYaXwuMRIu0UEP0Wa0", + "session_key": "AQAAAAAclzWVMeWBKH+B/WMowa3rb4ma3jEl6n5W4GCs9ue65CruzD3ihX+85pZ9hsV9Bf6fvhjp76WNRajoJYX0UIt7aosjmu0i+H+07hEQ0zqTKpVoSH0ykJ6stAMhdr6Q4uW5crBmdTTBIsqmoWsNJZKKoE2+ldYrZ1lrFeaJbjBIY/9ivle++74qQsT2dIKWPanKc9Q2Gl8LjESLtFBD9Fmt", + "sender_claimed_keys": { + "ed25519": "F4P7f1Z0RjbiZMgHk1xBCG3KC4/Ng9PmxLJ4hQ13sHA" + }, + "forwarding_curve25519_key_chain": ["DBPC2zr6c9qimo9YRFK3RVr0Two/I6ODb9mbsToZN3Q", "bBc/qzZFOOKshMMT+i4gjS/gWPDoKfGmETs9yfw9430"] + }); + + let backed_up_room_key: BackedUpRoomKey = serde_json::from_value(data).unwrap(); + + // Create the machine using `with_store` and without a call to enable_backup_v1, + // like regenerate_olm would do + let alice = OlmMachine::with_store(user_id(), alice_device_id(), store).await.unwrap(); + + let exported_key = ExportedRoomKey::from_backed_up_room_key( + room_id!("!room:id").to_owned(), + "/2K+V777vipCxPZ0gpY9qcpz1DYaXwuMRIu0UEP0Wa0".into(), + backed_up_room_key, + ); + + alice.store().import_exported_room_keys(vec![exported_key], |_, _| {}).await.unwrap(); + + let (_, request) = alice.backup_machine().backup().await.unwrap().unwrap(); + + let key_backup_data = request.rooms[&room_id!("!room:id").to_owned()] + .sessions + .get("/2K+V777vipCxPZ0gpY9qcpz1DYaXwuMRIu0UEP0Wa0") + .unwrap() + .deserialize() + .unwrap(); + + let ephemeral = key_backup_data.session_data.ephemeral.encode(); + let ciphertext = key_backup_data.session_data.ciphertext.encode(); + let mac = key_backup_data.session_data.mac.encode(); + + // Without the fix would generate a Mac(MacError) + backup_decryption_key + .decrypt_v1(&ephemeral, &mac, &ciphertext) + .expect("The backed up key should be decrypted successfully"); + } } From f4772c9b0e51a04aae08f15020a13cdceffbe4fb Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 10:20:08 +0200 Subject: [PATCH 2/7] Add FIXME comment --- crates/matrix-sdk-crypto/src/machine.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index f40efcb73..0f947856f 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -314,6 +314,10 @@ impl OlmMachine { } }; + // FIXME: This is a workaround due to `regenerate_olm` that was clearing backup + // state. Backups shouldn't be automatically enabled. + // The OlmMachine doesn't get enough info from the homeserver for this to work + // reliably. I don't belong here. let saved_keys = store.load_backup_keys().await?; let maybe_backup_key = saved_keys.decryption_key.and_then(|k| { if let Some(version) = saved_keys.backup_version { From e3dc094be4cb30ddb3dabfcecba26ef02498f028 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 10:20:19 +0200 Subject: [PATCH 3/7] Add minimal reproducing test --- crates/matrix-sdk-crypto/src/backups/mod.rs | 34 +++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/backups/mod.rs b/crates/matrix-sdk-crypto/src/backups/mod.rs index 4d6470f48..48181380a 100644 --- a/crates/matrix-sdk-crypto/src/backups/mod.rs +++ b/crates/matrix-sdk-crypto/src/backups/mod.rs @@ -627,8 +627,10 @@ mod tests { use super::BackupMachine; use crate::{ - olm::BackedUpRoomKey, store::BackupDecryptionKey, types::RoomKeyBackupInfo, OlmError, - OlmMachine, + olm::BackedUpRoomKey, + store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore}, + types::RoomKeyBackupInfo, + OlmError, OlmMachine, }; fn room_key() -> BackedUpRoomKey { @@ -863,4 +865,32 @@ mod tests { assert!(result.trusted()); } + + #[async_test] + async fn test_fix_backup_key_mismatch() { + let store = MemoryStore::new(); + + let backup_decryption_key = BackupDecryptionKey::new().unwrap(); + + store + .save_changes(Changes { + backup_decryption_key: Some(backup_decryption_key.clone()), + backup_version: Some("1".to_owned()), + ..Default::default() + }) + .await + .unwrap(); + + // Create the machine using `with_store` and without a call to enable_backup_v1, + // like regenerate_olm would do + let alice = OlmMachine::with_store(alice_id(), alice_device_id(), store).await.unwrap(); + + let binding = alice.backup_machine().backup_key.read().await; + let machine_backup_key = binding.as_ref().unwrap(); + + assert_eq!( + machine_backup_key.to_base64(), + backup_decryption_key.megolm_v1_public_key().to_base64() + ); + } } From 4e9edcb0db31c2a26b9d1dd0a888cf6b4d298253 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 14:42:52 +0200 Subject: [PATCH 4/7] Review: better comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Damir Jelić Signed-off-by: Valere --- crates/matrix-sdk-crypto/src/machine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 0f947856f..7885377e3 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -317,7 +317,7 @@ impl OlmMachine { // FIXME: This is a workaround due to `regenerate_olm` that was clearing backup // state. Backups shouldn't be automatically enabled. // The OlmMachine doesn't get enough info from the homeserver for this to work - // reliably. I don't belong here. + // reliably. let saved_keys = store.load_backup_keys().await?; let maybe_backup_key = saved_keys.decryption_key.and_then(|k| { if let Some(version) = saved_keys.backup_version { From d0776819c7211d321965344c1636e570c2eb64e8 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 14:43:15 +0200 Subject: [PATCH 5/7] Review: Add assert error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Damir Jelić Signed-off-by: Valere --- crates/matrix-sdk-crypto/src/backups/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/backups/mod.rs b/crates/matrix-sdk-crypto/src/backups/mod.rs index 48181380a..bf9bebfb9 100644 --- a/crates/matrix-sdk-crypto/src/backups/mod.rs +++ b/crates/matrix-sdk-crypto/src/backups/mod.rs @@ -890,7 +890,8 @@ mod tests { assert_eq!( machine_backup_key.to_base64(), - backup_decryption_key.megolm_v1_public_key().to_base64() + backup_decryption_key.megolm_v1_public_key().to_base64(), + "The OlmMachine loaded the wrong backup key." ); } } From a7cc3777d15323ad816f3331dd9fb2a16a2af0e9 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 10 May 2024 15:49:02 +0200 Subject: [PATCH 6/7] Review: better comment Co-authored-by: Denis Kasak Signed-off-by: Valere --- crates/matrix-sdk-crypto/src/machine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 7885377e3..5001ebb20 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -4397,7 +4397,7 @@ pub(crate) mod tests { let ciphertext = key_backup_data.session_data.ciphertext.encode(); let mac = key_backup_data.session_data.mac.encode(); - // Without the fix would generate a Mac(MacError) + // Prior to the fix for GHSA-9ggc-845v-gcgv, this would produce a `Mac(MacError)` backup_decryption_key .decrypt_v1(&ephemeral, &mac, &ciphertext) .expect("The backed up key should be decrypted successfully"); From df5e8e724e62ec7a03da58972c7af49e14981d5a Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 10 May 2024 15:49:20 +0200 Subject: [PATCH 7/7] Review: better comments Co-authored-by: Denis Kasak Signed-off-by: Valere --- crates/matrix-sdk-crypto/src/machine.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 5001ebb20..d8d933e8c 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -314,10 +314,10 @@ impl OlmMachine { } }; - // FIXME: This is a workaround due to `regenerate_olm` that was clearing backup - // state. Backups shouldn't be automatically enabled. - // The OlmMachine doesn't get enough info from the homeserver for this to work - // reliably. + // FIXME: This is a workaround for `regenerate_olm` clearing the backup + // state. Ideally, backups should not get automatically enabled since + // the `OlmMachine` doesn't get enough info from the homeserver for this + // to work reliably. let saved_keys = store.load_backup_keys().await?; let maybe_backup_key = saved_keys.decryption_key.and_then(|k| { if let Some(version) = saved_keys.backup_version {