From aac41fc82a0c0c46ee655a36d03cef4cec1ffb73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 12 Sep 2022 13:00:09 +0200 Subject: [PATCH] refactor(crypto): Remove the forwarding chains These aren't really useful since they can be easily spoofed by any room key forwarder. --- bindings/matrix-sdk-crypto-ffi/src/lib.rs | 3 -- bindings/matrix-sdk-crypto-ffi/src/machine.rs | 20 +++++------ .../matrix-sdk-crypto-js/src/responses.rs | 8 ++--- .../matrix-sdk-crypto-nodejs/src/responses.rs | 8 ++--- .../src/deserialized_responses.rs | 3 -- .../src/backups/keys/backup.rs | 6 ++-- .../src/gossiping/machine.rs | 2 +- crates/matrix-sdk-crypto/src/machine.rs | 5 --- .../src/olm/group_sessions/inbound.rs | 34 +++---------------- .../src/store/integration_tests.rs | 6 ---- 10 files changed, 22 insertions(+), 73 deletions(-) diff --git a/bindings/matrix-sdk-crypto-ffi/src/lib.rs b/bindings/matrix-sdk-crypto-ffi/src/lib.rs index 39244531a..229af1a3f 100644 --- a/bindings/matrix-sdk-crypto-ffi/src/lib.rs +++ b/bindings/matrix-sdk-crypto-ffi/src/lib.rs @@ -251,8 +251,6 @@ pub fn migrate( InboundGroupSession::from_libolm_pickle(&session.pickle, &data.pickle_key)?.pickle(); let sender_key = Curve25519PublicKey::from_base64(&session.sender_key)?; - let forwarding_chains: Result, _> = - session.forwarding_chains.iter().map(|k| Curve25519PublicKey::from_base64(k)).collect(); let pickle = matrix_sdk_crypto::olm::PickledInboundGroupSession { pickle, @@ -268,7 +266,6 @@ pub fn migrate( }) .collect::>()?, room_id: RoomId::parse(session.room_id)?, - forwarding_chains: forwarding_chains?, imported: session.imported, backed_up: session.backed_up, history_visibility: None, diff --git a/bindings/matrix-sdk-crypto-ffi/src/machine.rs b/bindings/matrix-sdk-crypto-ffi/src/machine.rs index 65ed31b0d..73722c37b 100644 --- a/bindings/matrix-sdk-crypto-ffi/src/machine.rs +++ b/bindings/matrix-sdk-crypto-ffi/src/machine.rs @@ -610,16 +610,16 @@ impl OlmMachine { let event_json: Event<'_> = serde_json::from_str(decrypted.event.json().get())?; Ok(match &encryption_info.algorithm_info { - AlgorithmInfo::MegolmV1AesSha2 { - curve25519_key, - sender_claimed_keys, - forwarding_curve25519_key_chain, - } => DecryptedEvent { - clear_event: serde_json::to_string(&event_json)?, - sender_curve25519_key: curve25519_key.to_owned(), - claimed_ed25519_key: sender_claimed_keys.get(&DeviceKeyAlgorithm::Ed25519).cloned(), - forwarding_curve25519_chain: forwarding_curve25519_key_chain.to_owned(), - }, + AlgorithmInfo::MegolmV1AesSha2 { curve25519_key, sender_claimed_keys } => { + DecryptedEvent { + clear_event: serde_json::to_string(&event_json)?, + sender_curve25519_key: curve25519_key.to_owned(), + claimed_ed25519_key: sender_claimed_keys + .get(&DeviceKeyAlgorithm::Ed25519) + .cloned(), + forwarding_curve25519_chain: vec![], + } + } }) } diff --git a/bindings/matrix-sdk-crypto-js/src/responses.rs b/bindings/matrix-sdk-crypto-js/src/responses.rs index 9d9fac1cf..888847f48 100644 --- a/bindings/matrix-sdk-crypto-js/src/responses.rs +++ b/bindings/matrix-sdk-crypto-js/src/responses.rs @@ -182,12 +182,8 @@ impl DecryptedRoomEvent { /// Chain of Curve25519 keys through which this session was /// forwarded, via `m.forwarded_room_key` events. #[wasm_bindgen(getter, js_name = "forwardingCurve25519KeyChain")] - pub fn forwarding_curve25519_key_chain(&self) -> Option { - Some(match &self.encryption_info.as_ref()?.algorithm_info { - AlgorithmInfo::MegolmV1AesSha2 { forwarding_curve25519_key_chain, .. } => { - forwarding_curve25519_key_chain.iter().map(JsValue::from).collect() - } - }) + pub fn forwarding_curve25519_key_chain(&self) -> Array { + Array::new() } /// The verification state of the device that sent us the event, diff --git a/bindings/matrix-sdk-crypto-nodejs/src/responses.rs b/bindings/matrix-sdk-crypto-nodejs/src/responses.rs index 30425197c..0efc96976 100644 --- a/bindings/matrix-sdk-crypto-nodejs/src/responses.rs +++ b/bindings/matrix-sdk-crypto-nodejs/src/responses.rs @@ -178,12 +178,8 @@ impl DecryptedRoomEvent { /// Chain of Curve25519 keys through which this session was /// forwarded, via `m.forwarded_room_key` events. #[napi(getter)] - pub fn forwarding_curve25519_key_chain(&self) -> Option> { - Some(match &self.encryption_info.as_ref()?.algorithm_info { - AlgorithmInfo::MegolmV1AesSha2 { forwarding_curve25519_key_chain, .. } => { - forwarding_curve25519_key_chain.clone() - } - }) + pub fn forwarding_curve25519_key_chain(&self) -> Vec { + vec![] } /// The verification state of the device that sent us the event, diff --git a/crates/matrix-sdk-common/src/deserialized_responses.rs b/crates/matrix-sdk-common/src/deserialized_responses.rs index 7ce534c22..a928d9132 100644 --- a/crates/matrix-sdk-common/src/deserialized_responses.rs +++ b/crates/matrix-sdk-common/src/deserialized_responses.rs @@ -68,9 +68,6 @@ pub enum AlgorithmInfo { /// decrypt this session. This map will usually contain a single ed25519 /// key. sender_claimed_keys: BTreeMap, - /// Chain of curve25519 keys through which this session was forwarded, - /// via m.forwarded_room_key events. - forwarding_curve25519_key_chain: Vec, }, } diff --git a/crates/matrix-sdk-crypto/src/backups/keys/backup.rs b/crates/matrix-sdk-crypto/src/backups/keys/backup.rs index 2de204e36..b7dac9a28 100644 --- a/crates/matrix-sdk-crypto/src/backups/keys/backup.rs +++ b/crates/matrix-sdk-crypto/src/backups/keys/backup.rs @@ -112,9 +112,9 @@ impl MegolmV1BackupKey { pub(crate) async fn encrypt(&self, session: InboundGroupSession) -> KeyBackupData { let pk = OlmPkEncryption::new(&self.to_base64()); - // It's ok to truncate here, there's a semantic difference only between - // 0 and 1+ anyways. - let forwarded_count = (session.forwarding_key_chain().len() as u32).into(); + // The forwarding chains don't mean much, we only care whether we received the + // session directly from the creator of the session or not. + let forwarded_count = (session.has_been_imported() as u8).into(); let first_message_index = session.first_known_index().into(); // Convert our key to the backup representation. diff --git a/crates/matrix-sdk-crypto/src/gossiping/machine.rs b/crates/matrix-sdk-crypto/src/gossiping/machine.rs index 4e2583303..b33d56361 100644 --- a/crates/matrix-sdk-crypto/src/gossiping/machine.rs +++ b/crates/matrix-sdk-crypto/src/gossiping/machine.rs @@ -879,7 +879,7 @@ impl GossipMachine { algorithm: EventEncryptionAlgorithm, content: &ForwardedMegolmV1AesSha2Content, ) -> Result, CryptoStoreError> { - match InboundGroupSession::from_forwarded_key(sender_key, &algorithm, content) { + match InboundGroupSession::from_forwarded_key(&algorithm, content) { Ok(session) => { let old_session = self .store diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 4cc2bcdf7..4b2a05fd3 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1068,11 +1068,6 @@ impl OlmMachine { .iter() .map(|(k, v)| (k.to_owned(), v.to_base64())) .collect(), - forwarding_curve25519_key_chain: session - .forwarding_key_chain() - .iter() - .map(|k| k.to_base64()) - .collect(), }, verification_state, }) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index 98c29e43f..7307c25ce 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -42,12 +42,12 @@ use super::{ use crate::{ error::{EventError, MegolmResult}, types::{ - deserialize_curve_key, deserialize_curve_key_vec, + deserialize_curve_key, events::{ forwarded_room_key::ForwardedMegolmV1AesSha2Content, room::encrypted::{EncryptedEvent, RoomEventEncryptionScheme}, }, - serialize_curve_key, serialize_curve_key_vec, EventEncryptionAlgorithm, SigningKeys, + serialize_curve_key, EventEncryptionAlgorithm, SigningKeys, }, }; @@ -72,7 +72,6 @@ pub struct InboundGroupSession { pub signing_keys: Arc>, /// The Room this GroupSession belongs to pub room_id: Arc, - forwarding_chains: Arc>, imported: bool, algorithm: Arc, backed_up: Arc, @@ -120,7 +119,6 @@ impl InboundGroupSession { sender_key, signing_keys: keys.into(), room_id: room_id.into(), - forwarding_chains: Vec::new().into(), imported: false, algorithm: encryption_algorithm.into(), backed_up: AtomicBool::new(false).into(), @@ -153,9 +151,9 @@ impl InboundGroupSession { room_id: room_id.to_owned(), sender_key: backup.sender_key, session_id, + forwarding_curve25519_key_chain: vec![], session_key: backup.session_key, sender_claimed_keys: backup.sender_claimed_keys, - forwarding_curve25519_key_chain: backup.forwarding_curve25519_key_chain, }) } @@ -169,7 +167,6 @@ impl InboundGroupSession { /// * `content` - A forwarded room key content that contains the session key /// to create the `InboundGroupSession`. pub fn from_forwarded_key( - sender_key: Curve25519PublicKey, algorithm: &EventEncryptionAlgorithm, content: &ForwardedMegolmV1AesSha2Content, ) -> Result { @@ -178,8 +175,6 @@ impl InboundGroupSession { let session = InnerSession::import(&content.session_key, config); let first_known_index = session.first_known_index(); - let mut forwarding_chains = content.forwarding_curve25519_key_chain.clone(); - forwarding_chains.push(sender_key); let mut sender_claimed_key = SigningKeys::new(); sender_claimed_key.insert(DeviceKeyAlgorithm::Ed25519, content.claimed_ed25519_key.into()); @@ -192,7 +187,6 @@ impl InboundGroupSession { history_visibility: None.into(), signing_keys: sender_claimed_key.into(), room_id: (*content.room_id).into(), - forwarding_chains: forwarding_chains.into(), imported: true, backed_up: AtomicBool::new(false).into(), algorithm: algorithm.to_owned().into(), @@ -213,7 +207,6 @@ impl InboundGroupSession { sender_key: self.sender_key, signing_key: (*self.signing_keys).clone(), room_id: (*self.room_id).to_owned(), - forwarding_chains: self.forwarding_key_chain().to_vec(), imported: self.imported, backed_up: self.backed_up(), history_visibility: self.history_visibility.as_ref().clone(), @@ -255,15 +248,6 @@ impl InboundGroupSession { &self.signing_keys } - /// Get the list of ed25519 keys that this session was forwarded through. - /// - /// Each ed25519 key represents a single device. If device A forwards the - /// session to device B and device B to C this list will contain the ed25519 - /// keys of A and B. - pub fn forwarding_key_chain(&self) -> &[Curve25519PublicKey] { - &self.forwarding_chains - } - /// Export this session at the given message index. pub async fn export_at_index(&self, message_index: u32) -> ExportedRoomKey { let message_index = std::cmp::max(self.first_known_index(), message_index); @@ -276,7 +260,7 @@ impl InboundGroupSession { room_id: (*self.room_id).to_owned(), sender_key: self.sender_key, session_id: self.session_id().to_owned(), - forwarding_curve25519_key_chain: self.forwarding_key_chain().to_vec(), + forwarding_curve25519_key_chain: vec![], sender_claimed_keys: (*self.signing_keys).clone(), session_key, } @@ -306,7 +290,6 @@ impl InboundGroupSession { first_known_index, signing_keys: pickle.signing_key.into(), room_id: (*pickle.room_id).into(), - forwarding_chains: pickle.forwarding_chains.into(), backed_up: AtomicBool::from(pickle.backed_up).into(), algorithm: pickle.algorithm.into(), imported: pickle.imported, @@ -463,14 +446,6 @@ pub struct PickledInboundGroupSession { pub signing_key: SigningKeys, /// The id of the room that the session is used in. pub room_id: OwnedRoomId, - /// The list of claimed Curve25519 keys that forwarded us this key. Will be - /// empty if we directly received this session. - #[serde( - default, - deserialize_with = "deserialize_curve_key_vec", - serialize_with = "serialize_curve_key_vec" - )] - pub forwarding_chains: Vec, /// Flag remembering if the session was directly sent to us by the sender /// or if it was imported. pub imported: bool, @@ -504,7 +479,6 @@ impl TryFrom<&ExportedRoomKey> for InboundGroupSession { first_known_index, signing_keys: key.sender_claimed_keys.to_owned().into(), room_id: key.room_id.to_owned().into(), - forwarding_chains: key.forwarding_curve25519_key_chain.to_owned().into(), imported: true, algorithm: key.algorithm.to_owned().into(), backed_up: AtomicBool::from(false).into(), diff --git a/crates/matrix-sdk-crypto/src/store/integration_tests.rs b/crates/matrix-sdk-crypto/src/store/integration_tests.rs index 871e1a336..888419f13 100644 --- a/crates/matrix-sdk-crypto/src/store/integration_tests.rs +++ b/crates/matrix-sdk-crypto/src/store/integration_tests.rs @@ -303,11 +303,6 @@ macro_rules! cryptostore_integration_tests { let mut export = session.export().await; - let curve_key = - Curve25519PublicKey::from_base64("Nn0L2hkcCMFKqynTjyGsJbth7QrVmX3lbrksMkrGOAw") - .unwrap(); - export.forwarding_curve25519_key_chain = vec![curve_key]; - let session = InboundGroupSession::from_export(&export).unwrap(); let changes = @@ -332,7 +327,6 @@ macro_rules! cryptostore_integration_tests { .unwrap(); assert_eq!(session, loaded_session); let export = loaded_session.export().await; - assert!(!export.forwarding_curve25519_key_chain.is_empty()); assert_eq!(store.get_inbound_group_sessions().await.unwrap().len(), 1); assert_eq!(store.inbound_group_session_counts().await.unwrap().total, 1);