MSC3946: Dynamic room predecessor

[MadLittleMods]

A proposal for specifying a room's predecessor after the fact.

Rendered

Client implementations:

• Element Web: PRs linked from this epic

[MadLittleMods]

predecessor_room_id is a bit wordy but plainly describes what it is and what the value should be 🤷

[turt2live]

I'm a bit worried this MSC introduces higher risk of security issues in clients regarding room takeovers. Currently, the relationship between the previous and current room can be verified (barring state resets), giving the client confidence that room B is in fact the most recent edition of room A. With the relationship being mutable, clients are likely to not check the predecessor correctly and therefore allow someone to hide/take over non-tombstoned rooms.

The current relationship of rooms is very linear, and though this MSC says the mutable event takes precedence, not all clients will have updated to consider that point. This can lead to inconsistencies in what users see, or abusive scenarios where rooms are hidden from some people and not others.

[dkasak]

(I think we race conditioned a bit and share more-or-less the same concern, just worded a bit differently.)

[turt2live]

I'm slightly more concerned about clients getting the implementation wrong, despite warnings in the spec, though yes: the root issue is "mutable is scary", I think.

[MadLittleMods]

Currently, the relationship between the previous and current room can be verified (baring state resets), giving the client confidence that room B is in fact the most recent edition of room A.

There is nothing "verifiable" about the m.room.create predecessor value besides that it came from someone with proper power levels. A Matrix room can be created with whatever predecessor desired (even multiple rooms with the same predecessor). And same for the tombstone from some old room pointing to a new room. And as a note in case onlookers aren't as familiar, tombstones are already mutable.

To me, the chain of tombstone and predecessor are mere suggestions of where you can continue looking at history in the room. I'm not quite following the line of thought to get the severity with danger in any of this.

All of these problems like selectively sharing the state only to certain servers or state resets seem completely separate to this MSC. If we can't trust the state in a room, then everything falls apart.

[turt2live]

The presence of a tombstone pointing to the new room and a predecessor pointing to the tombstoned room forms a verifiable relationship - this was specifically designed in for room upgrades.

With this MSC, we'd be breaking that relationship we previously went out of our way to make verifiable - this is where the concern is coming from. If we're breaking the verifiable nature of the relationship, it needs to be for justifiable/secure reasons.

We already acknowledge that state resets and similar can cause the relationship to be broken, however the expectation in those cases is that clients actually split the room into two logical rooms again, even if half the relationship still exists. With both halves of the relationship, the client can merge the rooms together safely.

[turt2live]

How is this a takeover? Above all, this is just a client display thing. And second, it looks like one room needs the tombstone and other one needs a predecessor for things to look replaced. Both sides acknowledged each other for this to happen. Anything other than this seems like a bad assumption on the clients part.

The case I'm concerned about isn't when both rooms point at each other, but rather when a client developer inevitably makes a mistake in the implementation of this MSC. There's already significant risk of clients getting the implementation wrong, and making the predecessor dynamic opens the door wider for mistakes (despite warnings in the spec, which often go unheeded). This would result in a malicious room being allowed to overwrite/hide another room it shouldn't have right to, thus takeover.

[MadLittleMods]

This would result in a malicious room being allowed to overwrite/hide another room it shouldn't have right to, thus takeover.

This seems like it would be a client doing something very wrong. We can't help a client that doesn't look at both tombstone and predecessor to see that they match. That's a bare minimum even with the current state of tombstone/predecessor if you're making rooms look replaced. A client can show the pointer forward/backward without this verification though.

Even a naive client implementation that only looks at the create event for the predecessor is safe in the new world that this MSC creates.

If a client implementation updates to look at m.room.predecessor, then that is also pretty fool-proof regardless of which era of state they look at. The only tricky thing I could think of is if we also lived in a MSC3779 world where someone doesn't check for the canoncial version with state_key: "" but that's not a concern yet (and is a general problem).

[turt2live]

I agree that the client would be doing something wrong, however the risk of a client doing something wrong is higher with an MSC like this (in its current form) that overall leaves an impression that the protocol is insecure, even if the blame is actually with the clients.

Unfortunately I don't recall all of the details, but there was also a reason why we embedded the predecessor into the create event instead of a dedicated state event: a dedicated state event would have been cleaner and "more proper" as far as Matrix is concerned, but we landed on putting it in the create event for a reason. Digging up that rationale would likely help this conversation, as it keeps coming to mind, though I sadly do not have the bandwidth to do that research.

[MadLittleMods]

I agree that the client would be doing something wrong, however the risk of a client doing something wrong is higher with an MSC like this (in its current form) that overall leaves an impression that the protocol is insecure, even if the blame is actually with the clients.

I don't think this MSC increases the risk level. We have the exact same risk with the current tombstone/predecessor state of things.

And to address that one aspect, it sounds we should consider another MSC to verify this server-side so clients can more easily rely on eating whatever we feed it.

Unfortunately I don't recall all of the details, but there was also a reason why we embedded the predecessor into the create event instead of a dedicated state event: a dedicated state event would have been cleaner and "more proper" as far as Matrix is concerned, but we landed on putting it in the create event for a reason. Digging up that rationale would likely help this conversation, as it keeps coming to mind, though I sadly do not have the bandwidth to do that research.

This would be interesting to find. We should start a new thread if we come up with anything here.

[andybalaam]

One datapoint around verifiability: I found a bug in matrix-js-sdk's code for verifying links: matrix-org/matrix-js-sdk#3089 . The fact that this has remained undiscovered for a long time may imply that people are not paying a great deal of attention to verifiability.

[andybalaam]

@MadLittleMods did you consider including event_id like in the create event? Some code in matrix-react-sdk expects to be able to have that. I'm not sure yet what the implications would be of not providing it when looking for a room's predecessor. (I imagine we can work around it, but I haven't looked into it yet)

[MadLittleMods]

did you consider including event_id like in the create event?

Didn't know about it so no consideration was given to it so far. For my own reference: https://spec.matrix.org/v1.5/client-server-api/#mroomcreate

event_id (Required): The event ID of the last known event in the old room.
room_id (Required): The ID of the old room.

Adding an optional last_known_event_id field seems fine 👍

I assume it's not related to the upgrade verification part at all since we should be looking at the latest state in the room. But this sort of info could be nice as one option for a continuation point to jump to in the old room.

It terms of workaround fallbacks, we can just go to where the latest tombstone in the old room is or just the latest in the old room. One more complicated option is to use the /timestamp_to_event endpoint to find the latest event from when the predecessor was sent but doesn't seem necessary or the right thing to do exactly.

[MadLittleMods]

So I understand the use cases, are my assumptions above correct? Are there other use cases? Is this useful?

[t3chguy]

IMO this should be via for consistency with already specced shapes, e.g. m.space.child. https://spec.matrix.org/v1.5/client-server-api/#mspacechild

[MadLittleMods]

There is also /join/{roomIdOrAlias}?server_name=foo.bar.

I can update this to be whatever name we want to start being consistent about. Thanks for that link, https://spec.matrix.org/v1.5/appendices/#routing is also an interesting reference.

I chose via_servers because it seemed like the best plainly readable thing and pretty much self-explains what the content should be.