Skip to content
This repository was 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

Reject instead of erroring on invalid membership events #15564

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
61fb212
Do not error if sending an invalid membership event.
clokep May 10, 2023
3c9ec0b
Remove identity check.
clokep May 10, 2023
748e9b6
Newsfragment
clokep May 10, 2023
c075657
Fix type hint.
clokep May 12, 2023
45b0bbf
Also check for signatures.
clokep May 15, 2023
34faf72
Merge remote-tracking branch 'origin/develop' into clokep/third-party…
clokep May 15, 2023
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
View file
Open in desktop
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.
15 changes: 10 additions & 5 deletions synapse/event_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,9 +1054,14 @@ 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
if "signed" not in third_party_invite:
return False
signed = third_party_invite["signed"]
if not isinstance(signed, collections.abc.Mapping):
return False
signed = event.content["third_party_invite"]["signed"]
for key in {"mxid", "token"}:
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
Member 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
Member 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