Skip to content

Refuse to decrypt to-device messages from unverified devices (when in exclude insecure mode) #5319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
9 changes: 7 additions & 2 deletions bindings/matrix-sdk-crypto-ffi/src/dehydrated_devices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use matrix_sdk_crypto::{
RehydratedDevice as InnerRehydratedDevice,
},
store::types::DehydratedDeviceKey as InnerDehydratedDeviceKey,
DecryptionSettings,
};
use ruma::{api::client::dehydrated_device, events::AnyToDeviceEvent, serde::Raw, OwnedDeviceId};
use serde_json::json;
Expand Down Expand Up @@ -154,9 +155,13 @@ impl Drop for RehydratedDevice {

#[matrix_sdk_ffi_macros::export]
impl RehydratedDevice {
pub fn receive_events(&self, events: String) -> Result<(), crate::CryptoStoreError> {
pub fn receive_events(
&self,
events: String,
decryption_settings: &DecryptionSettings,
) -> Result<(), crate::CryptoStoreError> {
let events: Vec<Raw<AnyToDeviceEvent>> = serde_json::from_str(&events)?;
self.runtime.block_on(self.inner.receive_events(events))?;
self.runtime.block_on(self.inner.receive_events(events, decryption_settings))?;

Ok(())
}
Expand Down
21 changes: 12 additions & 9 deletions bindings/matrix-sdk-crypto-ffi/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,7 @@ impl OlmMachine {
key_counts: HashMap<String, i32>,
unused_fallback_keys: Option<Vec<String>>,
next_batch_token: String,
decryption_settings: &DecryptionSettings,
) -> Result<SyncChangesResult, CryptoStoreError> {
let to_device: ToDevice = serde_json::from_str(&events)?;
let device_changes: RumaDeviceLists = device_changes.into();
Expand All @@ -544,15 +545,17 @@ impl OlmMachine {
let unused_fallback_keys: Option<Vec<OneTimeKeyAlgorithm>> =
unused_fallback_keys.map(|u| u.into_iter().map(OneTimeKeyAlgorithm::from).collect());

let (to_device_events, room_key_infos) = self.runtime.block_on(
self.inner.receive_sync_changes(matrix_sdk_crypto::EncryptionSyncChanges {
to_device_events: to_device.events,
changed_devices: &device_changes,
one_time_keys_counts: &key_counts,
unused_fallback_keys: unused_fallback_keys.as_deref(),
next_batch_token: Some(next_batch_token),
}),
)?;
let (to_device_events, room_key_infos) =
self.runtime.block_on(self.inner.receive_sync_changes(
matrix_sdk_crypto::EncryptionSyncChanges {
to_device_events: to_device.events,
changed_devices: &device_changes,
one_time_keys_counts: &key_counts,
unused_fallback_keys: unused_fallback_keys.as_deref(),
next_batch_token: Some(next_batch_token),
},
decryption_settings,
))?;

let to_device_events = to_device_events
.into_iter()
Expand Down
7 changes: 6 additions & 1 deletion crates/matrix-sdk-base/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,12 @@ impl BaseClient {
let processors::e2ee::to_device::Output {
processed_to_device_events: to_device,
room_key_updates,
} = processors::e2ee::to_device::from_sync_v2(&response, olm_machine.as_ref()).await?;
} = processors::e2ee::to_device::from_sync_v2(
&response,
olm_machine.as_ref(),
&self.decryption_settings,
)
.await?;

processors::latest_event::decrypt_from_rooms(
&mut context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
use std::collections::BTreeMap;

use matrix_sdk_common::deserialized_responses::ProcessedToDeviceEvent;
use matrix_sdk_crypto::{store::types::RoomKeyInfo, EncryptionSyncChanges, OlmMachine};
use matrix_sdk_crypto::{
store::types::RoomKeyInfo, DecryptionSettings, EncryptionSyncChanges, OlmMachine,
};
use ruma::{
api::client::sync::sync_events::{v3, v5, DeviceLists},
events::AnyToDeviceEvent,
Expand All @@ -34,6 +36,7 @@ pub async fn from_msc4186(
to_device: Option<&v5::response::ToDevice>,
e2ee: &v5::response::E2EE,
olm_machine: Option<&OlmMachine>,
decryption_settings: &DecryptionSettings,
) -> Result<Output> {
process(
olm_machine,
Expand All @@ -42,6 +45,7 @@ pub async fn from_msc4186(
&e2ee.device_one_time_keys_count,
e2ee.device_unused_fallback_key_types.as_deref(),
to_device.as_ref().map(|to_device| to_device.next_batch.clone()),
decryption_settings,
)
.await
}
Expand All @@ -54,6 +58,7 @@ pub async fn from_msc4186(
pub async fn from_sync_v2(
response: &v3::Response,
olm_machine: Option<&OlmMachine>,
decryption_settings: &DecryptionSettings,
) -> Result<Output> {
process(
olm_machine,
Expand All @@ -62,6 +67,7 @@ pub async fn from_sync_v2(
&response.device_one_time_keys_count,
response.device_unused_fallback_key_types.as_deref(),
Some(response.next_batch.clone()),
decryption_settings,
)
.await
}
Expand All @@ -77,6 +83,7 @@ async fn process(
one_time_keys_counts: &BTreeMap<OneTimeKeyAlgorithm, UInt>,
unused_fallback_keys: Option<&[OneTimeKeyAlgorithm]>,
next_batch_token: Option<String>,
decryption_settings: &DecryptionSettings,
) -> Result<Output> {
let encryption_sync_changes = EncryptionSyncChanges {
to_device_events,
Expand All @@ -92,7 +99,7 @@ async fn process(
// This makes sure that we have the decryption keys for the room
// events at hand.
let (events, room_key_updates) =
olm_machine.receive_sync_changes(encryption_sync_changes).await?;
olm_machine.receive_sync_changes(encryption_sync_changes, decryption_settings).await?;

Output { processed_to_device_events: events, room_key_updates: Some(room_key_updates) }
} else {
Expand Down
9 changes: 7 additions & 2 deletions crates/matrix-sdk-base/src/sliding_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,13 @@ impl BaseClient {
let mut context = processors::Context::default();

let processors::e2ee::to_device::Output { processed_to_device_events, room_key_updates } =
processors::e2ee::to_device::from_msc4186(to_device, e2ee, olm_machine.as_ref())
.await?;
processors::e2ee::to_device::from_msc4186(
to_device,
e2ee,
olm_machine.as_ref(),
&self.decryption_settings,
)
.await?;

processors::latest_event::decrypt_from_rooms(
&mut context,
Expand Down
11 changes: 11 additions & 0 deletions crates/matrix-sdk-common/src/deserialized_responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,15 @@ pub enum ProcessedToDeviceEvent {
/// required information to be processed (like no event `type` for
/// example)
Invalid(Raw<AnyToDeviceEvent>),

/// A to-device event that was ignored because the sender device was not
/// verified.
UnverifiedSender {
/// The raw decrypted event
raw: Raw<AnyToDeviceEvent>,
/// The Olm encryption info
encryption_info: EncryptionInfo,
},
Comment on lines +1202 to +1207
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but is this something we want? In the case of Megolm we just give you the UTD.

I would have expected us to do the same here, though I'm happy to be convinced that we need to do something different for Olm/to-device messages.

If we want to explain to people why this is an UTD we could expand the UnableToDecrypt variant.

If you agree with this, perhaps we should move all this logic into the Account somewhere in the parse_decrypted_to_device_event(). Not sure if we should also move the device dehydration check there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just felt rude to decrypt a message and then throw away all we know about it, so it would be hard to debug why it failed to decrypt, so I wanted to include the decrypted event (mainly so you could see the real event type) and encryption info.

If you like we could add to UTD like:

struct DecryptedCandidate {
    event: Raw<AnyToDeviceEvent>,
    encryption_info: EncryptionInfo,
}

...
pub enum ProcessedToDeviceEvent {
    UnableToDecrypt{
        encrypted_event: Raw<AnyToDeviceEvent>,
        decrypted_candidate: Option<SomeNewType>
    }
...

But I'd also be happy to just return the existing UTD if you think that's sensible.

If you agree with this, perhaps we should move all this logic into the Account somewhere in the parse_decrypted_to_device_event(). Not sure if we should also move the device dehydration check there.

Happy to do that if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just felt rude to decrypt a message

Yeah but that's what we do for room messages as well

and then throw away all we know about it, so it would be hard to debug why it failed to decrypt

We should probably return the error then. We can include more metadata i.e. the decrypted event type if you think that this is sensible.

If you like we could add to UTD like:

Yes, this seems sensible. It's also mimicking what we do for room events more or less. That is we can include the UTD reason/info like we did for room events:

/// An encrypted event which could not be decrypted.
UnableToDecrypt {
/// The `m.room.encrypted` event. Depending on the source of the event,
/// it could actually be an [`AnyTimelineEvent`] (i.e., it may
/// have a `room_id` property).
event: Raw<AnySyncTimelineEvent>,
/// Information on the reason we failed to decrypt
utd_info: UnableToDecryptInfo,
},

Happy to do that if you prefer.

Yeah, sounds like it would centralize this "refusing to decrypt" logic.

}

impl ProcessedToDeviceEvent {
Expand All @@ -1207,6 +1216,7 @@ impl ProcessedToDeviceEvent {
ProcessedToDeviceEvent::UnableToDecrypt(event) => event.clone(),
ProcessedToDeviceEvent::PlainText(event) => event.clone(),
ProcessedToDeviceEvent::Invalid(event) => event.clone(),
ProcessedToDeviceEvent::UnverifiedSender { raw, .. } => raw.clone(),
}
}

Expand All @@ -1217,6 +1227,7 @@ impl ProcessedToDeviceEvent {
ProcessedToDeviceEvent::UnableToDecrypt(event) => event,
ProcessedToDeviceEvent::PlainText(event) => event,
ProcessedToDeviceEvent::Invalid(event) => event,
ProcessedToDeviceEvent::UnverifiedSender { raw, .. } => raw,
}
}
}
Expand Down
15 changes: 15 additions & 0 deletions crates/matrix-sdk-crypto/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,21 @@ All notable changes to this project will be documented in this file.
- [**breaking**] Add a new `VerificationLevel::MismatchedSender` to indicate that the sender of an event appears to have been tampered with.
([#5219](https://github.com/matrix-org/matrix-rust-sdk/pull/5219))

- [**breaking**]: When in "exclude insecure devices" mode, refuse to decrypt
incoming to-device messages from unverified devices, except for some
exceptions for certain event types (see the documentation on
`OlmMachine::decrypt_to_device_event` for details). To support this, a new
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OlmMachine::decrypt_to_device_event is a private method, so it's gonna be a bit hard for users to look at the docs of that method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Do you think it's worth repeating ourselves by listing the exceptions in a public method doc? I looked and it felt a bit over-detailed. Maybe I should just remove this part from the release notes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing it from the changelog is fine.

variant has been added to `ProcessedToDeviceEvent`: `UnverifiedSender`, which
is returned from `OlmMachine::receive_encrypted_to_device_event` when we are
excluding insecure devices and the sender's device is not verified. Also,
several methods now take a `DecryptionSettings` argument to allow controlling
the processing of to-device events based on those settings. To recreate the
previous behaviour pass in: `DecryptionSettings {
sender_device_trust_requirement: TrustRequirement::Untrusted }`. Affected
methods are `OlmMachine::receive_sync_changes`,
`RehydratedDevice::receive_events`, and several internal methods.
([#5319](https://github.com/matrix-org/matrix-rust-sdk/pull/5319)

### Refactor

- [**breaking**] The `PendingChanges`, `Changes`, `StoredRoomKeyBundleData`,
Expand Down
31 changes: 19 additions & 12 deletions crates/matrix-sdk-crypto/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ The state machine works in a push/pull manner:
```rust,no_run
use std::collections::BTreeMap;

use matrix_sdk_crypto::{EncryptionSyncChanges, OlmMachine, OlmError};
use ruma::{
api::client::sync::sync_events::{v3::ToDevice, DeviceLists},
device_id, user_id,
use matrix_sdk_crypto::{
DecryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine, TrustRequirement,
};
use ruma::{api::client::sync::sync_events::DeviceLists, device_id, user_id};

#[tokio::main]
async fn main() -> Result<(), OlmError> {
Expand All @@ -33,19 +32,27 @@ async fn main() -> Result<(), OlmError> {
let unused_fallback_keys = Some(Vec::new());
let next_batch_token = "T0K3N".to_owned();

let decryption_settings =
DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted };

// Push changes that the server sent to us in a sync response.
let decrypted_to_device = machine.receive_sync_changes(EncryptionSyncChanges {
to_device_events: vec![],
changed_devices: &changed_devices,
one_time_keys_counts: &one_time_key_counts,
unused_fallback_keys: unused_fallback_keys.as_deref(),
next_batch_token: Some(next_batch_token),
}).await?;
let decrypted_to_device = machine
.receive_sync_changes(
EncryptionSyncChanges {
to_device_events: vec![],
changed_devices: &changed_devices,
one_time_keys_counts: &one_time_key_counts,
unused_fallback_keys: unused_fallback_keys.as_deref(),
next_batch_token: Some(next_batch_token),
},
&decryption_settings,
)
.await?;

// Pull requests that we need to send out.
let outgoing_requests = machine.outgoing_requests().await?;

// Send the requests here out and call machine.mark_request_as_sent().
// Send the requests out here and call machine.mark_request_as_sent().

Ok(())
}
Expand Down
27 changes: 20 additions & 7 deletions crates/matrix-sdk-crypto/src/dehydrated_devices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ use crate::{
CryptoStoreWrapper, MemoryStore, Store,
},
verification::VerificationMachine,
Account, CryptoStoreError, EncryptionSyncChanges, OlmError, OlmMachine, SignatureError,
Account, CryptoStoreError, DecryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine,
SignatureError,
};

/// Error type for device dehydration issues.
Expand Down Expand Up @@ -215,7 +216,9 @@ impl RehydratedDevice {
///
/// ```no_run
/// # use anyhow::Result;
/// # use matrix_sdk_crypto::{ OlmMachine, store::types::DehydratedDeviceKey };
/// # use matrix_sdk_crypto::{
/// DecryptionSettings, OlmMachine, TrustRequirement, store::types::DehydratedDeviceKey
/// };
/// # use ruma::{api::client::dehydrated_device, DeviceId};
/// # async fn example() -> Result<()> {
/// # let machine: OlmMachine = unimplemented!();
Expand Down Expand Up @@ -245,6 +248,9 @@ impl RehydratedDevice {
///
/// let mut since_token = None;
/// let mut imported_room_keys = 0;
/// let decryption_settings = DecryptionSettings {
/// sender_device_trust_requirement: TrustRequirement::Untrusted
/// };
///
/// loop {
/// let response =
Expand All @@ -255,7 +261,7 @@ impl RehydratedDevice {
/// }
///
/// since_token = response.next_batch.as_deref();
/// imported_room_keys += rehydrated.receive_events(response.events).await?.len();
/// imported_room_keys += rehydrated.receive_events(response.events, &decryption_settings).await?.len();
/// }
///
/// println!("Successfully imported {imported_room_keys} from the dehydrated device.");
Expand All @@ -273,6 +279,7 @@ impl RehydratedDevice {
pub async fn receive_events(
&self,
events: Vec<Raw<AnyToDeviceEvent>>,
decryption_settings: &DecryptionSettings,
) -> Result<Vec<RoomKeyInfo>, OlmError> {
trace!("Receiving events for a rehydrated Device");

Expand All @@ -290,7 +297,7 @@ impl RehydratedDevice {

let (_, changes) = self
.rehydrated
.preprocess_sync_changes(&mut rehydrated_transaction, sync_changes)
.preprocess_sync_changes(&mut rehydrated_transaction, sync_changes, decryption_settings)
.await?;

// Now take the room keys and persist them in our original `OlmMachine`.
Expand Down Expand Up @@ -423,7 +430,7 @@ mod tests {
store::types::DehydratedDeviceKey,
types::{events::ToDeviceEvent, DeviceKeys as DeviceKeysType},
utilities::json_convert,
EncryptionSettings, OlmMachine,
DecryptionSettings, EncryptionSettings, OlmMachine, TrustRequirement,
};

fn pickle_key() -> DehydratedDeviceKey {
Expand Down Expand Up @@ -568,9 +575,12 @@ mod tests {
assert_eq!(rehydrated.rehydrated.device_id(), request.device_id);
assert_eq!(rehydrated.original.device_id(), alice.device_id());

let decryption_settings =
DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted };

// Push the to-device event containing the room key into the rehydrated device.
let ret = rehydrated
.receive_events(vec![event])
.receive_events(vec![event], &decryption_settings)
.await
.expect("We should be able to push to-device events into the rehydrated device");

Expand Down Expand Up @@ -680,9 +690,12 @@ mod tests {
assert_eq!(rehydrated.rehydrated.device_id(), &device_id);
assert_eq!(rehydrated.original.device_id(), alice.device_id());

let decryption_settings =
DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted };

// Push the to-device event containing the room key into the rehydrated device.
let ret = rehydrated
.receive_events(vec![event])
.receive_events(vec![event], &decryption_settings)
.await
.expect("We should be able to push to-device events into the rehydrated device");

Expand Down
21 changes: 14 additions & 7 deletions crates/matrix-sdk-crypto/src/gossiping/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,7 @@ mod tests {
EncryptedEvent, EncryptedToDeviceEvent, RoomEncryptedEventContent,
},
verification::VerificationMachine,
DecryptionSettings, TrustRequirement,
};

fn alice_id() -> &'static UserId {
Expand Down Expand Up @@ -2051,14 +2052,20 @@ mod tests {
let stream = bob_machine.store().secrets_stream();
pin_mut!(stream);

let decryption_settings =
DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted };

bob_machine
.receive_sync_changes(EncryptionSyncChanges {
to_device_events: vec![event],
changed_devices: &Default::default(),
one_time_keys_counts: &Default::default(),
unused_fallback_keys: None,
next_batch_token: None,
})
.receive_sync_changes(
EncryptionSyncChanges {
to_device_events: vec![event],
changed_devices: &Default::default(),
one_time_keys_counts: &Default::default(),
unused_fallback_keys: None,
next_batch_token: None,
},
&decryption_settings,
)
.await
.unwrap();

Expand Down
Loading
Loading