-
Notifications
You must be signed in to change notification settings - Fork 269
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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?; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
Ok(decrypted) | ||||||||||||||
} | ||||||||||||||
|
@@ -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() { | ||||||||||||||
|
@@ -1274,7 +1286,7 @@ impl OlmMachine { | |||||||||||||
// Skip invalid events. | ||||||||||||||
warn!("Received an invalid to-device event: {e}"); | ||||||||||||||
|
||||||||||||||
return raw_event; | ||||||||||||||
return Some(raw_event); | ||||||||||||||
} | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
|
@@ -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" | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
); | ||||||||||||||
return None; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// New sessions modify the account so we need to save that | ||||||||||||||
// one as well. | ||||||||||||||
match decrypted.session { | ||||||||||||||
|
@@ -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( | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
&self, | ||||||||||||||
decrypted: &OlmDecryptionInfo, | ||||||||||||||
sender_user_id: &UserId, | ||||||||||||||
) -> bool { | ||||||||||||||
Comment on lines
+1365
to
+1369
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||
// 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? | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. light suggestion, no strong feelings here
Suggested change
|
||||||||||||||
} | ||||||||||||||
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. | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we just don't recognise the device, It's probably still ok to return |
||||||||||||||
false | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Handle a to-device and one-time key counts from a sync response. | ||||||||||||||
|
@@ -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, | ||||||||||||||
|
@@ -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 | ||||||||||||||
|
There was a problem hiding this comment.
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 existingSecretRequest
for anything to happen, and that looked complex to set up.There was a problem hiding this comment.
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.)