synapse checks signature on join_authorised_via_users_server in m.room.membership events for room versions which do not specify it

[richvdh]

As of room v8 and later, an extra auth rule is added which says that the event must be signed by the server given in join_authorised_via_users_server. However, synapse also applies that to check to old room versions, and to membership events other than joins. Since spec-compliant implementations will not perform the check in those instances, this could lead to spilt-brained rooms.

[richvdh]

cc @clokep

Synapse also does the signature check if do_sig_check is true. I don't really understand why that parameter is ever set to false, but the docstring is "True if it should be verified that the sending server signed the event." This is nothing to do with the sending server.

[clokep]

There's a few spots I think you might be referring to:

• _check_sigs_on_pdu which checks signatures on events to ensure they're signed by the sending server (according to _check_sigs_and_hash). This seems to properly check the room version; it does not check whether the join rules are set to restricted though since that would require knowing other events which isn't really part of checking signatures (but it does mean that you could fail this check if you just throw an unused join_authorised_via_users_server into the event).
• event_auth.check in the do_sig_check block which ensures that the event has a signature, but does not assert the signature is valid.
• event_auth.check in the restricted join rules processing, this processes the join event to ensure it meets the requirements of the join rule. I don't see this doing anything with signature checking?

I don't see any of these meeting the requirements in the description, could you point to which of these (or maybe I missed a spot!) that you were taking a look at?

Synapse also does the signature check if do_sig_check is true. I don't really understand why that parameter is ever set to false, but the docstring is "True if it should be verified that the sending server signed the event." This is nothing to do with the sending server.

do_sig_check is False during a make_{join,leave,knock} since the partial event cannot have valid signatures yet. There might be other situations too. Note that it doesn't seem to check the validity of the signature here, just that one exists. (The assumption being that the signature was already checked to be valid in _check_sigs_on_pdu, I think.)

The comment was added in #10393, I might have a flawed understanding of what this code is doing (or might have over-simplified for the comment).

[richvdh]

Sorry! This one:

• event_auth.check in the do_sig_check block which ensures that the event has a signature, but does not assert the signature is valid.

right, but it still checks it is there, which is a new requirement which existing servers would not apply - hence the danger of split-braining where some servers accept, and some reject, the event.

But now that you point out the other places, I have another opinion :/

• _check_sigs_on_pdu which checks signatures on events to ensure they're signed by the sending server (according to _check_sigs_and_hash). This seems to properly check the room version; it does not check whether the join rules are set to restricted though since that would require knowing other events which isn't really part of checking signatures (but it does mean that you could fail this check if you just throw an unused join_authorised_via_users_server into the event).

The implementation here looks sound, though:

• Again, this is not necessarily the sending server - we should make sure that comments and docstrings do not imply that these signature checks are limited to the sending server.
• Not a new problem, but the fact that event_auth.check doesn't check the validity of any signatures (merely assuming that any signatures that are present have already been validated) is confusing and feels a bit brittle. In particular, it feels like it would be very easy for us to end up relying on a signature whose validity we haven't already checked. We should at the very least have some comments explaining what's going on.
• We'll need to make sure that whatever ends up getting specced in Room versions 8 and 9: Restricted rooms matrix-spec-proposals#3387 matches this. I don't think it does, currently.

There might be other situations too.

well yes, therein lies the rub. I was mostly worrying about the calls from synapse.state.{v1,v2}. However, on staring at them a bit more closely, I think it's probably correct (at that point the events have already been accepted or rejected, and we are now authing them against different sets of state - so we wouldn't expect things like "is a signature present" to change).

I would say: we should update the docstring on event_auth.check to make it clear that do_sig_check does not just apply to the sending server; however it's hard not to feel like it just needs splitting up into smaller functions. Leave that one with me, and I'll do it as part of my work on #9595.

[clokep]

Sorry! This one:

• event_auth.check in the do_sig_check block which ensures that the event has a signature, but does not assert the signature is valid.

right, but it still checks it is there, which is a new requirement which existing servers would not apply - hence the danger of split-braining where some servers accept, and some reject, the event.

This should be checking the room versions, I think this is what you're filing this issue about?

But now that you point out the other places, I have another opinion :/

• _check_sigs_on_pdu which checks signatures on events to ensure they're signed by the sending server (according to _check_sigs_and_hash). This seems to properly check the room version; it does not check whether the join rules are set to restricted though since that would require knowing other events which isn't really part of checking signatures (but it does mean that you could fail this check if you just throw an unused join_authorised_via_users_server into the event).

The implementation here looks sound, though:

* Again, this is not necessarily the sending server - we should make sure that comments and docstrings do not imply that these signature checks are limited to the sending server.

If you have thoughts on an updated comment I'd really appreciate it! I added it while trying to figure out how all this works and I probably don't have a full underestanding!

* Not a new problem, but the fact that `event_auth.check` doesn't check the validity of any signatures (merely assuming that any signatures that are present have already been validated) is confusing and feels a bit brittle. In particular, it feels like it would be very easy for us to end up relying on a signature whose validity we haven't already checked. We should at the very least have some comments explaining what's going on.

I think @erikjohnston said that this was for (a) speed, and (b) because a signature could become invalid after it was accepted due to a server changing their keys, so you don't want to constantly check them. They should be accepted or not once.

* We'll need to make sure that whatever ends up getting specced in [Room versions 8 and 9: Restricted rooms matrix-doc#3387](https://github.com/matrix-org/matrix-doc/pull/3387) matches this. I don't think it does, currently.

It sounds like this is a minor implementation bug (which I'll fix shortly!), but I don't think the proper implementation is different than the spec?

There might be other situations too.

well yes, therein lies the rub. I was mostly worrying about the calls from synapse.state.{v1,v2}. However, on staring at them a bit more closely, I think it's probably correct (at that point the events have already been accepted or rejected, and we are now authing them against different sets of state - so we wouldn't expect things like "is a signature present" to change).

I would say: we should update the docstring on event_auth.check to make it clear that do_sig_check does not just apply to the sending server; however it's hard not to feel like it just needs splitting up into smaller functions. Leave that one with me, and I'll do it as part of my work on #9595.

This sounds like my understanding of why this flag exists (b) from above.

[richvdh]

• We'll need to make sure that whatever ends up getting specced in Room versions 8 and 9: Restricted rooms matrix-doc#3387 matches this. I don't think it does, currently.

It sounds like this is a minor implementation bug (which I'll fix shortly!), but I don't think the proper implementation is different than the spec?

As an example: we currently check the signature irrespective of the current membership of the user, and will reject any incorrectly-signed events, even if they represent join->join transitions. That is not what matrix-org/matrix-spec-proposals#3387 says at present, hence my thread at matrix-org/matrix-spec-proposals#3387 (comment).

I don't know if there are other examples, but I'm really asking for your help in making sure that we have an exact match between the spec and the implementation.

[clokep]

As an example: we currently check the signature irrespective of the current membership of the user, and will reject any incorrectly-signed events, even if they represent join->join transitions. That is not what matrix-org/matrix-doc#3387 says at present, hence my thread at matrix-org/matrix-doc#3387 (comment).

This is pretty much #10920 which I'm working on now!

[clokep]

@richvdh I'm not 100% sure this is "fixed", but I think the initial technical big is fixed!