-
Notifications
You must be signed in to change notification settings - Fork 401
MSC3757: Restricting who can overwrite a state event #3757
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
ff5fd48
610f244
bfde329
344e876
ccb7e52
1ce7e0e
6df6109
6e108b3
bd4176f
e352a1d
5e95ff3
68dc97f
17890fd
f962bf3
dd9b33e
eb0eed6
ac24510
9490cbd
486b0cd
590ff96
d9b149d
ae17437
8222738
63955d7
07d784a
99698ef
9f4f31a
75f03da
e833e8a
a4b40b5
a0da59b
5855a7f
deba3b8
8090f69
3a0d095
e16482a
1ddddb6
fd87b8a
2a77266
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,116 @@ | ||||||||||||||
# 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
or something There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revisiting this a few months later (sorry), I still find my suggestion clearer. |
||||||||||||||
|
||||||||||||||
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. | ||||||||||||||
AndrewFerr marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the This has me leaning towards removing the |
||||||||||||||
|
||||||||||||||
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 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My general concern is that we already have JSON available to us, so we should use that. String packing works in areas where we don't have as fine control (voip call candidates, for example), but for something like this we can and should afford fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the SCT came down on the side of this proposal vs MSC3760 I consider this resolved. Please unresolve if you disagree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure where the conversation about the SCT preferring this over MSC3760 happened, but I'm strongly against the current proposal. String packing is going to cause bugs, having separate fields seems better in every sense. Hence my concern I've left about insufficient alternatives being explored. |
||||||||||||||
|
||||||||||||||
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
AndrewFerr marked this conversation as resolved.
Show resolved
Hide resolved
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 or higher PL users can update | ||||||||||||||
kegsay 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. | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ac24510 adds this to the "Potential issues" section. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
|
||||||||||||||
## Potential issues | ||||||||||||||
|
||||||||||||||
None yet. | ||||||||||||||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
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. | ||||||||||||||
|
||||||||||||||
[MSC3760](https://github.com/matrix-org/matrix-spec-proposals/pull/3760) | ||||||||||||||
proposes to include a dedicated `state_subkey` as the third component of what | ||||||||||||||
makes a state event unique. | ||||||||||||||
|
||||||||||||||
## Security considerations | ||||||||||||||
|
||||||||||||||
This change requires a new room version, so will not affect old events. | ||||||||||||||
AndrewFerr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
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 | ||||||||||||||
|
||||||||||||||
While this MSC is not considered stable, implementations should apply the behaviours of this MSC on top of room version 9 as `org.matrix.msc3757`. | ||||||||||||||
AndrewFerr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
## Dependencies | ||||||||||||||
|
||||||||||||||
None |
Uh oh!
There was an error while loading. Please reload this page.