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.

Sign up for GitHub

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

MSC4245: Immutable encryption algorithm #4245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+63 −0
Open

MSC4245: Immutable encryption algorithm #4245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions proposals/4245-immutable-encryption-algorithm.md
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation requirements:

  • Client
  • Server (to prevent clients making mistakes)
  • A rollout plan

Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# MSC4245: Immutable encryption algorithm

Enabling encryption in a room after it's been created works well for some encryption algorithms,
like Megolm in today's specification, but doesn't work well when aiming to support RFC 9420 MLS in
Matrix. Rooms using MLS will be required to change their authorization rules and possibly state
resolution behaviours, which could be breaking if a server misses the [`m.room.encryption`](https://spec.matrix.org/v1.13/client-server-api/#mroomencryption)
state event being sent. To avoid this situation, this proposal suggests that the encryption algorithm
can *optionally* be specified in the [`m.room.create`](https://spec.matrix.org/v1.13/client-server-api/#mroomcreate)
event instead, nullifying the `m.room.encryption` event's `algorithm` field.
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see immutable encryption in all rooms, not just MLS rooms, and potentially even make it non-optional in future room versions. Changing the encryption protocol for a room from a server-controlled setting is inherently problematic and almost never useful.

Indeed, clients today mostly ignore any changes in the room encryption algorithm after they've observed the first m.room.encryption event. This means the existence of the algorithm field only risks client splitbrains without actually serving any purpose.

Placing the algorithm in the create event would ensure uniqueness because to change it, you'd have to be in a different room. This also hints at the natural way to change the algorithm: room upgrades.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this proposal isn't MLS-specific, fwiw. It's just using MLS as a driver. The encryption_algorithm could also be a megolm string, making that room stuck on megolm.

The rationale for optionality is to allow existing rooms to continue "upgrading" from unencrypted to encrypted, while setting a standard for future rooms to use. As we see uptake (and critically, problems/cheers as a result of the immutability), we could remove m.room.encryption's algorithm field.

Copy link
Member

@dkasak dkasak Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this proposal isn't MLS-specific, fwiw. It's just using MLS as a driver. The encryption_algorithm could also be a megolm string, making that room stuck on megolm.

Great!

The rationale for optionality is to allow existing rooms to continue "upgrading" from unencrypted to encrypted

I'm not sure that the benefits outweigh the costs, so I'm raising this as an early request to look into whether we might want to be more proactive on this front. The encryption algorithm is very much something that should be baked into the room / group, and there should be no opportunities for participants to disagree on whether the room is encrypted or not and using which algorithm.

The modern Matrix approach is to rely on room upgrades more heavily, so it's really hard to come up with a convincing reason why encryption, of all things, should be left malleable, with all the downsides that carries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale for optionality is to allow existing rooms to continue "upgrading" from unencrypted to encrypted

... which is fine when all participants are behaving and have accepted the m.room.encryption event, but due to the eventually consistent nature of Matrix, that's not guaranteed to happen immediately or even within any reasonable timeframe. Worse is that someone who enables encryption on a room has no visibility whatsoever into whether other participant servers are consistent or not until one of their users sends a message, by which time a leak could have happened in plain-text.

I can see value in just deprecating m.room.encryption altogether in favour of an approach like this.



## Proposal

When the `m.room.create` event's `content` contains an `encryption_algorithm` field, that encryption
algorithm MUST be used within that room. The `algorithm` field of `m.room.encryption` serves no purpose
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, MSC2883 basically does the same thing, though it uses a different name (encryption instead of encryption_algorithm). But it's easy enough to harmonize the naming. In MSC2883, I did consider making the encryption property an object (e.g. encryption: {algorithm: "...", ...}), in case we wanted to make other encryption parameters immutable. But I don't know that there are any other such parameters.

in such rooms.

If the `m.room.create` event's `content` does not have an `encryption_algorithm`, the `m.room.encryption`
state event operates as per usual.

Other fields/properties of `m.room.encryption` are not modified by this proposal, such as key rotation
periods.

Note that an empty string, or unrecognized value, may be present in `encryption_algorithm`: this is
legal, and causes `m.room.encryption`'s `algorithm` to be ignored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if encryption_algorithm is null? It would be good to be explicit about this, even if the answer is that it's the same as any other unrecognised value.



## Alternatives

None applicable


## Potential issues

From a security perspective, it's entirely possible that the `m.room.encryption` event's `algorithm`
is different from the `m.room.create`'s `encryption_algorithm`. Clients may (rightly) get confused
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possibility is to extend the auth rules so that an m.room.encryption event will only be accepted if either the create event doesn't specify an algorithm or if the algorithm matches the one specified in the create event exactly.

if they don't yet recognize `encryption_algorithm` in the create event, and use a less secure algorithm
in the room. Receiving clients may further accept these events and attempt to decrypt them instead
of rejecting due to the wrong algorithm being used. Or, worse, there are effectively two rooms going
on as half the participants only encrypt for their half of the room.

Solving this is tricky, and may require a room version bump with significant public information
campaigns to encourage client developers to adopt the change, once the MSC is accepted. The Spec Core
Team should consider rollout requirements prior to Final Comment Period of `merge` for this MSC.


## Security considerations

See potential issues.


## Dependencies

This MSC is not dependent on any other MSCs. [MSC4244](https://github.com/matrix-org/matrix-spec-proposals/pull/4244)
relies on the functionality introduced by this proposal. Others may additionally exist.


## Unstable prefix

While this proposal is not considered stable, implementations should use `org.matrix.msc4245.encryption_algorithm`
in the create event instead of `encryption_algorithm`. Developers should note that rooms created
using this unstable prefix may break when implementations drop support for the unstable prefix,
and not expose such rooms to production end users.
Loading