matrix-org / matrix-spec

The Matrix protocol specification
Apache License 2.0
194 stars 96 forks source link

Multiple "Canonical JSON" definitions not distinct in spec #1247

Open neilalexander opened 2 years ago

neilalexander commented 2 years ago

At the moment the spec has one definition for "Canonical JSON" in the appendices. In reality there are multiple places that we care about signing JSON, including but possibly not limited to:

It isn't clear precisely which rules apply to which — did we at some point expect that the post-v6 change to strict canonical JSON enforcement should also apply to E2EE and federation request signing? Presumably we can't change federation request body signing if we still need to be able to send events for old rooms?

Need better clarity on precisely which behaviours to apply where (i.e. #1244, #1245).

(created from #1232)

richvdh commented 2 years ago

I think that the only place that the One True Definition in the appendices (https://spec.matrix.org/v1.3/appendices/#canonical-json) doesn't apply is in the signature for events for room versions 5 or earlier (and in fairness, the appendix does say that, though is vague about the exact differences).

did we at some point expect that the post-v6 change to strict canonical JSON enforcement should also apply to E2EE and federation request signing?

E2EE and Federation requests should always have followed the strict canonical JSON: any instance where they did not was a bug. The differences with events are:

Presumably we can't change federation request body signing if we still need to be able to send events for old rooms?

Certainly we can. There is nothing that obliges us to be able to send events containing large numbers etc, even in old rooms. We can, and do, therefore enforce strict canonical JSON even on /send requests containing events in old rooms. (This does cause a problem if you attempt to do something like update a state event which contains a large number. IIRC Synapse rejects such attempts at the CS API.)

So... I think this is basically a duplicate of #1244?

neilalexander commented 2 years ago

E2EE and Federation requests should always have followed the strict canonical JSON: any instance where they did not was a bug.

Certainly we can. There is nothing that obliges us to be able to send events containing large numbers etc, even in old rooms. We can, and do, therefore enforce strict canonical JSON even on /send requests containing events in old rooms. (This does cause a problem if you attempt to do something like update a state event which contains a large number. IIRC Synapse rejects such attempts at the CS API.)

The CS API is one thing because there we can catch things before they ever reach the room. On the SS API it's a different story because we can't really safely drop things if they were accepted and became important to the fabric of a room because of a bug at the time, otherwise we end up breaking the room.

Let's say we have a room v3 state event with a large number in it. If the federation /_matrix/federation/v1/send request body is supposed to enforce the strict canonical JSON rules to generate the X-Matrix signature, then by extension we're saying it isn't possible for a /_matrix/federation/v1/send request body to contain that event which breaks those strict rules, even though that event was allowed by the room version at the time because that room version didn't require that strictness.

We have to canonicalise the request body because that's the only way to create a reproducible JSON blob to sign, but if we're always enforcing the strict set of rules in doing so, then we are potentially applying new strict rules to old non-strict JSON blobs.

richvdh commented 2 years ago

Let's say we have a room v3 state event with a large number in it. If the federation /send request body is supposed to enforce the strict canonical JSON rules to generate the X-Matrix signature, then by extension we're saying it isn't possible for a /send request body to contain that event which breaks those strict rules

That's true, you can't send such an event in /send (or rather, spec-compliant servers will reject such attempts). There's nothing stopping it being pulled in retrospectively though. Still, the best thing for a server to do is to not generate (new) events like that in the first place.

even though that event was allowed by the room version at the time because that room version didn't require that strictness.

Yes, but there's a difference between events which were created before the bug was discovered, and those that are created now. Any server which still allows events with malformed canonicaljson to be created can expect to have a bad time when doing so, but because they can still be backfilled, it's not going to break the fabric of the room.

(I've just realised that Synapse does not reject such requests, and I have filed https://github.com/matrix-org/synapse/issues/13883 to track it.)