-
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 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. | ||||||||||||||
|
||||||||||||||
## 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. |
||||||||||||||
|
||||||||||||||
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. | ||||||||||||||
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.