Skip to content

Commit

Permalink
fix(crypto): Avoid incorrect usage of private backup key
Browse files Browse the repository at this point in the history
This fixes instances of key backup corruption and prevents inadvertently
logging the private backup key to the logs.
  • Loading branch information
BillCarsonFr authored and poljar committed May 13, 2024
1 parent 4164eff commit fa10bbb
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 8 deletions.
35 changes: 33 additions & 2 deletions crates/matrix-sdk-crypto/src/backups/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -626,8 +626,10 @@ mod tests {
use serde_json::json;

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 {
Expand Down Expand Up @@ -850,4 +852,33 @@ 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(),
"The OlmMachine loaded the wrong backup key."
);
}
}
78 changes: 72 additions & 6 deletions crates/matrix-sdk-crypto/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,16 @@ impl OlmMachine {
}
};

// 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 {
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
}
Expand Down Expand Up @@ -2169,8 +2172,10 @@ pub(crate) mod tests {
use crate::{
error::EventError,
machine::{EncryptionSyncChanges, OlmMachine},
olm::{InboundGroupSession, OutboundGroupSession, VerifyJson},
store::Changes,
olm::{
BackedUpRoomKey, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, VerifyJson,
},
store::{BackupDecryptionKey, Changes, CryptoStore, MemoryStore, RoomSettings},
types::{
events::{
room::encrypted::{EncryptedToDeviceEvent, ToDeviceEncryptedEventContent},
Expand Down Expand Up @@ -4071,4 +4076,65 @@ pub(crate) mod tests {
// The waiting should successfully complete.
wait.await.unwrap();
}

#[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();

// 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");
}
}

0 comments on commit fa10bbb

Please # to comment.