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

andybalaam · 2025-07-01T12:05:58Z

First half of #4147 - this is the receiving part.

codecov · 2025-07-01T12:17:51Z

Codecov Report

Attention: Patch coverage is 90.32258% with 18 lines in your changes missing coverage. Please review.

Project coverage is 88.86%. Comparing base (822b709) to head (a597752).
Report is 6 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...sdk-base/src/response_processors/e2ee/to_device.rs	41.66%	6 Missing and 1 partial ⚠️
crates/matrix-sdk-crypto/src/machine/mod.rs	89.74%	1 Missing and 3 partials ⚠️
crates/matrix-sdk-crypto/src/olm/account.rs	94.23%	3 Missing ⚠️
crates/matrix-sdk-base/src/client.rs	83.33%	0 Missing and 1 partial ⚠️
crates/matrix-sdk-base/src/sliding_sync.rs	85.71%	0 Missing and 1 partial ⚠️
...es/matrix-sdk-common/src/deserialized_responses.rs	66.66%	1 Missing ⚠️
...ates/matrix-sdk-crypto/src/machine/test_helpers.rs	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5319      +/-   ##
==========================================
+ Coverage   88.85%   88.86%   +0.01%     
==========================================
  Files         333      333              
  Lines       91587    91731     +144     
  Branches    91587    91731     +144     
==========================================
+ Hits        81379    81520     +141     
  Misses       6382     6382              
- Partials     3826     3829       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

poljar

Left some comments, the biggest discussion point is if we should return these events as UTDs or as this new separate variant.

crates/matrix-sdk-crypto/README.md

crates/matrix-sdk-crypto/src/machine/mod.rs

poljar · 2025-07-01T14:35:27Z

crates/matrix-sdk-crypto/src/machine/mod.rs

-            self.handle_decrypted_to_device_event(transaction.cache(), &mut decrypted, changes)
-                .await?;
+        // Return early if the sending device is decrypted
+        self.check_to_device_event_is_not_from_dehydrated_device(&decrypted, &event.sender).await?;


It's kinda tripping me up that we flattened out the branches here. A bit worried that we'll somehow miss this subtle ? that prevents us from accepting secrets from dehydrated devices.

That being said, I don't have an idea how to improve it.

Yeah, I don't love it, but I think splitting into a separate function is good.

I could make it an explicit if and return but as the method name says "check" and we have a comment saying "Return early" maybe it's good enough?

Attempted to make it clearer in e5fac47

crates/matrix-sdk-common/src/deserialized_responses.rs

crates/matrix-sdk-crypto/src/machine/mod.rs

poljar · 2025-07-01T14:45:24Z

crates/matrix-sdk-crypto/CHANGELOG.md

+- [**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


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.

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.

I think removing it from the changelog is fine.

Removed in 3924042

poljar · 2025-07-02T10:00:09Z

crates/matrix-sdk-common/src/deserialized_responses.rs

+    UnverifiedSender {
+        /// The raw decrypted event
+        raw: Raw<AnyToDeviceEvent>,
+        /// The Olm encryption info
+        encryption_info: EncryptionInfo,
+    },


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.

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.

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:

matrix-rust-sdk/crates/matrix-sdk-common/src/deserialized_responses.rs

Lines 720 to 729 in 6de4032

/// 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.

I was thinking about that in the context of element-call.
EC uses to-device messages to send the key to decrypt the video stream.
I initialy thought that it would be good for me to know that the incoming message was a call key even if we decided to not decrypt it because of trust settings. This could allow me to put some feedback on screen.

But actually I think I don't really need it. The call membership event has the sender device info in it, so I already know the verification state of the device, so I will know that it will be impossible to receive a key from it.

So the refusing to decrypt logic is ok for me.

If we really need some info maybe the UnableToDecryptInfo could hold the clear type? but not sure how useful it would be (appart for some metrics/analytics)

Yeah, I'm fine with adding the clear type to the info if necessary, just not the whole content.

Updated to make this a UTD in 56a51a4 . Working on moving the logic to Account.

Updated to move the logic to Account in e6748a0

Yeah, I'm fine with adding the clear type to the info if necessary, just not the whole content.

I have not done this since we don't see a clear need but it should be simple to do in future.

poljar · 2025-07-03T12:19:08Z

I see that you re-requested review, but what about my other comments? Most importantly about #5319 (comment)?

andybalaam · 2025-07-14T07:59:01Z

I see that you re-requested review, but what about my other comments? Most importantly about #5319 (comment)?

I'm waiting for replies from you on several comments including that one.

poljar · 2025-07-14T09:16:36Z

I see that you re-requested review, but what about my other comments? Most importantly about #5319 (comment)?

I'm waiting for replies from you on several comments including that one.

Err, did you reply to any of my comments? Or do you expect me to reply to my own comments?

I can't see any comments after my initial comments:

Is there supposed to be something else there? I'm confused.

andybalaam · 2025-07-14T09:25:22Z

I can't see any comments after my initial comments:

Odd. I can:

andybalaam · 2025-07-14T09:27:36Z

D'oh, it was a pending review, sorry. I think I've clicked the relevant button now.

poljar

Hopefully I replied to all the outstanding comments. If I missed some, please ping me.

BillCarsonFr · 2025-07-15T07:59:49Z

crates/matrix-sdk-crypto/src/machine/mod.rs

@@ -2816,6 +2939,28 @@ impl OlmMachine {
    }
 }

+fn sending_device_is_unverified(encryption_info: &EncryptionInfo) -> bool {
+    if let VerificationState::Unverified(level) = &encryption_info.verification_state {


nit: I think that a full match with Verified/Unverified will be easier to read than the if let and else

Also the name is_unverified can be better? because now when UnverifiedIdentity is true then is_unverified is false.

Can we do something similar than for megolm? something around check_sender_trust_requirement?

Thanks, improved with a match in 02ba63c and re-arranged to try and look more like check_sender_trust_requirement in 550fa11 . What do you think?

andybalaam · 2025-07-23T15:31:04Z

Hopefully I replied to all the outstanding comments. If I missed some, please ping me.

I think I addressed everything - let me know if not. Thanks!

poljar

Alright, I think that this is good now.

There's an incomplete commit message in 550fa11.

Not sure if you want to clean up the history and the commit message or if you'd like to just squash merge or something else.

…o_device_event This will be used in the next commit, but it was very noisy, so I separated it out into this commit to make the next one easier to read.

… dehydrated devices

…ypted_to_device_test_helper This will allow us to re-use it in more tests.

…per in 2 more tests

…ypted_to_device_test_helper To allow passing in different values in future tests.

…vices (when in exclude insecure mode)

andybalaam force-pushed the andybalaam/filter-untrusted-to-device branch from e273eba to 6c47df0 Compare July 1, 2025 12:06

andybalaam force-pushed the andybalaam/filter-untrusted-to-device branch 2 times, most recently from a3065e2 to b899348 Compare July 1, 2025 12:32

andybalaam marked this pull request as ready for review July 1, 2025 12:48

andybalaam requested review from a team as code owners July 1, 2025 12:48

andybalaam requested review from poljar and removed request for a team July 1, 2025 12:48

poljar requested changes Jul 2, 2025

View reviewed changes

andybalaam requested a review from poljar July 2, 2025 11:40

poljar requested changes Jul 14, 2025

View reviewed changes

BillCarsonFr reviewed Jul 15, 2025

View reviewed changes

andybalaam requested review from BillCarsonFr and poljar July 23, 2025 15:30

poljar approved these changes Jul 24, 2025

View reviewed changes

andybalaam added 6 commits July 24, 2025 14:53

refactor(crypto): Pass DecryptionSettings in to OlmMachine::decrypt_t…

98b064a

…o_device_event This will be used in the next commit, but it was very noisy, so I separated it out into this commit to make the next one easier to read.

refactor(crypto): Extract a method to check for to-device events from…

aa87c14

… dehydrated devices

refactor(tests): Take a reference to content in send_and_receive_encr…

2fe09fb

…ypted_to_device_test_helper This will allow us to re-use it in more tests.

refactor(tests): Re-use send_and_receive_encrypted_to_device_test_hel…

31a16a1

…per in 2 more tests

refactor(tests): Pass decryption_settings in to send_and_receive_encr…

1684a83

…ypted_to_device_test_helper To allow passing in different values in future tests.

feat(crypto): Refuse to decrypt to-device messages from unverified de…

a597752

…vices (when in exclude insecure mode)

andybalaam force-pushed the andybalaam/filter-untrusted-to-device branch from 0416539 to a597752 Compare July 24, 2025 13:56

andybalaam enabled auto-merge (rebase) July 24, 2025 13:56

andybalaam mentioned this pull request Jul 24, 2025

Option to exclude insecure devices for Olm traffic, to be consistent with megolm sharing? #4147

Open

andybalaam merged commit def1fed into main Jul 24, 2025
44 checks passed

andybalaam deleted the andybalaam/filter-untrusted-to-device branch July 24, 2025 14:08

	/// 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,
	},

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

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

Uh oh!

Conversation

andybalaam commented Jul 1, 2025

Uh oh!

codecov bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andybalaam Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poljar commented Jul 3, 2025

Uh oh!

andybalaam commented Jul 14, 2025

Uh oh!

poljar commented Jul 14, 2025

Uh oh!

andybalaam commented Jul 14, 2025

Uh oh!

andybalaam commented Jul 14, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andybalaam commented Jul 23, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 1, 2025 •

edited

Loading

andybalaam Jul 22, 2025 •

edited

Loading