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 10 commits into
base: main
Choose a base branch
from
116 changes: 116 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,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
andybalaam marked this conversation as resolved.
Show resolved Hide resolved

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.

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.

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

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*.
Copy link
Member

Choose a reason for hiding this comment

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

To overwrite another user's state event, must your power level exceed the event type's PL, or the PL of the user who owns the event?

i.e. should this be reworded to the following:

Suggested change
> 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*.
> 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 powerlevel of the user whose matrix ID is the prefix of the `state_key`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To overwrite another user's state event, must your power level exceed the event type's PL, or the PL of the user who owns the event?

I was thinking of the event type's power level i.e. we're not changing anything here except expanding the existing exception where a user can create a state event beyond their power level if they use a specific state key.

Could the wording be improved to clarify 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 user can create a state event beyond their power level if they use a specific state key

This isn't mentioned anywhere, at least not explicitly. If the only intended change to the auth rules is the change to step 8, then the preceding step still says to reject if the sender's PL is less than the event type’s required power level.

For the exception to hold, step 7 could simply be removed, meaning the PL restriction would be applied by the proposed change to rule 8.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, no -- that would remove PL restrictions for non-state events.

How about replacing steps 7 and 8 with this:

  • If the event type’s required power level is greater than the sender’s power level, and the event does not have a state_key matching the sender optionally followed by a substring prefixed with _, reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hang on, I was getting confused with #3779 - sorry for the confusion!

I think this MSC restricts who can overwrite certain state events, and I agree that the wording as it is now doesn't make sense, because it actually allows everyone to overwrite, because we won't reach rule 8 without first passing rule 7 which already checks power level.

I think I just prefer #3779 over this one, and if I understand right, we don't need both.


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
"content": {
"m.beacon_info": {
"description": "Stefan's live location",
"timeout": 600000,
"live": true
},
"m.ts": 1436829458432,
"m.asset": {
"type": "m.self"
}
}
}
```

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.

## Potential issues

None yet.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

String-based matching still feels like a bandaid solution for a larger problem. The problem statement is reasonable (beacons, calls, and integrations all want an ability to narrow power levels), however there are non-state events where it'd be worth having a similar access mechanism (namely for edits).

At the point where we're considering string-packing, a top level field on the event works just as well and is more extensible, capable, and presentable than a packed string. It can be populated with query parameters to the PUT /state or PUT /send calls, and servers are easily able to retrieve information from the event. Further, since we're modifying the auth rules to accomplish this, there is no material difference to the implementation effort of the proposal itself.

Copy link

Choose a reason for hiding this comment

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

I am wondering if a top level field implies that state_key and access mechanism are independent.

If this is what is proposed I have the concern that this would allow blocking of state keys by not allowing to write to them anymore. (I think I saw this being brought up somewhere else as well. cannot find it right now)

But the concept of merging key + access level makes a lot of sense to me since it would behave more like a namespace than an add-on permission system. And I think the idea of namespaces (kind of like inverse domains where each user owns the domain starting with their username) is a very good fit for a key value based state like the room state.

Copy link
Contributor Author

@andybalaam andybalaam Jun 14, 2024

Choose a reason for hiding this comment

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

(edited out to avoid confusion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, the above was wrong - I was mixing this up with #3779 which I think replaces this, and which I prefer.


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.

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

## Dependencies

None