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

MSC3917: Cryptographically Constrained Room Membership #3917

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

duxovni
Copy link

@duxovni duxovni commented Oct 25, 2022

@uhoreg uhoreg added e2e proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. requires-room-version An idea which will require a bump in room version labels Oct 25, 2022
@turt2live turt2live added unassigned-room-version Remove this label when things get versioned. hacktoberfest-accepted labels Oct 25, 2022
| Attack | Membership tree | State DAG | Signal |
|---|---|---|---|
| Homeserver admin inserts a new member | ✅ | ✅ | ✅ |
| Homeserver admin re-inserts a banned member | ❌ | ✅ | ❌ |
Copy link
Contributor

Choose a reason for hiding this comment

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

Also applies to kicked members or the ones that left on their own. I think that in general is very far down, since that is a major downside, that any member that ever was in the room can be reinserted by the homeserver.

Copy link
Author

Choose a reason for hiding this comment

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

You're correct, when I said "banned member", I wasn't implying that there's anything unique in this regard about the operation of banning as opposed to kicking or leaving.

This shortcoming is raised and discussed in the very first paragraph of the Potential Issues section, along with an argument for why this proposal is nevertheless valuable. The side-by-side comparison of the alternatives is further down in the document because I can't compare the alternative system before I've described what the alternative system is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't quite sure where to put the comment, so this was just one place. But the problem isn't as much reinserting the member or authing new events, it is that the homeserver could just choose to never propagate the leave event or not provide it to a new initial sync, which would make it look like the user never left.

Specifically this:

It does not cryptographically verify
that the senders of those state events had the required power levels
at the time they issued the events, or that they didn't leave or get
banned between joining and issuing the events.

It talks about issuing events, which I needed to read several times to understand, that it means "issuing the original join event". I think if you are not already aware about the problem with rescinding a membership, then this looks like it is talking about sending new messages, that can't be verified that they should or should not be in the room. Maybe that can be fixed by changing it to something like this?:

It does not cryptographically verify
that the senders of those state events had the required power levels
at the time they issued the events. It also does not verify that the user
didn't leave, got kicked or banned in the meantime. The signature would still
be valid for their join event, they however should not be treated as a member
anymore.

Copy link
Author

Choose a reason for hiding this comment

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

But the problem isn't as much reinserting the member or authing new events, it is that the homeserver could just choose to never propagate the leave event or not provide it to a new initial sync, which would make it look like the user never left.

This is in the first paragraph of the Alternatives section as the motivation for the alternative proposal which solves these problems.

Copy link
Author

@duxovni duxovni Oct 28, 2022

Choose a reason for hiding this comment

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

And to be clear, when I say "issuing events", I mean issuing invite events - it isn't just that banned members can remain in the room, it's that anyone who's ever been in the room can always invite others from the perspective of this cryptographic verification. The homeserver is still expected to enforce these sorts of permissions, but the homeserver can only tamper with a room's membership with the collaboration of someone who has been a legitimate member in the past.

Copy link
Author

@duxovni duxovni left a comment

Choose a reason for hiding this comment

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

The verification process relies on being able to access a user's invite event and their join event as separate entities, including after the user has left the room. Under the current spec, these would all be m.room.member events with the same state key, and would overwrite each other. The same also goes for older m.room.join_rules events.

I see two options here:

  • These signed state events can include the entire chain of stripped parent state events within their event content, just as in the case where a joining user is proving their membership in a parent space. For instance, a join event would also include a copy of the invite event that authorized it, and that copy of the invite event would include a copy of the inviter's cause-of-membership event, and so on back to the room's creation. (Disadvantage: the sizes of invite, join, and join-rules events will monotonically increase over time as the chains of inviters and invitees grow longer.)
  • Alternatively, we could explicitly mandate that old versions of these state events be kept around by the server, and shown to clients that explicitly request them by their event IDs. (Disadvantage: this may require homeserver implementers to change storage schemas based on the old assumption that stored state events are unique per room/type/state-key.)

Comment on lines +63 to +72
This proposal has each user generate a new cryptographic signing key
called the Room Signing Key, or RSK. The RSK is used for signing
certain types of room state events that the user sends (specifically,
invitations, joins, and join rule changes), so that other room members
can verify that the events were genuinely sent by that user. The RSK
should be signed by the Master Signing Key, and stored and retrieved
alongside the user's other signing keys. This key will be identified
by the string `m.cross_signing.room_signing`, and will be published to
the `/keys/device_signing/upload` endpoint using the new optional
field `room_signing_key`, with usage `["room_signing"]`.

Choose a reason for hiding this comment

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

By signing the RSK using the MSK, this proposal creates a cryptographic publishable proof that a user participates in a room (if they perform an action which uses the RSK).

The signing key used within a Megolm session is unsigned by the user so cannot prove that a user sent the message in a way which could not be faked by another participant. Although a homeserver could produce various records they would not be signed by the user so again could be faked.

Although deniability is not an explicit security guarantee it should be acknowledged that this change removes it. A possible solution would be to add the users MSK directly to the membership event, and allow the RSK to either be signed or unsigned. Some mechanism in m.room.create and m.room.join_rules could be used to signal if an unsigned RSK is acceptable.

Verification of an RSK which is unsigned could be achieved using an inband message encrypted message to the room for example, which would inherit the authenticity guarantees of the Megolm session without creating a cryptographic proof.

@cvwright
Copy link

Is anyone actively working on an implementation of this?

@uhoreg
Copy link
Member

uhoreg commented Mar 31, 2023

We are planning on working on it, but are not currently doing so yet.

connecting the sender's MSK to their RSK, particularly in cases
where the sender may no longer be in the room or may have even
deactivated their account.
+ `parent_event_id`:
Copy link

Choose a reason for hiding this comment

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

I wonder if we can borrow this idea of having a back-pointer to the previous membership state event to do something similar for leave and ban events. If so, we might be able to cheaply prevent a malicious server from re-adding a banned/left user by re-sending the content from an old join event.

Maybe all that would be required is to include the parent_event_id for ban and leave too, pointing back to the join event that is being "revoked" or undone. Then when a client receives a new join event for a banned or left member, it can compare the new signatures against those in the previous join event for that user (the one that was the parent for the leave or ban). Ed25519 signatures are randomized, so if the new join event is legitimate, then the new signature should not be identical to the old one. If the two signatures are bit-for-bit identical, then the client can be sure that they are looking at a replay attack.

Of course, this doesn't work for more complicated scenarios where a given user is repeatedly joined, then banned, then re-invited and re-joins, is banned again, and so on and so forth. Full protection would require fully verifying the DAG of state events on the client as described in the second half of this MSC. But still, checking the parent event is low-hanging fruit and should cover the most common case of a replay attack in practice.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, as you say, it's not a complete fix. But it looks like it's easy enough to do and will catch some cases.

@Michael-Hollister
Copy link

Michael-Hollister commented May 16, 2023

We might want to consider having clients sign m.room.tombstone events to ensure users that created the event are verified room members . Since this MSC will require a new room version, a malicious homeserver could downgrade the room version to a previous version without cryptographic room membership verification by injecting a fake m.room.tombstone event in the room.

the RRK should *be* the room ID. Similar to
[MSC1228](https://github.com/matrix-org/matrix-spec-proposals/pull/1228),
the new format of a room ID will be `![unpadded urlsafe-base64ed
ed25519 public key]`,

Choose a reason for hiding this comment

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

Limiting the key type to ed25519 is bad practice considering potential future threats of quantum computing. So a mechanism should be established to allow cryptoagility. See also https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Crypto/Migration_to_Post_Quantum_Cryptography.pdf?__blob=publicationFile&v=2

"content": {
"room_version": "9",
"creator": "@alice:example.com",
"room_root_key" : "/ZK6paR+wBkKcazPx2xijn/0g+m2KCRqdCUZ6agzaaE",
Copy link
Member

Choose a reason for hiding this comment

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

presumably this is supposed to match the room_id

"sender": "@alice:example.com",
"content": {
"room_version": "9",
"creator": "@alice:example.com",
Copy link
Member

Choose a reason for hiding this comment

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

creator was removed by msc2175. presumably the intention is not to reintroduce it.

communications channel the URIs were sent through, which is again a
fundamental limit.

## Potential issues
Copy link
Member

Choose a reason for hiding this comment

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

It's worth emphasising that, in order to validate a given event, the client must have access to all of the events in its chain of trust, and this is not currently available via the /sync api.

For example, if I join an invite-only room where Alice is only a member, /sync will give me Alice's join event, but not the invite from Bob event that allowed the join to take place. Similarly Bob may have left the room since inviting Alice, in which case my client will get Bob's leave event rather than his join event, so can't validate the invite he sent to Alice. It gets even worse when Matrix state resolution is thrown into the mix (since state res can legitimately cause state events to be replaced by earlier instances of the state.)

All these events should be in the auth chain, so the server should have them, and an easy solution is for the client to request said events via a /context request. That won't be terribly efficient, but a more efficient mechanism could be designed later.

More of a concern is that, in a room with shared or joined history visiblity, many of the events in the chain won't be accessible to a newly-joined user.

We could probably relax the history visibility rules in some way to make old m.room.member and m.room.join_rules events visible even to newly-joined users, but we would need to be mindful of the potential resultant metadata leak (eg, getting a complete list of everyone who has ever participated in a room).

Comment on lines +7 to +8
members will not receive keys for past messages, since those are only
shared by existing members when they invite new members, but the
Copy link
Member

Choose a reason for hiding this comment

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

This is referring to MSC3061, which is pending disclosure of security concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API e2e hacktoberfest-accepted kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal requires-room-version An idea which will require a bump in room version unassigned-room-version Remove this label when things get versioned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.