MSC3700: Deprecate plaintext sender key #3700

erikjohnston · 2022-02-03T14:22:26Z

This deprecates the superfluous sender_key and device_id in a megolm message, e.g. an encrypted room event.

Implementation: matrix-org/matrix-rust-sdk#481

Preview: https://pr3700--matrix-org-previews.netlify.app

erikjohnston · 2022-02-03T14:26:46Z

proposals/3700-deprecate-sender-key.md

+When updating an existing session key, clients must ensure:
+1. that the updated session data comes from a trusted source, e.g. either the
+   session data has a) a valid signature, or b) comes from the user’s session
+   key backup; and


We should spell out what we mean by "trusted source" here

erikjohnston · 2022-02-04T15:12:26Z

I have done an implementation in the matrix-rust-sdk here: matrix-org/matrix-rust-sdk#481

anoadragon453 · 2022-03-01T15:38:19Z

The positive review of matrix-org/matrix-rust-sdk#481 makes me cautiously optimistic about the implementation side of this. Though I would still like to see a review here from someone more intimately familiar with megolm before calling FCP.

erikjohnston · 2022-03-22T15:13:41Z

I think this is ready for FCP

proposals/3700-deprecate-sender-key.md

richvdh · 2022-03-22T18:22:00Z

proposals/3700-deprecate-sender-key.md

+globally unique and so no disambiguation using `sender_key` or `device_id` is
+needed.
+
+Session IDs are encoded ed25519 public keys; when new sessions are shared they


Session IDs are encoded ed25519 public keys

I think we should consider strengthening the wording in the spec to make this clear. Currently, https://spec.matrix.org/v1.2/client-server-api/#mmegolmv1aes-sha2 just says "outbound_group_session_id", without clarity on what exactly it is.

Buried within https://gitlab.matrix.org/matrix-org/olm/blob/master/docs/megolm.md we have "... the public part of K (acting as a session identifier) ...", but it's a bit hard to join the dots as things stand.

(Background: when I designed the megolm message format, the intention was that receiving clients would treat session_id as opaque, and they had no guarantee that it would be unique to a given device or even sender, hence the presence of the other fields.)

proposals/3700-deprecate-sender-key.md

richvdh · 2022-03-22T18:26:33Z

seems good to me.

@mscbot fcp merge

mscbot · 2022-03-22T18:31:01Z

Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

uhoreg · 2022-03-30T02:16:45Z

proposals/3700-deprecate-sender-key.md

+should still include it when generating *if* there is a `sender_key` field in
+the event we're requesting keys for.
+
+Clients must store and lookup sessions based purely on the session ID.


This makes it sound like clients are not allowed to store sessions based on room ID or sender user ID, which I don't think is what is intended.

Hmm, that is an interesting question actually. I think we don't want clients to ever lookup inbound sessions based on room ID or sender user ID, as they can be faked in exactly the same way as sender_key and device_id. Though I guess clients need to explicitly check that the sender user and room matches the session? I can make that more explicit.

At least for room ID, it's hard to fake since the event itself is tied to the room. (Also, your rust SDK PR stores by room ID and session ID.) Maybe it could say something like:

Clients should store and lookup sessions based purely on the session ID or based on the room ID and session ID. Clients must not store sessions based on the sender key or device ID.

?

At least for room ID, it's hard to fake since the event itself is tied to the room.

The server could easily lie about the room ID for an event though. If we don't check the room ID matches the session room ID then it allows servers to forward encrypted events into other rooms.

I've updated the language to:

Clients must not store or lookup sessions using the sender key or device ID.

Client must continue to ensure that the event's sender and room ID fields match
those of the looked up session, e.g. by storing and looking up session using the
room ID and sender as well as the session ID.

The server could easily lie about the room ID for an event though. If we don't check the room ID matches the session room ID then it allows servers to forward encrypted events into other rooms.

It can't, the room ID is part of the encrypted payload as well[1].

Hmm, so there are three places where we have a room ID? The event, the session and the encrypted payload? Presumably we still want to check that they all match?

Well, the event room id is a hint to find the right bucket, the session room id is just a hint into which bucket to put the session. The room id on the session also can't be replaced by the server.

The encrypted payload room id ensures that the server doesn't lie about the room id.

Well, the event room id is a hint to find the right bucket, the session room id is just a hint into which bucket to put the session. The room id on the session also can't be replaced by the server.

Sure, but that relies on the client actually verifying the room IDs match. This can be done as you say by storing the sessions under the room ID they correspond to and then looking the session up via the room ID.

However, there is a risk here that clients will do the following:

Look up sessions solely via session ID.

Not check the room ID in the encrypted payload

Display the message in the room corresponding to the event's room ID.

At that point the client is vulnerable to the server lying about the event's room ID and making the client display the verified encrypted content into the wrong room.

A similar attack can happen with the sender.

The key issue here is that clients need to handle both encrypted and unecrypted events, so they're very likely to use the event's room ID and sender when rendering things in the UI, rather than any data in the encrypted payload or session. This makes it very important for clients to actually check that the user ID/senders and room IDs match.

This makes it very important for clients to actually check that the user ID/senders and room IDs match.

Agreed. But what can we do to make this more prominent? The spec has some vague wording on this for room IDs:

We include the room ID in the payload, because otherwise the homeserver would be able to change the room a message was sent in.

But this is IMO neither clear nor noticeable enough. There is also this bogus paragraph:

As with Olm events, clients must confirm that the sender_key belongs to the user who sent the message. The same reasoning applies, but the sender ed25519 key has to be inferred from the keys.ed25519 property of the event which established the Megolm session.

This seems like a description of exactly what not to do. Clients should not look at sender_key as the server can easily spoof that (and is what's now being removed!). Instead, clients should record the (Curve25519) identity key and (Ed25519) fingerprint keys of the Olm device that initially shared a particular Megolm session.

I guess we need clear, actionable steps of what to do upon an encrypted room message receipt. Something like:

Upon receipt of an encrypted room message, do the following:

Record (Curve25519) identity key and (Ed25519) fingerprint key of the Olm device that initially sends the m.room_key for a particular message.

Check that the m.room.encrypted event's room ID matches the room_id field in the encrypted plaintext of the message. If not, fail.
...

If any of the above checks fail, the client must not display the message as a normal message. The client may display the message with a clear indication that it violates a required security property.

Ideally these rules would be in a differently coloured box to make it stand out from the rest of the text.

BillCarsonFr · 2022-04-11T16:02:31Z

proposals/3700-deprecate-sender-key.md

+
+When clients receive an encrypted event with an unknown session they will need
+to send a key request to all clients, rather than the device specified by
+`sender_key` and `device_id`. This is the current behaviour used by Element


The current behavior is to send to all your own devices with wildcard, but to the specific device_id of the sender. So this will need to change as it's not the current behavior. Notice that it will produce new to_device traffic

MSC: matrix-org/matrix-spec-proposals#3700 ([Markdown](https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/3700-deprecate-sender-key.md)) The language around `m.room.encrypted` is a bit awkward because *technically* you can use the event to represent non-Megolm events, however that's considered an edge case at this time.

turt2live · 2022-05-31T00:48:14Z

Spec PR: matrix-org/matrix-spec#1101

* Deprecate the `sender_key` and `device_id` on Megolm events MSC: matrix-org/matrix-spec-proposals#3700 ([Markdown](https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/3700-deprecate-sender-key.md)) The language around `m.room.encrypted` is a bit awkward because *technically* you can use the event to represent non-Megolm events, however that's considered an edge case at this time. * changelog * Apply wording changes * Remove incorrect example * Add missing sentence

turt2live · 2022-06-09T08:25:10Z

Merged 🎉

MRAAGH · 2022-09-21T14:52:46Z

This sounds like it might make decryption issues harder to debug; if there's no plaintext ID in the event, I won't even be able to check which client messages were sent from

anoadragon453 · 2022-09-21T15:03:11Z

@MRAAGH Note that reduced ease of debugging is a known downside - it's listed at the bottom of the MSC. It was determined to be worth it for the increased privacy benefits (I can no longer tell you're sending messages from your phone at the moment).

I agree that this makes debugging old, undecryptable messages more difficult. But hopefully for recent conversations the involved parties will know which client sent what message.

BillCarsonFr · 2023-03-29T12:51:27Z

A late comment to note that this MSC is impacting MSC2399, as no_olm withheld codes are sent without session_id or room_id. This will make it impossible to relate a no_olm code to an unable to decrypt message.
We might need to modify MSC2399 so that no_olm codes are sent in the same way as other codes, removing the optimisation for sending them only once.

cc @poljar @uhoreg

This is mostly to see if we can deal with MSC3700 already. I need to add back sending the sender key in requests and such, but ignore it when processing events. See matrix-org/matrix-spec-proposals#3700

erikjohnston force-pushed the erikj/deprecate_sender_key branch from efdfed7 to 48e87de Compare February 3, 2022 14:23

erikjohnston changed the title ~~MSC: Deprecate plaintext sender key~~ MSC3700: Deprecate plaintext sender key Feb 3, 2022

Initial deprecate sender key MSC

3908f07

erikjohnston force-pushed the erikj/deprecate_sender_key branch from 48e87de to 3908f07 Compare February 3, 2022 14:23

Formatting

b594a0b

erikjohnston commented Feb 3, 2022

View reviewed changes

turt2live added proposal A matrix spec change proposal proposal-in-review kind:maintenance MSC which clarifies/updates existing spec e2e needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Feb 3, 2022

Deprecate 'sender_key' in 'm.room_key_request'

81b38c8

erikjohnston mentioned this pull request Feb 4, 2022

MSC3700: Ignore content.sender_key matrix-org/matrix-rust-sdk#481

Closed

erikjohnston removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Mar 22, 2022

richvdh reviewed Mar 22, 2022

View reviewed changes

mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Mar 22, 2022

erikjohnston and others added 3 commits March 23, 2022 10:10

Update proposals/3700-deprecate-sender-key.md

e97461a

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update proposals/3700-deprecate-sender-key.md

2eb74bf

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Formatting

c080d7e

uhoreg reviewed Mar 30, 2022

View reviewed changes

turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Apr 10, 2022

BillCarsonFr mentioned this pull request Apr 11, 2022

Support deprecation of plain text sender_key and device_id in a megolm message element-hq/element-meta#223

Open

BillCarsonFr reviewed Apr 11, 2022

View reviewed changes

BillCarsonFr mentioned this pull request Apr 28, 2022

Share Megolm session keys when inviting a new user element-hq/element-android#5853

Merged

turt2live mentioned this pull request May 16, 2022

Matrix 1.3 matrix-org/matrix-spec#1063

Closed

15 tasks

turt2live self-assigned this May 30, 2022

turt2live mentioned this pull request May 31, 2022

Deprecate the sender_key and device_id on Megolm events matrix-org/matrix-spec#1101

Merged

turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels May 31, 2022

turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Jun 9, 2022

zecakeh mentioned this pull request Jun 10, 2022

Incorporate Matrix v1.3 changes ruma/ruma#1145

Closed

66 tasks

MRAAGH mentioned this pull request Sep 15, 2022

sender_key and device_id were deprecated matrix-nio/matrix-nio#346

Open

clokep mentioned this pull request Oct 3, 2022

Support Matrix 1.3 matrix-org/synapse#13953

Closed

11 tasks

poljar mentioned this pull request Oct 7, 2022

Don't use the Curve25519 sender key to store room keys matrix-org/matrix-rust-sdk#1091

Merged

uhoreg mentioned this pull request Nov 14, 2022

Element-web sends, and responds to, hopeless m.room_key_request messages element-hq/element-web#23766

Closed

BillCarsonFr mentioned this pull request May 11, 2023

MSC4014: Pseudonymous Identities #4014

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC3700: Deprecate plaintext sender key #3700

MSC3700: Deprecate plaintext sender key #3700

erikjohnston commented Feb 3, 2022 •

edited by turt2live

Loading

erikjohnston Feb 3, 2022

erikjohnston commented Feb 4, 2022

anoadragon453 commented Mar 1, 2022

erikjohnston commented Mar 22, 2022

richvdh Mar 22, 2022

richvdh commented Mar 22, 2022

mscbot commented Mar 22, 2022 •

edited by uhoreg

Loading

uhoreg Mar 30, 2022

erikjohnston Apr 5, 2022

uhoreg Apr 5, 2022

erikjohnston Apr 6, 2022

poljar Apr 6, 2022

erikjohnston Apr 6, 2022

poljar Apr 6, 2022

erikjohnston Apr 6, 2022

dkasak Apr 15, 2022 •

edited

Loading

BillCarsonFr Apr 11, 2022

turt2live commented May 31, 2022

turt2live commented Jun 9, 2022

MRAAGH commented Sep 21, 2022 •

edited

Loading

anoadragon453 commented Sep 21, 2022

BillCarsonFr commented Mar 29, 2023

MSC3700: Deprecate plaintext sender key #3700

MSC3700: Deprecate plaintext sender key #3700

Conversation

erikjohnston commented Feb 3, 2022 • edited by turt2live Loading

Choose a reason for hiding this comment

erikjohnston commented Feb 4, 2022

anoadragon453 commented Mar 1, 2022

erikjohnston commented Mar 22, 2022

Choose a reason for hiding this comment

richvdh commented Mar 22, 2022

mscbot commented Mar 22, 2022 • edited by uhoreg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkasak Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live commented May 31, 2022

turt2live commented Jun 9, 2022

MRAAGH commented Sep 21, 2022 • edited Loading

anoadragon453 commented Sep 21, 2022

BillCarsonFr commented Mar 29, 2023

erikjohnston commented Feb 3, 2022 •

edited by turt2live

Loading

mscbot commented Mar 22, 2022 •

edited by uhoreg

Loading

dkasak Apr 15, 2022 •

edited

Loading

MRAAGH commented Sep 21, 2022 •

edited

Loading