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

MSC3757: Restricting who can overwrite a state event #3757

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ff5fd48
Add 'Restricting who can overwrite a state event'
andybalaam Mar 25, 2022
610f244
Rename MSC3757 to its proper number
andybalaam Mar 25, 2022
bfde329
Clarify wording of comment
andybalaam Mar 31, 2022
344e876
Add unstable room version
andybalaam Mar 31, 2022
ccb7e52
Note that this requires a new room version
andybalaam Mar 31, 2022
1ce7e0e
Refer to MSC3760 as an alternative
andybalaam Mar 31, 2022
6df6109
Re-word link to MSC3760
andybalaam Mar 31, 2022
6e108b3
Update to m.self to match latest thinking on MSC3672
andybalaam Apr 26, 2022
bd4176f
Remove the user of 'unprivileged'
andybalaam May 11, 2022
e352a1d
Add a note about allowed characters
andybalaam May 11, 2022
5e95ff3
Reword proposed auth rule
AndrewFerr Sep 4, 2024
68dc97f
Nitpick: always use formatted text for state_key
AndrewFerr Sep 11, 2024
17890fd
Nitpick: remove trailing whitespace
AndrewFerr Sep 11, 2024
f962bf3
Change recommended room versions to apply on
AndrewFerr Sep 11, 2024
dd9b33e
Mention that _ can't be in any form of server name
AndrewFerr Sep 24, 2024
eb0eed6
Add issue of incompatibility with long MXIDs
AndrewFerr Sep 24, 2024
ac24510
Add issue of underscores in domain names
AndrewFerr Sep 24, 2024
9490cbd
Fix typo
AndrewFerr Sep 24, 2024
486b0cd
Use device ID suffix in location beacon example
AndrewFerr Sep 26, 2024
590ff96
Increase state key size limit & set suffix limit
AndrewFerr Sep 26, 2024
d9b149d
Keep original size limit on unprefixed state keys
AndrewFerr Sep 27, 2024
ae17437
Move paragraph to alternative section
AndrewFerr Sep 27, 2024
8222738
Add alternative of state key arrays
AndrewFerr Sep 27, 2024
63955d7
Add alternative of field for non-state events
AndrewFerr Sep 27, 2024
07d784a
Clarify state subkey/array relevance to user IDs
AndrewFerr Sep 27, 2024
99698ef
Fix formatting of auth rule's numeric list
AndrewFerr Sep 27, 2024
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
115 changes: 115 additions & 0 deletions proposals/3757-restricting-who-can-overwrite-a-state-event.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
# MSC3757: Restricting who can overwrite a state event.
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I think this is actually derestricting who can overwrite a state event.

Suggested change
# MSC3757: Restricting who can overwrite a state event.
# MSC3757: Relaxing the restrictions on who can overwrite a state event.

Copy link
Member

Choose a reason for hiding this comment

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

That's true for MSC3779, but not as much for this one; the former bypasses PL restrictions for setting state that belongs to the sender (where "belongs to" = the state key starts with the sender's MXID), but this MSC does not.

The restriction proposed by this MSC is to prevent state that belongs to a particular user from being overwritten by other, equally-powerful users.

The only PL restriction that's relaxed by this MSC is for allowing more powerful users to overwrite state that doesn't belong to them, for the sake of having a tool against state graffiti.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Still, I find the title a bit confusing. How about:

Suggested change
# MSC3757: Restricting who can overwrite a state event.
# MSC3757: Extending the set of write-protected state keys

or something

Copy link
Member

Choose a reason for hiding this comment

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

TBH I informally refer to this MSC as "the owned state MSC" despite that being the title of MSC3779 😉

Since one of the distinguishing differences of this MSC over 3779 is the ability for admins to manage others' state, maybe we could call it

Suggested change
# MSC3757: Restricting who can overwrite a state event.
# MSC3757: State event ownership (with admin management)

?


andybalaam marked this conversation as resolved.
Show resolved Hide resolved
## Problem

Currently there are three main ways to limit who can overwrite a state event:

* If a user's PL is greater than the `m.room.power_levels` `state_default` field
* If a user's PL is greater than the `m.room.power_levels` `events` field for that event type
* If a state event has a state key which begins with an `@`, then the sender's mxid must match that state key.
Comment on lines +5 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 find this a confusing way of phrasing the current restrictions.

I'd say that there are essentially two current restrictions, imposed by rules 7 and 8 in the auth rules.

In short:

  1. The user's PL must not be less than the "required power level" for that event.
  2. In addition, if a state event has a state key which begins with an @, then the sender's mxid must match that state key.

The spec for m.room.power_levels defines required power level, but in summary, it is determined by the setting in m.room.power_levels events field for that event type if present; otherwise, it is determined by the state_default field.

Copy link
Member

Choose a reason for hiding this comment

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

I find the @ prefix restriction a bit questionable, for the record. The only specified state event which uses it is m.room.member, which the auth rules validate state_key well before the restriction is checked. Non-spec usage of the state key already applies tricks like prefixing user IDs with _ to ensure they aren't hit by the restriction, and I'm not really convinced that location sharing/beacons need to pack a user ID into the state key anyways (see https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r1103877363 ).

This has me leaning towards removing the @ prefix restriction entirely, but I recognize that's not exactly a helpful opinion for this MSC to tackle. Creating stronger arguments for why the dependent features require the user ID in the state key I think would help, though.


This is problematic if an unprivileged user needs to publish multiple state
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
events of the same type in a room, but would like to set access control so
that only they can subsequently update the event. An example of this is if a
user wishes to publish multiple live location share beacons as per [MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489)
and [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672), for instance one per device. They will typically not want
other users in the room to be able to overwrite the state event,
so we need a mechanism to prevent other peers from doing so.

[MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489) originally proposed that the event type could be made variable,
appending an ID to each separately posted event so that each one could
separately be locked to the same mxid in the state_key. However, this is
problematic because you can't proactively refer to these event types in the
`events` field of the `m.room.power_levels` event to allow users to post
them - and they also are awkward for some client implementations to
manipulate.
AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved

## Proposal
andybalaam marked this conversation as resolved.
Show resolved Hide resolved

Therefore, we need a different way to state that a given state event may only
be written by its owner. **We propose that if a state event's state_key *starts with* a matrix ID (followed by an underscore), only the sender with that matrix ID (or higher PL users) can set the state event.** This is an extension of the current behaviour where state events may be overwritten only if the sender's mxid *exactly equals* the state_key.
turt2live marked this conversation as resolved.
Show resolved Hide resolved
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Proposal language should generally be assertive: "The state_key restrictions are changed to..." rather than "The state key should be fixed". This would also resolve a question about who 'we' is.

(please also limit lines to ~100 characters for easier review)

AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved

We also allow users with higher PL than the original sender to overwrite state
events even if their mxid doesn't match the event's state_key. This fixes an abuse
vector where an unprivileged user can immutably graffiti the state within a room
by sending state events whose state_key is their matrix ID.

Practically speaking, this means modifying the [authorization rules](https://spec.matrix.org/v1.2/rooms/v9/#authorization-rules) such that rule 8:

> If the event has a `state_key` that starts with an `@` and does not match the `sender`, reject.

becomes:

> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*.
AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Last time we changed the auth rules, there was a lot of discussion around "let's never do this again without test vectors for the auth rules" (cf matrix-org/matrix-spec#1710, matrix-org/matrix-spec#1642).

The apparent confusion over what the current auth rules mean, and what they should say after the change, makes me think that we should start some progress in that direction.

Copy link
Member

Choose a reason for hiding this comment

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

The Complement tests should be a step in the right direction.

AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved

For example, to post a live location sharing beacon from [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672):

```json=
{
"type": "m.beacon_info",
"state_key": "@stefan:matrix.org_assetid1", // Ensures only the sender can update
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
"content": {
"m.beacon_info": {
"description": "Stefan's live location",
"timeout": 600000,
"live": true
},
"m.ts": 1436829458432,
"m.asset": {
"type": "m.self.live"
}
}
}
```

Since `:` is not permitted in the localpart and `_` is not permitted in the domain part of an mxid (see [Historical User IDs](https://spec.matrix.org/v1.2/appendices/#historical-user-ids)), it is not possible to craft an mxid that matches the beginning of a state key constructed for another user's mxid, so state keys restricted to one owner can never be overwritten by another user.
Copy link
Contributor

Choose a reason for hiding this comment

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

While the spec specifies a grammar for server names, are we SURE that servers verify that grammar? While officially underscores are invalid in domain names and you can't order a TLS certificate for it nowadays, such domains still exist in the wild and wildcard certificates even allow using TLS with them. For example: https://my_sarisari_store.typepad.com/

I think relying on underscores as a separator is a rather scary trap and I wouldn't bet on all currently developed Matrix servers rejecting such server names.

Copy link
Member

Choose a reason for hiding this comment

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

ac24510 adds this to the "Potential issues" section.

Copy link
Member

Choose a reason for hiding this comment

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

How about using @ as the separator? Since the separator will never be the first character in the state key, it shouldn't be possible to confuse it with the user ID sigil.

Copy link
Member

Choose a reason for hiding this comment

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

| could also be a good separator.


## Potential issues

None yet.
Copy link
Member

Choose a reason for hiding this comment

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

I remain concerned that this MSC opens the door to a proliferation of state events that we cannot delete, and that faster-joins can't help with. I would like to see some discussion of this in the MSC.

Admittedly, we have this problem to some extent today, in that there is nothing stopping users creating lots of state events with different types, but:

  • (a) such behaviour is currently restricted to room moderators, because it relies on their power level being state_default or higher. This MSC would change that, by allowing any user to (say) create millions of m.beacon_info events.
  • (b) there is currently no specced usecase that requires multiple state events per user, and introducing one feels like a step-change. Arguably that is not a problem of this MSC per se (and rather MSCs that make use of it such as MSC3672), but I think we'll need to at least set some expectations here.

This is not a veto from me, but I do think we need to consider this properly and at least set out guidelines for future MSCs that build on this.

Copy link
Member

Choose a reason for hiding this comment

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

  • (a) ... This MSC would change that, by allowing any user to (say) create millions of m.beacon_info events.

This is what MSC3779 proposes, not this one.

To clarify, the effective auth rule changes proposed by this MSC are to:

  • grant write access on state events with state keys of the form <sender>_<anySuffix> to all users
    • N.B. <sender> is the MXID of the user sending the state event
  • grant write access on state events with state keys of the form <mxid> & <mxid>_<anySuffix> to users whose PL is greater than that of <mxid>'s PL
    • N.B. <mxid> is any MXID

The event-sending restrictions imposed by m.room.power_levels remain in effect.

What this MSC does allow is creating lots of state events with different state key suffixes -- but that would be limited to room moderators, just like how creating lots of state events with different types currently is.

  • (b) there is currently no specced usecase that requires multiple state events per user, and introducing one feels like a step-change.

As a start, MSC4143 makes an explicit mention to this one (namely, in the section regarding MatrixRTC room state), and is what has driven interest in this MSC.

AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved

## Alternatives
AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved

As originally proposed in [MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489) and [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672), we can require
the use of a state key equal to the sender's mxid, but this means we can only
have one such event of each type, so those MSCs proposed using different types
for each unique event.

An earlier draft of this MSC proposed putting a flag on the contents of the
event (outside of the E2EE payload) called `m.peer_unwritable: true` to indicate
if other users were prohibited from overwriting the event or not. However, this
unravelled when it became clear that there wasn't a good value for the state_key,
which needs to be unique and not subject to races from other malicious users.
By scoping who can set the state_key to be the mxid of the sender, this problem
goes away.

Another option would be to include the `sender` (or a dedicated `state_subkey`)
as the third component of what makes a state event unique. This is, however, a
much bigger change and not justifiable for the goal meant to be reached with
this MSC.

## Security considerations

As this changes auth rules, we should think carefully about whether could
introduce an attack on state resolution. For instance: if a user had higher
PL at some point in the past, will they be able to abuse somehow this to
overwrite the state event, despite not being its owner?

When using a state_key prefix to restrict who can write the event, we have
deliberately chosen an underscore to terminate the mxid prefix, as underscores
are not allowed in domain names. A pure prefix match will **not** be sufficient,
as `@matthew:matrix.org` will match a state_key of form `@matthew:matrix.org.evil.com:id1`.

This changes auth rules in a backwards incompatible way, which will break any
use cases which assume that higher PL users cannot overwrite state events whose
state_key is a different mxid. This is considered a feature rather than a bug,
andybalaam marked this conversation as resolved.
Show resolved Hide resolved
fixing an abuse vector where unprivileged users could send arbitrary state events
which could never be overwritten.

andybalaam marked this conversation as resolved.
Show resolved Hide resolved
## Unstable prefix

None
andybalaam marked this conversation as resolved.
Show resolved Hide resolved

## Dependencies

None