Synapse is overly accepting of content in the unsigned object in events received over federation

[richvdh]

Synapse makes use of various properties within the unsigned object of events - either internally, or by passing them on to clients. One example is replaces_state, which is used to store the event id of the previous event with the same type and state_key, and is later used to populate the prev_content property for events served to clients.

The problem is that homeservers are free to populate unsigned, without it affecting the event hashes or signatures; a malicious or buggy homeserver could therefore populate the content with incorrect data.

Taking the example of replaces_state, Synapse overwrites this property when receiving an event, but only if there was previously an event with the same type and state_key in the room state; it is otherwise passed through unchanged. So, a malicious homeserver could confuse remote servers' clients by sending incorrect values of replaces_state over federation.

---

The specification is not clear on how unspecified properties within unsigned should be handled, but I think they should be stripped off by the receiving homeserver. This will ensure that if, in future, the C-S API spec is extended to specify new properties be added to unsigned, there will be no confusion about whether they were added by the local or remote homeserver.

As far as I am aware, the only properties that should be allowed in unsigned over federation are:

• invite_room_state
• knock_room_state
• age - though see also age logic for events is broken #8429.

[Aside: in an ideal world, we might have different properties for "things added by the remote homeserver - treat with caution!" vs "things added by the local homeserver - can be trusted". However, that ship has probably sailed for now.]

[ShadowJonathan]

Couldn't we add a "remote" key under unsigned that logs all of the properties received directly(!) over federation in the spec? That way, clients can pick and choose what they trust andsoforth.

I say "directly", as i think that if remote keys embedded in forwarded events will lead to disaster, maybe a security issue, and probably a recursion amplification and such.

[richvdh]

Couldn't we add a "remote" key under unsigned

I'm failing to understand you. You're suggesting we move invite_room_state and friends to a different key? Well, as noted, yes we could, but that would be an annoying breaking change.

[richvdh]

giving this a security label, as I think it could be abused by malicious HS impls.

[callahad]

@richvdh Can you clarify why you added this back to the list for discussion by the team? Suspect it has to do with matrix-org/matrix-spec-proposals#3522 merging.

Is the anticipated resolution:

• Stop Synapse from returning replaces_state (or any other unsigned property) over federation.
• Strip all unsigned values arriving over federation, except for an allowlist of invite_room_state, knock_room_state, and age

[richvdh]

I don't think it ever got discussed by the team, so it's not really adding it back to the list.

Given its security implications, I think that is worthwhile considering how we prioritise this.

Is the anticipated resolution:

• Stop Synapse from returning replaces_state (or any other unsigned property) over federation.
• Strip all unsigned values arriving over federation, except for an allowlist of invite_room_state, knock_room_state, and age

Just the latter is necessary, I think. (And I'd only allow invite_room_state and knock_room_state for event types that are meant to have it).