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

EX: Messages sent after an identity reset can sometimes be flagged as sent from insecure device #2697

Open
Tracked by #2700
BillCarsonFr opened this issue Jan 20, 2025 · 11 comments
Assignees

Comments

@BillCarsonFr
Copy link
Member

BillCarsonFr commented Jan 20, 2025

Image

Steps to reproduce

  • Alice and Bob are talking in a room. Bob is using EX.
  • Put Bob in offline mode.
  • Logout Alice, then log-in again and reset the identity (as if you've forgot recovery)
  • Alice sends a new message
  • Wait a bit
  • Reopen Bob's session

The new message sent by Alice is marked as Sent from an device not verified by its owner. (Or, in "exclude insecure devices" mode, the message is hidden altogether.

This is an intermittent failure: it depends how long the /keys/query request takes to complete.

Note that:

  • Further messages sent by Alice after Bob comes back online are decrypted fine.
  • If Bob restarts his app after receiving Alice's messages, the timeline is updated and all messages are decrypted fine.

What happened:

When coming back online, Bob first receives the new room_key for the new message; the device_keys embedded in the to-device are are not signed with the identity we have for Alice (rather, they are signed by her new identity). The InboundGroupSession is therefore marked as insecure.

Meanwhile, Alice is marked as dirty, so the a new /keys/query is made; once that completes, we receive her new identity and devices. The device update triggers an update of the InboundGroupSession, meaning future messages are decrypted correctly; however the cached timeline is not updated on EX. (It is updated on EW.)

Related to matrix-org/matrix-rust-sdk#3890 and #2710.

@richvdh
Copy link
Member

richvdh commented Jan 21, 2025

duplicate of #2710?

@richvdh
Copy link
Member

richvdh commented Jan 28, 2025

I was unable to reproduce this, as written, on either EX or EW.

If Bob logs out before Alice logs in, the symptoms are as written; but that makes this a duplicate of #2710.

@richvdh richvdh closed this as completed Jan 28, 2025
@richvdh
Copy link
Member

richvdh commented Jan 28, 2025

Wait wait wait.

I was able to reproduce it on EX (only) by adding a sleep(2) in the server-side handling of /keys/query.

@richvdh richvdh reopened this Jan 28, 2025
@richvdh richvdh changed the title Exclude Insecure Device | First message sent after an identity reset can sometimes be flagged as sent from insecure device EX: Messages sent after an identity reset can sometimes be flagged as sent from insecure device Jan 28, 2025
@richvdh richvdh self-assigned this Jan 28, 2025
@richvdh
Copy link
Member

richvdh commented Jan 29, 2025

In EW, whenever we get an update to an existing InboundGroupSession, such as an update to the SenderData, we trigger another decryption event, which may result in an update to the UI.

In EX, we only re-decrypt UTD messages (https://github.com/matrix-org/matrix-rust-sdk/blob/d755a8a3aa0ecf7b718487b0401b9f3cee7f3155/crates/matrix-sdk-ui/src/timeline/controller/mod.rs#L1048): I think we should remove this constraint.

@richvdh richvdh removed their assignment Jan 29, 2025
@andybalaam andybalaam self-assigned this Feb 4, 2025
@andybalaam
Copy link
Member

Wait wait wait.

I was able to reproduce it on EX (only) by adding a sleep(2) in the server-side handling of /keys/query.

Something like this @richvdh ?

$ git diff
diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py
index 7025662fdc..b01d54ed4a 100644
--- a/synapse/rest/client/keys.py
+++ b/synapse/rest/client/keys.py
@@ -195,6 +195,7 @@ class KeyQueryServlet(RestServlet):
     def __init__(self, hs: "HomeServer"):
         super().__init__()
         self.auth = hs.get_auth()
+        self.clock = hs.get_clock();
         self.e2e_keys_handler = hs.get_e2e_keys_handler()
 
     @cancellable
@@ -220,6 +221,9 @@ class KeyQueryServlet(RestServlet):
         result = await self.e2e_keys_handler.query_devices(
             body, timeout, user_id, device_id
         )
+
+        await self.clock.sleep(2)
+
         return 200, result

@richvdh
Copy link
Member

richvdh commented Feb 4, 2025

yes, something like that!

@andybalaam
Copy link
Member

I can't reproduce this, even with a 10 second delay in Synapse's /keys/query response:

Image

@andybalaam
Copy link
Member

I noticed that the messages were UTD for a while, and then they appeared, so I'm guessing this is why

@andybalaam
Copy link
Member

Or maybe just because I am actually stopping and restarting the EX process. I will try again to find a way to stop the network without restarting the process.

@andybalaam
Copy link
Member

I am able to reproduce this when I can persuade the Android emulator to go offline. I think the reason it was difficult was because I am reverse-forwarding localhost:8008 on the device to my local machine's Synapse with adb reverse tcp:8008 tcp:8008 and that stays working even when I go into flight mode.

To go offline I run:

abd reverse --remove-all

Note that this doesn't work on its own. I also need to restart Synapse, presumably because that breaks the established connection.

Then to go back online, I just run the original adb reverse tcp:8008 tcp:8008 command again (no need to restart Synapse).

@andybalaam
Copy link
Member

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants