From 96c79e430498a3e3e8a506fdb6e38c953575c877 Mon Sep 17 00:00:00 2001 From: Niklas Baumstark Date: Fri, 18 Apr 2025 16:59:00 +0200 Subject: [PATCH 1/6] fix(crypto): Fixed a bug where room keys would be rotated unecessarily. Previously, `is_session_overshared_for_user` did not take into account that `shared_with_set` also contains withheld device IDs who explicitly have never received the session keys. This would lead to it mistakenly determining oversharing for those devices for every event being sent in the presence of blacklisted/withheld devices in the room, and rotating the group session accordingly. The fix is to correctly exclude devices with ShareInfo::Withheld from the enumeration. --- .../src/session_manager/group_sessions/share_strategy.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index 1681c8dcca2..ca49a6256dc 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -27,6 +27,7 @@ use tracing::{debug, instrument, trace}; use super::OutboundGroupSession; use crate::{ error::{OlmResult, SessionRecipientCollectionError}, + olm::ShareInfo, store::Store, DeviceData, EncryptionSettings, LocalTrust, OlmError, OwnUserIdentityData, UserIdentityData, }; @@ -433,7 +434,11 @@ fn is_session_overshared_for_user( }; // Devices that received this session - let shared: BTreeSet<&DeviceId> = shared.keys().map(|d| d.as_ref()).collect(); + let shared: BTreeSet<&DeviceId> = shared + .iter() + .filter(|(_, info)| matches!(info, ShareInfo::Shared(_))) + .map(|(d, _)| d.as_ref()) + .collect(); // The set difference between // From a117f833f0d600f694aebb1f7300b145dd806bd3 Mon Sep 17 00:00:00 2001 From: Niklas Baumstark Date: Wed, 23 Apr 2025 18:53:04 +0200 Subject: [PATCH 2/6] add CHANGELOG entry --- crates/matrix-sdk-crypto/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index 40857f7f04b..022059a5ae7 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -16,6 +16,10 @@ All notable changes to this project will be documented in this file. (as per [MSC4147](https://github.com/matrix-org/matrix-spec-proposals/pull/4147)). ([#4922](https://github.com/matrix-org/matrix-rust-sdk/pull/4922)) +- Fix bug which caused room keys to be unnecessarily rotated on every send in the + presence of blacklisted/withheld devices in the room. + ([#4954](https://github.com/matrix-org/matrix-rust-sdk/pull/4954)) + ## [0.11.0] - 2025-04-11 ### Features From 33bdfcafb6f313b72da7c6fcdab38aa8bbb8e48f Mon Sep 17 00:00:00 2001 From: Niklas Baumstark Date: Wed, 23 Apr 2025 20:22:18 +0200 Subject: [PATCH 3/6] fix existing test and add a regression test --- .../group_sessions/share_strategy.rs | 50 ++++++++++++++++--- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index ca49a6256dc..c3702935afa 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -2100,9 +2100,49 @@ mod tests { let machine = test_machine().await; import_known_users_to_test_machine(&machine).await; - let fake_room_id = room_id!("!roomid:localhost"); let encryption_settings = all_devices_strategy_settings(); + let group_session = create_test_outbound_group_session(&machine, &encryption_settings); + let sender_key = machine.identity_keys().curve25519; + + group_session.mark_shared_with( + KeyDistributionTestData::dan_id(), + KeyDistributionTestData::dan_signed_device_id(), + sender_key).await; + group_session.mark_shared_with( + KeyDistributionTestData::dan_id(), + KeyDistributionTestData::dan_unsigned_device_id(), + sender_key).await; + + // Try to share again after dan has removed one of his devices + let keys_query = KeyDistributionTestData::dan_keys_query_response_device_loggedout(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + // share again + let share_result = collect_session_recipients( + machine.store(), + vec![KeyDistributionTestData::dan_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await + .unwrap(); + assert!(share_result.should_rotate); + } + + /// Test that the session is not rotated if a devices is removed + /// but was already withheld from receiving the session. + #[async_test] + async fn test_should_not_rotate_if_keys_were_withheld() { + let machine = test_machine().await; + import_known_users_to_test_machine(&machine).await; + + let encryption_settings = all_devices_strategy_settings(); + let group_session = create_test_outbound_group_session(&machine, &encryption_settings); + let fake_room_id = group_session.room_id(); + + // Because we don't have Olm sessions initialized, this will contain + // withheld requests for both of Dan's devices let requests = machine .share_room_key( fake_room_id, @@ -2120,13 +2160,11 @@ mod tests { .await .unwrap(); } + // Try to share again after dan has removed one of his devices let keys_query = KeyDistributionTestData::dan_keys_query_response_device_loggedout(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); - let group_session = - machine.store().get_outbound_group_session(fake_room_id).await.unwrap().unwrap(); // share again let share_result = collect_session_recipients( machine.store(), @@ -2137,7 +2175,7 @@ mod tests { .await .unwrap(); - assert!(share_result.should_rotate); + assert!(!share_result.should_rotate); } /// Common setup for tests which require a verified user to have unsigned From c36d512dc418ff45e9c85e46b2352ca3c8d49dd5 Mon Sep 17 00:00:00 2001 From: Niklas Baumstark Date: Wed, 23 Apr 2025 20:22:48 +0200 Subject: [PATCH 4/6] be conservative and take also potentially pending to-device requests into account --- .../src/olm/group_sessions/outbound.rs | 2 +- .../group_sessions/share_strategy.rs | 45 +++++++++++++++---- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs index 21e4eeb8eb0..7d18189a984 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -160,7 +160,7 @@ pub struct OutboundGroupSession { pub(crate) shared_with_set: Arc>>>, #[allow(clippy::type_complexity)] - to_share_with_set: + pub(crate) to_share_with_set: Arc, ShareInfoSet)>>>, } diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index c3702935afa..f11089cffaf 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -427,22 +427,49 @@ fn is_session_overshared_for_user( let recipient_device_ids: BTreeSet<&DeviceId> = recipient_devices.iter().map(|d| d.device_id()).collect(); + let mut shared: Vec<&DeviceId> = Vec::new(); + + // This duplicates a conservative subset of the logic in OutbundGroupSession::is_shared_with, + // because we don't have corresponding DeviceData at hand + fn is_actually_shared(info: &ShareInfo) -> bool { + match info { + ShareInfo::Shared(_) => true, + ShareInfo::Withheld(_) => false, + } + } + + // Collect the devices that have definitely received the session already let guard = outbound_session.shared_with_set.read(); + if let Some(for_user) = guard.get(user_id) { + shared.extend(for_user.iter().filter_map(|(d, info)| if is_actually_shared(info) { + Some(AsRef::::as_ref(d)) + } else { + None + })); + } - let Some(shared) = guard.get(user_id) else { + // To be conservative, also collect the devices that would still receive the session + // from a pending to-device request if we don't rotate beforehand + let guard = outbound_session.to_share_with_set.read(); + for (_txid, share_infos) in guard.values() { + if let Some(for_user) = share_infos.get(user_id) { + shared.extend(for_user.iter().filter_map(|(d, info)| if is_actually_shared(info) { + Some(AsRef::::as_ref(d)) + } else { + None + })); + } + } + + if shared.is_empty() { return false; - }; + } - // Devices that received this session - let shared: BTreeSet<&DeviceId> = shared - .iter() - .filter(|(_, info)| matches!(info, ShareInfo::Shared(_))) - .map(|(d, _)| d.as_ref()) - .collect(); + let shared: BTreeSet<&DeviceId> = shared.into_iter().collect(); // The set difference between // - // 1. Devices that had previously received the session, and + // 1. Devices that had previously received (or are queued to receive) the session, and // 2. Devices that would now receive the session // // Represents newly deleted or blacklisted devices. If this From 4ff3b9d6c7b2ea1f6950dbe1e7103c4deb9a8885 Mon Sep 17 00:00:00 2001 From: Niklas Baumstark Date: Wed, 23 Apr 2025 20:31:41 +0200 Subject: [PATCH 5/6] lint --- .../group_sessions/share_strategy.rs | 54 +++++++++++-------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index f11089cffaf..127003f60e5 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -429,8 +429,9 @@ fn is_session_overshared_for_user( let mut shared: Vec<&DeviceId> = Vec::new(); - // This duplicates a conservative subset of the logic in OutbundGroupSession::is_shared_with, - // because we don't have corresponding DeviceData at hand + // This duplicates a conservative subset of the logic in + // `OutboundGroupSession::is_shared_with`, because we + // don't have corresponding DeviceData at hand fn is_actually_shared(info: &ShareInfo) -> bool { match info { ShareInfo::Shared(_) => true, @@ -441,22 +442,26 @@ fn is_session_overshared_for_user( // Collect the devices that have definitely received the session already let guard = outbound_session.shared_with_set.read(); if let Some(for_user) = guard.get(user_id) { - shared.extend(for_user.iter().filter_map(|(d, info)| if is_actually_shared(info) { - Some(AsRef::::as_ref(d)) - } else { - None + shared.extend(for_user.iter().filter_map(|(d, info)| { + if is_actually_shared(info) { + Some(AsRef::::as_ref(d)) + } else { + None + } })); } - // To be conservative, also collect the devices that would still receive the session - // from a pending to-device request if we don't rotate beforehand + // To be conservative, also collect the devices that would still receive the + // session from a pending to-device request if we don't rotate beforehand let guard = outbound_session.to_share_with_set.read(); for (_txid, share_infos) in guard.values() { if let Some(for_user) = share_infos.get(user_id) { - shared.extend(for_user.iter().filter_map(|(d, info)| if is_actually_shared(info) { - Some(AsRef::::as_ref(d)) - } else { - None + shared.extend(for_user.iter().filter_map(|(d, info)| { + if is_actually_shared(info) { + Some(AsRef::::as_ref(d)) + } else { + None + } })); } } @@ -469,7 +474,8 @@ fn is_session_overshared_for_user( // The set difference between // - // 1. Devices that had previously received (or are queued to receive) the session, and + // 1. Devices that had previously received (or are queued to receive) the + // session, and // 2. Devices that would now receive the session // // Represents newly deleted or blacklisted devices. If this @@ -2131,14 +2137,20 @@ mod tests { let group_session = create_test_outbound_group_session(&machine, &encryption_settings); let sender_key = machine.identity_keys().curve25519; - group_session.mark_shared_with( - KeyDistributionTestData::dan_id(), - KeyDistributionTestData::dan_signed_device_id(), - sender_key).await; - group_session.mark_shared_with( - KeyDistributionTestData::dan_id(), - KeyDistributionTestData::dan_unsigned_device_id(), - sender_key).await; + group_session + .mark_shared_with( + KeyDistributionTestData::dan_id(), + KeyDistributionTestData::dan_signed_device_id(), + sender_key, + ) + .await; + group_session + .mark_shared_with( + KeyDistributionTestData::dan_id(), + KeyDistributionTestData::dan_unsigned_device_id(), + sender_key, + ) + .await; // Try to share again after dan has removed one of his devices let keys_query = KeyDistributionTestData::dan_keys_query_response_device_loggedout(); From 25636053cc4e8d90d387b5260138ee98941b204a Mon Sep 17 00:00:00 2001 From: Niklas Baumstark Date: Thu, 24 Apr 2025 13:40:52 +0200 Subject: [PATCH 6/6] Revert "be conservative and take also potentially pending to-device requests into account" This reverts commit c36d512dc418ff45e9c85e46b2352ca3c8d49dd5. --- .../src/olm/group_sessions/outbound.rs | 2 +- .../group_sessions/share_strategy.rs | 51 ++++--------------- 2 files changed, 10 insertions(+), 43 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs index 7d18189a984..21e4eeb8eb0 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -160,7 +160,7 @@ pub struct OutboundGroupSession { pub(crate) shared_with_set: Arc>>>, #[allow(clippy::type_complexity)] - pub(crate) to_share_with_set: + to_share_with_set: Arc, ShareInfoSet)>>>, } diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index 127003f60e5..6eb6a8726d0 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -427,55 +427,22 @@ fn is_session_overshared_for_user( let recipient_device_ids: BTreeSet<&DeviceId> = recipient_devices.iter().map(|d| d.device_id()).collect(); - let mut shared: Vec<&DeviceId> = Vec::new(); - - // This duplicates a conservative subset of the logic in - // `OutboundGroupSession::is_shared_with`, because we - // don't have corresponding DeviceData at hand - fn is_actually_shared(info: &ShareInfo) -> bool { - match info { - ShareInfo::Shared(_) => true, - ShareInfo::Withheld(_) => false, - } - } - - // Collect the devices that have definitely received the session already let guard = outbound_session.shared_with_set.read(); - if let Some(for_user) = guard.get(user_id) { - shared.extend(for_user.iter().filter_map(|(d, info)| { - if is_actually_shared(info) { - Some(AsRef::::as_ref(d)) - } else { - None - } - })); - } - - // To be conservative, also collect the devices that would still receive the - // session from a pending to-device request if we don't rotate beforehand - let guard = outbound_session.to_share_with_set.read(); - for (_txid, share_infos) in guard.values() { - if let Some(for_user) = share_infos.get(user_id) { - shared.extend(for_user.iter().filter_map(|(d, info)| { - if is_actually_shared(info) { - Some(AsRef::::as_ref(d)) - } else { - None - } - })); - } - } - if shared.is_empty() { + let Some(shared) = guard.get(user_id) else { return false; - } + }; - let shared: BTreeSet<&DeviceId> = shared.into_iter().collect(); + // Devices that received this session + let shared: BTreeSet<&DeviceId> = shared + .iter() + .filter(|(_, info)| matches!(info, ShareInfo::Shared(_))) + .map(|(d, _)| d.as_ref()) + .collect(); // The set difference between // - // 1. Devices that had previously received (or are queued to receive) the - // session, and + // 1. Devices that had previously received the session, and // 2. Devices that would now receive the session // // Represents newly deleted or blacklisted devices. If this