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

First message in a DM was not encrypted #28094

Open
andybalaam opened this issue Sep 24, 2024 · 5 comments
Open

First message in a DM was not encrypted #28094

andybalaam opened this issue Sep 24, 2024 · 5 comments
Labels
A-DM-Start Creating a DM with another user A-E2EE O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Critical Prevents work, causes data loss and/or has no workaround T-Defect

Comments

@andybalaam
Copy link
Contributor

Steps to reproduce

  1. Start new chat
  2. Type mxid of other person, click Go
  3. Type a message to the other person

Outcome

What did you expect?

I expected all messages in this DM to be encrypted.

What happened instead?

The first message was not encrypted:

image

Source of that first event:

{
  "content": {
    "body": "Hi, thank you for looking at MSC3757, and sorry I have neglected it!",
    "m.mentions": {},
    "msgtype": "m.text"
  },
  "origin_server_ts": 1727188841453,
  "sender": "@andybalaam:matrix.org",
  "type": "m.room.message",
  "unsigned": {
    "membership": "join",
    "age": 12541,
    "transaction_id": "m1727188841284.81"
  },
  "event_id": "$RX_kUJTWDQL3DVE1fRkx2foUYt73DXpyaMlHRA73TXU",
  "room_id": "!qKSwlzySCzuQreJffe:matrix.org"
}

Later messages in this DM were encrypted as expected.

Operating system

Debian 12.7

Browser information

Firefox 130.0

URL for webapp

https://develop.element.io

Application version

Element version: 787feca-react-a1bdceed3e54-js-f8208b18916d Crypto version: Rust SDK 0.7.2 (7a21514), Vodozemac 0.7.0

Homeserver

matrix.org

Will you send logs?

Yes

@dosubot dosubot bot added A-DM-Start Creating a DM with another user A-E2EE O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Sep 24, 2024
@kegsay
Copy link
Contributor

kegsay commented Sep 24, 2024

This could be due to the same race condition I outlined in my UTD talk - if we only latch encryption on if we see the m.room.encrpytion state event, then what happens if you try to send a message before the client is notified about that event via /sync?

@BillCarsonFr
Copy link
Member

I recently had a similar problem of EW sending in clear but with a local setup, I sent a RS https://github.com/element-hq/element-web-rageshakes/issues/27122
As far as I remember it was not a fresh DM. Maybe not related as it was a local setup running local code. But yet adding here

@dkasak
Copy link
Member

dkasak commented Sep 30, 2024

This could be due to the same race condition I outlined in my UTD talk - if we only latch encryption on if we see the m.room.encrpytion state event, then what happens if you try to send a message before the client is notified about that event via /sync?

Since encryption cannot be switched off, I wonder if there's any real benefit of it being able to get switched on. Why not embed this information in the m.room.create event? The rest of the use cases could be served with room upgrades.

@kegsay
Copy link
Contributor

kegsay commented Sep 30, 2024

It's not an immutable property though, it's a latch. You can enable it but not disable it. If it's part of the create event, you cannot go from unencrypted => encrypted. You also then can't change encryption parameters (algorithm, rotation period, etc), which feels undesirable.

@dkasak
Copy link
Member

dkasak commented Sep 30, 2024

If it's part of the create event, you cannot go from unencrypted => encrypted.

Yes, this is what I meant by "rest of the use cases" which should IMO be served by room upgrades. I'm arguing it should be an immutable property. It not being one makes it fragile.

You also then can't change encryption parameters (algorithm, rotation period, etc), which feels undesirable.

I'm not sure it's actually undesirable. Configurable encryption parameters are a misfeature due to the potential for rollback/downgrade they introduce, especially since our state events are not client-side authenticated using cryptography. In fact, we already severely limit what you can actually change using a m.room.encryption event for this exact reason—and it's next to nothing. The algorithm is already immutable in practice and the rotation period is severely limited.

My argument is that all of this would be better modelled with a room upgrade.

@t3chguy t3chguy added S-Critical Prevents work, causes data loss and/or has no workaround O-Uncommon Most users are unlikely to come across this or unexpected workflow and removed S-Major Severely degrades major functionality or product features, with no satisfactory workaround O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-DM-Start Creating a DM with another user A-E2EE O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Critical Prevents work, causes data loss and/or has no workaround T-Defect
Projects
None yet
Development

No branches or pull requests

5 participants