-
Notifications
You must be signed in to change notification settings - Fork 411
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 all 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,210 @@ | ||||||||||||||
# 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 two main restrictions on who can overwrite a state event, enforced by rules | ||||||||||||||
7 and 8 of the [authorization rules](https://spec.matrix.org/latest/rooms/v11/#authorization-rules): | ||||||||||||||
|
||||||||||||||
* Only users with a power level greater than or equal to the "required power level" for a state | ||||||||||||||
event type may send state events of that type. | ||||||||||||||
* State events with a `state_key` that equals a user ID may be overwritten only by the user whose | ||||||||||||||
ID matches the state key. | ||||||||||||||
|
||||||||||||||
With these restrictions, only a single piece of state for any state event type may have its write | ||||||||||||||
access limited to a particular user (the state event whose `state_key` is set to the ID of the user | ||||||||||||||
who has write access to it). | ||||||||||||||
|
||||||||||||||
This is problematic if a user needs to publish multiple state | ||||||||||||||
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 participate in VoIP call as per [MatrixRTC (MSC4143)]( | ||||||||||||||
https://github.com/matrix-org/matrix-spec-proposals/blob/toger5/matrixRTC/proposals/4143-matrix-rtc.md#the-matrixrtc-room-state), | ||||||||||||||
by sending a state event for each of their devices that is to participate in the call. | ||||||||||||||
They will typically not want other users in the room to be able to overwrite those state events, | ||||||||||||||
so there ought to be a mechanism to prevent other users from doing so. | ||||||||||||||
Comment on lines
+17
to
+24
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. To be honest, it's not clear to me that MatrixRTC call events should be state events. Call state is very ephemeral information, while room state is generally meant for longer-lived state, like room configuration. Areas where room state did end up being used for quicker evolving information are precisely those that showcase many problems with such an approach: the need to retain stricter ownership over a subset of state events; the need to allow less privileged users to send state events, which brings about state spam concerns; the large volume of events calling for a mechanism to delete state, which turns out to be an almost impossible feat. Not to mention that state events in today's Matrix are unencrypted. It all screams bad fit to me. I realise this is coming a bit late to the party given when MatrixRTC implementations are already running in the wild. But given that this is used as the sole example of a requirement for this MSC and there is widespread dissatisfaction with string packing, I think it needs to be raised. 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 second this. State events is the wrong place for this data, and by putting it in this wrong place we are creating a rod for our own back. String packing is, in the words of other people, a horrible hack. Does it work? Undeniably yes. Do we want to bake this into the protocol permanently? No. This is precisely the job of the SCT to block this kind of thing imo. Now this does beg the question: what is the right place then? I've banged on internally that there isn't really a right place for this kind of data currently, and have been spending my downtime slowly crafting a proposal for an alternative. Effectively, all we want is:
Thus sending a state event only you can modify becomes a persistent EDU (quite an oxymoron given the E stands for Ephemeral!) with whatever custom type/content you want. The main driver for this MSC afaict is MatrixRTC so we can even drop the "custom" part for now and only have fixed known EDUs for now to reduce the scope and risk of the changes. I'd be much happier to thumbs up a persistent EDU proposal than shoehorn in owned state events. 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.
Agreed. Apart from the ephemerality of the data, another thing that makes it different to room state is that there is never any real reason to do state resolution. We only ever care about the "current" state, and (unlike regular state) there is a single server that can and should be trusted to be authoritative. 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.
#4354 is this. |
||||||||||||||
|
||||||||||||||
## 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. 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 there’s general agreement that string packing is not a great long-term idea. This is not only a topic in the context of this MSC but also in the context of MSC4143. The reason it’s being considered is that it’s an pragmatic change — lightweight for SDK maintainers to adopt and gives already (amongst others) the nice feature of pragmatic name-spacing which is a powerful property. In contrast a big-bang change is disruptive and complex. Probably leading to a situation where nobody currently has time to implement it. Hence, I am proposing deliver value without disruption (=using string packing) over disruptive and complex 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. Despite frequent appearances to the contrary, matrix is intended to be a production-quality protocol, not some finished-over-a-weekend demo. You don't do "pragmatic" hacks in such a project, especially not for a new feature. If the feature requires "complex and disruptive" changes, so be it - this PR has been sitting around for three and a half years now, it can wait a couple more until those changes are worked out. 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. Right now, I am minded that the problems with this proposal (fundamentally: it's a hack that we don't want to have to support in the long term) outweigh the advantages of landing it swiftly. |
||||||||||||||
|
||||||||||||||
In a future room version, | ||||||||||||||
**if a state event's `state_key` *starts with* a user ID followed by an underscore, only the user | ||||||||||||||
with that ID or users with a higher power level then them may overwrite that state event.** | ||||||||||||||
This is an extension of the current behaviour where state events may be overwritten only by users | ||||||||||||||
whose ID *exactly equals* the `state_key`. | ||||||||||||||
|
||||||||||||||
As the spec currently enforces [a size limit of 255 bytes for both user IDs and state keys]( | ||||||||||||||
https://spec.matrix.org/unstable/client-server-api/#size-limits), | ||||||||||||||
the size limit on state keys is increased to **511 bytes** to allow prefixing any currently-valid | ||||||||||||||
state key with a maximum-length user ID (and a separator character). | ||||||||||||||
The size of a state key suffix after a leading user ID and the separator character is limited to | ||||||||||||||
**255 bytes** so that any such suffix may follow any user ID without the complete state key | ||||||||||||||
ever surpassing the total state key size limit. | ||||||||||||||
Similarly, the size of a state key without a leading user ID is limited to **255 bytes** so that any | ||||||||||||||
state key without a leading user ID may be given one without ever surpassing the total size limit. | ||||||||||||||
Comment on lines
+38
to
+42
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. Is this conversion between prefixed and unprefixed state keys a real requirement? It seems to lead to a lot of complexity. If we're doing this, I'd be inclined to just increase max state key length to (say) 512 bytes, and be done with it. 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 I was thrown off a bit by the explanation involving currently-valid state keys and thinking that this implied wanting to user-namespace currently state keys. Really the point is simply that, with a maximum length user ID, you need some space to fit something on the end, so yeah, I'd say just keep it simple & double state key length.
Comment on lines
+34
to
+42
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. This part can most charitably be described as an awful hack. There should never be a reason to encode multiple independent values into a single string field inside a structured data format like JSON, let alone then enforcing strict length limits on different parts. No, this is terrible, the section "Multi-component state keys" describes what a real solution would have to look like. |
||||||||||||||
|
||||||||||||||
Users with a higher power level than a state event's original sender may overwrite the event | ||||||||||||||
despite their user ID not matching the one in event's `state_key`. This fixes an abuse | ||||||||||||||
vector where a user can immutably graffiti the state within a room | ||||||||||||||
by sending state events whose `state_key` is their user ID. | ||||||||||||||
Comment on lines
+44
to
+47
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'd say there's a strong argument for making this a separate MSC, rather than trying to fix two things at once. |
||||||||||||||
|
||||||||||||||
Practically speaking, this means modifying the | ||||||||||||||
[authorization rules](https://spec.matrix.org/v1.2/rooms/v9/#authorization-rules) such that rule 8: | ||||||||||||||
|
||||||||||||||
> 8. If the event has a `state_key` that starts with an `@` and does not match the `sender`, reject. | ||||||||||||||
|
||||||||||||||
becomes: | ||||||||||||||
|
||||||||||||||
> 8. If the event has a `state_key`: | ||||||||||||||
> 1. If the `state_key` starts with an `@`: | ||||||||||||||
> 1. If the prefix of the `state_key` before the first `_` that follows the first `:` (or end | ||||||||||||||
> of string) is not a valid user ID, reject. | ||||||||||||||
> 1. If the size of the `state_key` without its leading user ID is greater than 256 bytes, reject. | ||||||||||||||
> 1. If the leading user ID does not match the `sender`, and the `sender`'s power level is not | ||||||||||||||
> greater than that of the user denoted by that ID, reject. | ||||||||||||||
> 1. Otherwise, if size the `state_key` is greater than 255 bytes, reject. | ||||||||||||||
|
||||||||||||||
Note that the size limit of 256 bytes after a leading user ID includes the separating `_`. | ||||||||||||||
|
||||||||||||||
No additional restrictions are made about the content of the `state_key`, so any characters that | ||||||||||||||
follow the `sender` + `_` part are only required to be valid for use in a `state_key`. | ||||||||||||||
|
||||||||||||||
For example, to post a live location sharing beacon from | ||||||||||||||
[MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672) | ||||||||||||||
for one of a user's devices: | ||||||||||||||
|
||||||||||||||
```json= | ||||||||||||||
{ | ||||||||||||||
"type": "m.beacon_info", | ||||||||||||||
"state_key": "@stefan:matrix.org_{deviceid1}", // Ensures only the sender or higher PL users can update | ||||||||||||||
"content": { | ||||||||||||||
"m.beacon_info": { | ||||||||||||||
"description": "Stefan's live location", | ||||||||||||||
"timeout": 600000, | ||||||||||||||
"live": true | ||||||||||||||
}, | ||||||||||||||
"m.ts": 1436829458432, | ||||||||||||||
"m.asset": { | ||||||||||||||
"type": "m.self" | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
``` | ||||||||||||||
|
||||||||||||||
## Potential issues | ||||||||||||||
|
||||||||||||||
### Incompatibility with domain names containing underscores | ||||||||||||||
|
||||||||||||||
Although both [the spec](https://spec.matrix.org/unstable/appendices/#server-name) | ||||||||||||||
and [RFC 1035 §2.3.1](https://www.rfc-editor.org/rfc/rfc1035#section-2.3.1) | ||||||||||||||
forbid the presence of underscores in domain names, | ||||||||||||||
there noneless exist resolvable domain names that contain underscores. | ||||||||||||||
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.
Suggested change
|
||||||||||||||
The proposed auth rule for parsing a leading user ID from an underscore-separated state key would | ||||||||||||||
fail on such domain names. | ||||||||||||||
|
||||||||||||||
Possible solutions include: | ||||||||||||||
- using a different character to terminate a leading user ID in state keys. That character must be | ||||||||||||||
one known to be absent from domain names in practice, and must also not be any character that | ||||||||||||||
the spec allows to appear in a server name. | ||||||||||||||
- refining the proposed auth rule for parsing a leading user ID such that it does not fail on domain | ||||||||||||||
names that contain an underscore. One way to achieve this is to leverage the absence of | ||||||||||||||
underscores from top-level domains. | ||||||||||||||
|
||||||||||||||
## Alternatives | ||||||||||||||
AndrewFerr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
### Variable event types | ||||||||||||||
|
||||||||||||||
[MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489) | ||||||||||||||
and [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672) | ||||||||||||||
originally proposed that the event type could be made variable, | ||||||||||||||
with an ID appended to each separately posted event so that each one could | ||||||||||||||
separately be locked to the same user ID 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. | ||||||||||||||
|
||||||||||||||
### Event ownership flag | ||||||||||||||
|
||||||||||||||
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 | ||||||||||||||
signify ownership of the containing event by its `sender`, which would 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 user ID of the sender, this problem | ||||||||||||||
goes away. | ||||||||||||||
|
||||||||||||||
One way to satisfy the need for unique and non-racing state keys with an event ownership flag | ||||||||||||||
is to key state events by not only their event type and `state_key`, but also their `sender` | ||||||||||||||
when the event ownership flag is set. | ||||||||||||||
This would set a flagged event's owner implicitly from whoever sent the event, | ||||||||||||||
instead of from an explicit field set in the event. | ||||||||||||||
To support this, server implementations would need to change how they key state events, and | ||||||||||||||
the endpoint for retrieving state events would need to allow specifying the owner of the event to | ||||||||||||||
retrieve (or no owner to retrieve un-owned state). | ||||||||||||||
Additionally, the endpoint for setting state events may support a query parameter to specify | ||||||||||||||
which user's state to overwrite, which would work only for senders with a power level higher than | ||||||||||||||
that of the targeted user. | ||||||||||||||
Otherwise, administration of owned events would be limited to redacting them. | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
### Multi-component state keys | ||||||||||||||
|
||||||||||||||
[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. | ||||||||||||||
As an extension to this idea, a comment in [the discussion of this MSC]( | ||||||||||||||
https://github.com/matrix-org/matrix-spec-proposals/pull/3757#issuecomment-2099010555) | ||||||||||||||
proposes allowing `state_key` to be an array of strings. | ||||||||||||||
With either proposal of multi-component state keys, state events could be scoped to an owning user | ||||||||||||||
by setting one of the components of the state key (either the `state_subkey` or an element of an | ||||||||||||||
array `state_key`) to a user ID, instead of by prefixing the `state_key` string with a user ID. | ||||||||||||||
Doing so would avoid having to parse user IDs out of `state_key` strings, | ||||||||||||||
and would avoid needing to increase the size limit of the `state_key` field to give it enough room | ||||||||||||||
to contain a leading user ID. | ||||||||||||||
However, allowing state keys to be multi-component would change state key comparison from being a | ||||||||||||||
string comparison to an array-of-strings comparison, which could be costly for existing server | ||||||||||||||
implementations to migrate to. | ||||||||||||||
|
||||||||||||||
### Owning user ID field | ||||||||||||||
|
||||||||||||||
A comment in [the discussion of this MSC]( | ||||||||||||||
https://github.com/matrix-org/matrix-spec-proposals/pull/3757#discussion_r1103877363) | ||||||||||||||
proposes an optional top-level field for both state and non-state events that designates ownership | ||||||||||||||
of the containing event to a particular user. | ||||||||||||||
This would provide ownership semantics for not only state events, but also message events, which may | ||||||||||||||
be used to restrict event replacements / redactions to only the designated owner of an event. | ||||||||||||||
However, it remains to be decided how using this top-level field for state events should affect | ||||||||||||||
state resolution; namely, whether it is possible to set multiple events with the same `state_key` | ||||||||||||||
but different owners. | ||||||||||||||
|
||||||||||||||
## 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, the possibility of new attacks on state resolution must be considered. | ||||||||||||||
For instance, if a user had higher power level at some point in the past, will they be able to | ||||||||||||||
somehow abuse this to overwrite the state event, despite not being its owner? | ||||||||||||||
|
||||||||||||||
When using a leading user ID in a `state_key` to restrict who can write the event, the character to | ||||||||||||||
terminate the leading user ID was deliberately chosen to be an underscore, as it is not | ||||||||||||||
allowed in [any form of server name](https://spec.matrix.org/v1.11/appendices/#server-name) | ||||||||||||||
(either a DNS name or IPv4/6 address, with or without a numeric port specifier) and thus cannot be | ||||||||||||||
confused as part of the server name of a leading user ID (with one caveat mentioned as a | ||||||||||||||
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. But above you've said that resolvable domain names with underscores exist: wouldn't this allow the attack you talk about below? |
||||||||||||||
[potential issue](#incompatibility-with-domain-names-containing-underscores)). | ||||||||||||||
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 power level users cannot overwrite state events whose | ||||||||||||||
`state_key` is a different user ID. This is considered a feature rather than a bug, | ||||||||||||||
fixing an abuse vector where 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 10 or higher as `org.matrix.msc3757`. | ||||||||||||||
|
||||||||||||||
## Dependencies | ||||||||||||||
|
||||||||||||||
None |
Uh oh!
There was an error while loading. Please reload this page.