Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Reject instead of erroring on invalid membership events #15564

Merged
merged 6 commits into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15564.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where an invalid membership event could cause an internal server error.
17 changes: 11 additions & 6 deletions synapse/event_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -1054,10 +1054,15 @@ def _verify_third_party_invite(
"""
if "third_party_invite" not in event.content:
return False
if "signed" not in event.content["third_party_invite"]:
third_party_invite = event.content["third_party_invite"]
if not isinstance(third_party_invite, collections.abc.Mapping):
return False
signed = event.content["third_party_invite"]["signed"]
for key in {"mxid", "token"}:
if "signed" not in third_party_invite:
return False
signed = third_party_invite["signed"]
if not isinstance(signed, collections.abc.Mapping):
return False
for key in {"mxid", "token", "signatures"}:
if key not in signed:
return False

Expand All @@ -1075,8 +1080,6 @@ def _verify_third_party_invite(

if signed["mxid"] != event.state_key:
return False
if signed["token"] != token:
return False
Comment on lines -1078 to -1079
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On line 1064 we set token = signed["token"]. Neither token nor signed change in the interim, so this is now just checking against itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I dug into the past to see if it was different and this just seems to be the third_party_invite content shape evolving and mistaken refactors from 8 years ago.


for public_key_object in get_public_keys(invite_event):
public_key = public_key_object["public_key"]
Expand All @@ -1088,7 +1091,9 @@ def _verify_third_party_invite(
verify_key = decode_verify_key_bytes(
key_name, decode_base64(public_key)
)
verify_signed_json(signed, server, verify_key)
# verify_signed_json incorrectly states it wants a dict, it
# just needs a mapping.
verify_signed_json(signed, server, verify_key) # type: ignore[arg-type]
Comment on lines +1094 to +1096
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this separately in signed-json.


# We got the public key from the invite, so we know that the
# correct server signed the signed bundle.
Expand Down
Loading