-
Notifications
You must be signed in to change notification settings - Fork 314
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
base: main
Are you sure you want to change the base?
Changes from all commits
97ca9ee
94b28a1
c3c6025
e638677
92d064e
b899348
4c1ef53
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 |
---|---|---|
|
@@ -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 | ||
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.
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. 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. 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 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`, | ||
|
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.
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 theparse_decrypted_to_device_event()
. Not sure if we should also move the device dehydration check there.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.
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:
But I'd also be happy to just return the existing UTD if you think that's sensible.
Happy to do that if you prefer.
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.
Yeah but that's what we do for room messages as well
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.
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:
matrix-rust-sdk/crates/matrix-sdk-common/src/deserialized_responses.rs
Lines 720 to 729 in 6de4032
Yeah, sounds like it would centralize this "refusing to decrypt" logic.