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

Strip unauthorized fields from unsigned object in events received over federation #11530

Merged
merged 21 commits into from
Jan 6, 2022
Merged
Show file tree
Hide file tree
Changes from 13 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/11530.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add functionality to strip the unsignedData object of unauthorized fields in events arriving over federation.
H-Shay marked this conversation as resolved.
Show resolved Hide resolved
50 changes: 49 additions & 1 deletion synapse/handlers/federation_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from http import HTTPStatus
from typing import (
TYPE_CHECKING,
Any,
Collection,
Container,
Dict,
Expand Down Expand Up @@ -52,7 +53,7 @@
check_auth_rules_for_event,
validate_event_for_room_version,
)
from synapse.events import EventBase
from synapse.events import EventBase, make_event_from_dict
from synapse.events.snapshot import EventContext
from synapse.federation.federation_client import InvalidResponseError
from synapse.logging.context import nested_logging_context, run_in_background
Expand Down Expand Up @@ -181,6 +182,11 @@ async def on_receive_pdu(self, origin: str, pdu: EventBase) -> None:
logger.warning("Received event failed sanity checks")
raise FederationError("ERROR", err.code, err.msg, affected=pdu.event_id)

# Strip any unauthorized unsigned values if they exist
event_dict = pdu.get_dict()
if "unsigned" in event_dict and event_dict["unsigned"] != {}:
pdu = self._strip_unsigned_values(pdu, event_dict)

# If we are currently in the process of joining this room, then we
# queue up events for later processing.
if room_id in self.room_queues:
Expand Down Expand Up @@ -325,6 +331,11 @@ async def on_send_membership_event(

assert not event.internal_metadata.outlier

# Strip any unauthorized unsigned values if they exist
event_dict = event.get_dict()
if "unsigned" in event_dict and event_dict["unsigned"] != {}:
event = self._strip_unsigned_values(event, event_dict)

# Send this event on behalf of the other server.
#
# The remote server isn't a full participant in the room at this point, so
Expand Down Expand Up @@ -1939,3 +1950,40 @@ def _sanity_check_event(self, ev: EventBase) -> None:
len(ev.auth_event_ids()),
)
raise SynapseError(HTTPStatus.BAD_REQUEST, "Too many auth_events")

def _strip_unsigned_values(
self, pdu: EventBase, event_dict: Dict[str, Any]
) -> EventBase:
"""
Strip any unsigned values unless specifically allowed, as defined by the whitelist.

pdu: the event to strip values from
event_dict: a dict of values derived from the pdu (note that this dict is mutated
by this function)
"""
unsigned = event_dict["unsigned"]

if not isinstance(unsigned, dict):
event_dict["unsigned"] = {}
return make_event_from_dict(
event_dict, pdu.room_version, pdu.internal_metadata.get_dict()
)

if pdu.type == EventTypes.Member:
whitelist = ["knock_room_state", "invite_room_state", "age"]
else:
whitelist = ["age"]

filtered_unsigned = {}

for k, v in unsigned.items():
if k in whitelist:
filtered_unsigned[k] = v

event_dict["unsigned"] = filtered_unsigned
H-Shay marked this conversation as resolved.
Show resolved Hide resolved

stripped_event = make_event_from_dict(
event_dict, pdu.room_version, pdu.internal_metadata.get_dict()
)

return stripped_event
107 changes: 107 additions & 0 deletions tests/test_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -276,3 +276,110 @@ def test_cross_signing_keys_retry(self):
"ed25519:" + remote_self_signing_key in self_signing_key["keys"].keys(),
)
self.assertTrue(remote_self_signing_key in self_signing_key["keys"].values())


class StripUnsignedFromEventsTestCase(MessageAcceptTests):
Copy link
Member

Choose a reason for hiding this comment

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

MessageAcceptTests includes a load of its own tests, so by deriving your test class from it in this way, we end up with another copy of those tests, and so do them twice (see https://github.com/matrix-org/synapse/runs/4612144055?check_suite_focus=true#step:8:2560).

I think your tests will be fine without prev_state and prev_events, so I think you can remove them and the calls to get_latest_event_ids_in_room. Then you can just inherit from UnitTest, rather than having to do all the complicated setup in MessageAcceptTests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, it's helpful to know.

def test_strip_unauthorized_unsigned_values(self):
most_recent = self.get_success(
self.homeserver.get_datastore().get_latest_event_ids_in_room(self.room_id)
)[0]
federation_event_handler = self.homeserver.get_federation_event_handler()
event1 = make_event_from_dict(
{
"room_id": self.room_id,
"sender": "@baduser:test.serv",
"state_key": "@baduser:test.serv",
"event_id": "$event1:test.serv",
"depth": 1000,
"origin_server_ts": 1,
"type": "m.room.member",
"origin": "test.servx",
"content": {"membership": "join"},
"auth_events": [],
"prev_state": [(most_recent, {})],
"prev_events": [(most_recent, {})],
"unsigned": {"malicious garbage": "hackz", "more warez": "more hackz"},
}
)
self.get_success(federation_event_handler.on_receive_pdu("test.serv", event1))

event = self.get_success(self.store.get_event("$event1:test.serv"))
event_dict = event.get_dict()
# Make sure unauthorized fields are stripped from unsigned
self.assertNotIn("more warez", event_dict["unsigned"])

def test_strip_event_maintains_allowed_fields(self):
most_recent = self.get_success(
self.homeserver.get_datastore().get_latest_event_ids_in_room(self.room_id)
)[0]
federation_event_handler = self.homeserver.get_federation_event_handler()
event2 = make_event_from_dict(
{
"room_id": self.room_id,
"sender": "@baduser:test.serv",
"state_key": "@baduser:test.serv",
"event_id": "$event2:test.serv",
"depth": 1000,
"origin_server_ts": 1,
"type": "m.room.member",
"origin": "test.servx",
"auth_events": [],
"prev_state": [(most_recent, {})],
"prev_events": [(most_recent, {})],
"content": {"membership": "join"},
"unsigned": {
"malicious garbage": "hackz",
"more warez": "more hackz",
"age": 14,
"invite_room_state": [],
},
}
)
self.get_success(
federation_event_handler.on_send_membership_event("test.serv", event2)
)

event = self.get_success(self.store.get_event("$event2:test.serv"))
event_dict = event.get_dict()
self.assertIn("age", event_dict["unsigned"])
self.assertEqual(14, event_dict["unsigned"]["age"])
self.assertNotIn("more warez", event_dict["unsigned"])
# Invite_room_state is allowed in events of type m.room.member
self.assertIn("invite_room_state", event_dict["unsigned"])
self.assertEqual([], event_dict["unsigned"]["invite_room_state"])

def test_strip_event_removes_fields_based_on_event_type(self):
most_recent = self.get_success(
self.homeserver.get_datastore().get_latest_event_ids_in_room(self.room_id)
)[0]
federation_event_handler = self.homeserver.get_federation_event_handler()
event3 = make_event_from_dict(
{
"room_id": self.room_id,
"sender": "@baduser:test.serv",
"state_key": "@baduser:test.serv",
"event_id": "$event3:test.serv",
"depth": 1000,
"origin_server_ts": 1,
"type": "m.room.power_levels",
"origin": "test.servx",
"content": {},
"auth_events": [],
"prev_state": [(most_recent, {})],
"prev_events": [(most_recent, {})],
"unsigned": {
"malicious garbage": "hackz",
"more warez": "more hackz",
"age": 14,
"invite_room_state": [],
},
}
)
self.get_success(federation_event_handler.on_receive_pdu("test.serv", event3))

event = self.get_success(self.store.get_event("$event3:test.serv"))
event_dict = event.get_dict()
self.assertIn("age", event_dict["unsigned"])
# Invite_room_state field is only permitted in event type m.room.member
self.assertNotIn("invite_room_state", event_dict["unsigned"])
self.assertNotIn("more warez", event_dict["unsigned"])