Skip to content
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

Ignore to-device messages from dehydrated devices #4569

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 89 additions & 8 deletions crates/matrix-sdk-crypto/src/machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -852,9 +852,14 @@ impl OlmMachine {
let mut decrypted =
transaction.account().await?.decrypt_to_device_event(&self.inner.store, event).await?;

// Handle the decrypted event, e.g. fetch out Megolm sessions out of
// the event.
self.handle_decrypted_to_device_event(transaction.cache(), &mut decrypted, changes).await?;
// We ignore all to-device events from dehydrated devices - we should not
// receive any
if !self.event_is_from_dehydrated_device(&decrypted, &event.sender).await {
// Handle the decrypted event, e.g. fetch out Megolm sessions out of
// the event.
self.handle_decrypted_to_device_event(transaction.cache(), &mut decrypted, changes)
.await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

I could not find a way to test that the side effects of this call are suppressed when sending from a dehydrated device. I think the only observable side effects happen when we send a SecretSend to-device event, and when I looked at how to do that it looked like I would need an existing SecretRequest for anything to happen, and that looked complex to set up.

Copy link
Member Author

Choose a reason for hiding this comment

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

(To try to clarify what I mean: my tests would still pass if this if did not exist.)

}

Ok(decrypted)
}
Expand Down Expand Up @@ -1259,13 +1264,20 @@ impl OlmMachine {
}
}

/// Decrypt the supplied to-device event (if needed, and if we can) and
/// handle it.
///
/// Return the same event, decrypted if possible and needed.
///
/// If we can identify that this to-device event came from a dehydrated
/// device, this method does not process it, and returns `None`.
#[instrument(skip_all, fields(sender, event_type, message_id))]
async fn receive_to_device_event(
&self,
transaction: &mut StoreTransaction,
changes: &mut Changes,
mut raw_event: Raw<AnyToDeviceEvent>,
) -> Raw<AnyToDeviceEvent> {
) -> Option<Raw<AnyToDeviceEvent>> {
Self::record_message_id(&raw_event);

let event: ToDeviceEvents = match raw_event.deserialize_as() {
Expand All @@ -1274,7 +1286,7 @@ impl OlmMachine {
// Skip invalid events.
warn!("Received an invalid to-device event: {e}");

return raw_event;
return Some(raw_event);
}
};

Expand All @@ -1299,10 +1311,21 @@ impl OlmMachine {
}
}

return raw_event;
return Some(raw_event);
}
};

// We ignore all to-device events from dehydrated devices - we should not
// receive any
if self.event_is_from_dehydrated_device(&decrypted, &e.sender).await {
warn!(
sender = ?e.sender,
session = ?decrypted.session,
"Received a to-device event from a dehydrated device. This is unexpected"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Received a to-device event from a dehydrated device. This is unexpected"
"Received a to-device event from a dehydrated device. This is unexpected: ignoring event"

);
return None;
}

// New sessions modify the account so we need to save that
// one as well.
match decrypted.session {
Expand Down Expand Up @@ -1336,7 +1359,54 @@ impl OlmMachine {
e => self.handle_to_device_event(changes, &e).await,
}

raw_event
Some(raw_event)
}

async fn event_is_from_dehydrated_device(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async fn event_is_from_dehydrated_device(
async fn to_device_event_is_from_dehydrated_device(

&self,
decrypted: &OlmDecryptionInfo,
sender_user_id: &UserId,
) -> bool {
Comment on lines +1365 to +1369
Copy link
Member

Choose a reason for hiding this comment

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

it might come as a bit of a surprise that this accepts an OlmDecryptionInfo rather than, say, an AnyToDeviceEvent. Might be worth adding a doc-comment that explains that it works on decrypted to-device events.

// Does the to-device message include device info?
if let Some(device_keys) = decrypted.result.event.sender_device_keys() {
// There is no need to check whether the device keys are signed correctly - any
// to-device message that claims to be from a dehydrated device is weird, so we
// will drop it.

// Does the included device info say the device is dehydrated?
if device_keys.dehydrated.unwrap_or(false) {
return true;
}
// If not, fall through and check our existing list of devices
// below, just in case the sender is sending us
// incorrect information embedded in the to-device message, but we
// know better.
}

// Do we already know about this device?
Copy link
Member Author

Choose a reason for hiding this comment

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

My tests would still pass if the rest of this function did not exist.

I looked into how to construct a to-device message that was from a known dehydrated device, but did not itself contain device_keys and it just looked too complex. I even got part-way in to an implementation pairing with Rich, but it just seemed so much!

match self
.store()
.get_device_from_curve_key(sender_user_id, decrypted.result.sender_key)
.await
{
Ok(device) => {
// Yes - is it dehydrated?
if let Some(device) = device {
device.is_dehydrated()
} else {
false
}
Comment on lines +1394 to +1398
Copy link
Member

Choose a reason for hiding this comment

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

light suggestion, no strong feelings here

Suggested change
if let Some(device) = device {
device.is_dehydrated()
} else {
false
}
device.is_some_and(Device::is_dehydrated)

}
Err(e) => {
// No - we received a to-device message from a device we don't know about. We
// continue to process it, but this is unexpected so we log an error.
Copy link
Member

Choose a reason for hiding this comment

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

we log an error

We are logging a warning, not an error.

warn!(
error = ?e,
"Received a to-device message from a device we don't recognise",
);
Comment on lines +1401 to +1406
Copy link
Member

Choose a reason for hiding this comment

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

I think if we just don't recognise the device, get_device_from_curve_key will return Ok(None). This suggests a problem with the store.

It's probably still ok to return false here; but the comment and warning could do with a tweak.

false
}
}
}

/// Handle a to-device and one-time key counts from a sync response.
Expand Down Expand Up @@ -1377,6 +1447,14 @@ impl OlmMachine {
Ok((events, room_key_updates))
}

/// Initial processing of the changes specified within a sync response.
///
/// Returns the to-device events (decrypted where needed and where possible)
/// and the processed set of changes.
///
/// If any of the to-device events in the supplied changes were sent from
/// dehydrated devices, these are not processed, and are omitted from
/// the returned list, as per MSC3814.
pub(crate) async fn preprocess_sync_changes(
&self,
transaction: &mut StoreTransaction,
Expand Down Expand Up @@ -1412,7 +1490,10 @@ impl OlmMachine {
for raw_event in sync_changes.to_device_events {
let raw_event =
Box::pin(self.receive_to_device_event(transaction, &mut changes, raw_event)).await;
events.push(raw_event);

if let Some(raw_event) = raw_event {
events.push(raw_event);
}
}

let changed_sessions = self
Expand Down
61 changes: 56 additions & 5 deletions crates/matrix-sdk-crypto/src/machine/test_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use ruma::{
use serde_json::json;

use crate::{
store::Changes,
store::{Changes, MemoryStore},
types::{events::ToDeviceEvent, requests::AnyOutgoingRequest},
CrossSigningBootstrapRequests, DeviceData, OlmMachine,
};
Expand Down Expand Up @@ -102,6 +102,23 @@ pub async fn get_machine_after_query_test_helper() -> (OlmMachine, OneTimeKeys)
(machine, otk)
}

pub async fn get_machine_pair_using_store(
alice: &UserId,
bob: &UserId,
use_fallback_key: bool,
alice_store: MemoryStore,
alice_device_id: &DeviceId,
) -> (OlmMachine, OlmMachine, OneTimeKeys) {
let (bob, otk) = get_prepared_machine_test_helper(bob, use_fallback_key).await;

let alice = OlmMachine::with_store(alice, alice_device_id, alice_store, None)
.await
.expect("Failed to create OlmMachine from supplied store");

store_each_others_device_data(&alice, &bob).await;
(alice, bob, otk)
}

pub async fn get_machine_pair(
alice: &UserId,
bob: &UserId,
Expand All @@ -112,12 +129,34 @@ pub async fn get_machine_pair(
let alice_device = alice_device_id();
let alice = OlmMachine::new(alice, alice_device).await;

let alice_device = DeviceData::from_machine_test_helper(&alice).await.unwrap();
let bob_device = DeviceData::from_machine_test_helper(&bob).await.unwrap();
store_each_others_device_data(&alice, &bob).await;
(alice, bob, otk)
}

/// Store alice's device data in bob's store and vice versa
async fn store_each_others_device_data(alice: &OlmMachine, bob: &OlmMachine) {
let alice_device = DeviceData::from_machine_test_helper(alice).await.unwrap();
let bob_device = DeviceData::from_machine_test_helper(bob).await.unwrap();
alice.store().save_device_data(&[bob_device]).await.unwrap();
bob.store().save_device_data(&[alice_device]).await.unwrap();
}

(alice, bob, otk)
/// Return a pair of [`OlmMachine`]s, with an olm session created on Alice's
/// side, but with no message yet sent.
///
/// Create Alice's `OlmMachine` using the [`MemoryStore`] provided
pub async fn get_machine_pair_with_session_using_store(
alice: &UserId,
bob: &UserId,
use_fallback_key: bool,
alice_store: MemoryStore,
alice_device_id: &DeviceId,
) -> (OlmMachine, OlmMachine) {
let (alice, bob, one_time_keys) =
get_machine_pair_using_store(alice, bob, use_fallback_key, alice_store, alice_device_id)
.await;

build_session_for_pair(alice, bob, one_time_keys).await
}

/// Return a pair of [`OlmMachine`]s, with an olm session created on Alice's
Expand All @@ -127,8 +166,20 @@ pub async fn get_machine_pair_with_session(
bob: &UserId,
use_fallback_key: bool,
) -> (OlmMachine, OlmMachine) {
let (alice, bob, mut one_time_keys) = get_machine_pair(alice, bob, use_fallback_key).await;
let (alice, bob, one_time_keys) = get_machine_pair(alice, bob, use_fallback_key).await;

build_session_for_pair(alice, bob, one_time_keys).await
}

/// Create a session for the two supplied Olm machines to communicate.
async fn build_session_for_pair(
alice: OlmMachine,
bob: OlmMachine,
mut one_time_keys: BTreeMap<
ruma::OwnedKeyId<ruma::OneTimeKeyAlgorithm, ruma::OneTimeKeyName>,
Raw<OneTimeKey>,
>,
) -> (OlmMachine, OlmMachine) {
let (device_key_id, one_time_key) = one_time_keys.pop_first().unwrap();

let one_time_keys = BTreeMap::from([(
Expand Down
Loading
Loading