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.

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

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

Codecov Report

Attention: Patch coverage is 91.66667% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.75%. Comparing base (c2f50fd) to head (4c1ef53).
Report is 40 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/machine/mod.rs	91.89%	4 Missing and 2 partials ⚠️
...es/matrix-sdk-common/src/deserialized_responses.rs	0.00%	2 Missing ⚠️
crates/matrix-sdk-base/src/client.rs	83.33%	0 Missing and 1 partial ⚠️
...sdk-base/src/response_processors/e2ee/to_device.rs	83.33%	0 Missing and 1 partial ⚠️
crates/matrix-sdk-base/src/sliding_sync.rs	85.71%	0 Missing and 1 partial ⚠️
...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.74%   88.75%   +0.01%     
==========================================
  Files         332      332              
  Lines       90030    90141     +111     
  Branches    90030    90141     +111     
==========================================
+ Hits        79895    80004     +109     
- Misses       6316     6317       +1     
- Partials     3819     3820       +1

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

…vices (when in exclude insecure mode)

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?

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.

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.

Co-authored-by: Damir Jelić <poljar@termina.org.uk> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

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?

andybalaam added 5 commits July 1, 2025 10:31

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

97ca9ee

…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…

94b28a1

… dehydrated devices

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

c3c6025

…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…

e638677

…per in 2 more tests

refactor(tests): Pass decryption_settings in to send_and_receive_encr…

92d064e

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

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 from 6c47df0 to a3065e2 Compare July 1, 2025 12:23

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

b899348

…vices (when in exclude insecure mode)

andybalaam force-pushed the andybalaam/filter-untrusted-to-device branch 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

Apply suggestions from code review

4c1ef53

Co-authored-by: Damir Jelić <poljar@termina.org.uk> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

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

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

Are you sure you want to change the base?

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!

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!

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!

Uh oh!

codecov bot commented Jul 1, 2025 •

edited

Loading