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

* add some tests to verify we are stripping unauthorized fields out of unsigned

* add function to strip unauthorized fields from the unsigned object of event

* newsfragment

* update newsfragment number

* add check to on_send_membership_event

* refactor tests

* fix lint error

* slightly refactor tests and add some comments

* slight refactor

* refactor tests

* fix import error

* slight refactor

* remove unsigned filtration code from synapse/handlers/federation_event.py

* lint

* move unsigned filtering code to event base

* refactor tests

* update newsfragment

* requested changes

* remove unused retun values
This commit is contained in:
Shay 2022-01-06 09:09:30 -08:00 committed by GitHub
parent 2ef1fea8d2
commit 70ce9aea71
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 99 additions and 0 deletions

2
changelog.d/11530.bugfix Normal file
View File

@ -0,0 +1,2 @@
Fix a long-standing issue which could cause Synapse to incorrectly accept data in the unsigned field of events
received over federation.

View File

@ -230,6 +230,10 @@ def event_from_pdu_json(pdu_json: JsonDict, room_version: RoomVersion) -> EventB
# origin, etc etc) # origin, etc etc)
assert_params_in_dict(pdu_json, ("type", "depth")) assert_params_in_dict(pdu_json, ("type", "depth"))
# Strip any unauthorized values from "unsigned" if they exist
if "unsigned" in pdu_json:
_strip_unsigned_values(pdu_json)
depth = pdu_json["depth"] depth = pdu_json["depth"]
if not isinstance(depth, int): if not isinstance(depth, int):
raise SynapseError(400, "Depth %r not an intger" % (depth,), Codes.BAD_JSON) raise SynapseError(400, "Depth %r not an intger" % (depth,), Codes.BAD_JSON)
@ -245,3 +249,24 @@ def event_from_pdu_json(pdu_json: JsonDict, room_version: RoomVersion) -> EventB
event = make_event_from_dict(pdu_json, room_version) event = make_event_from_dict(pdu_json, room_version)
return event return event
def _strip_unsigned_values(pdu_dict: JsonDict) -> None:
"""
Strip any unsigned values unless specifically allowed, as defined by the whitelist.
pdu: the json dict to strip values from. Note that the dict is mutated by this
function
"""
unsigned = pdu_dict["unsigned"]
if not isinstance(unsigned, dict):
pdu_dict["unsigned"] = {}
if pdu_dict["type"] == "m.room.member":
whitelist = ["knock_room_state", "invite_room_state", "age"]
else:
whitelist = ["age"]
filtered_unsigned = {k: v for k, v in unsigned.items() if k in whitelist}
pdu_dict["unsigned"] = filtered_unsigned

View File

@ -17,7 +17,9 @@ from unittest.mock import Mock
from twisted.internet.defer import succeed from twisted.internet.defer import succeed
from synapse.api.errors import FederationError from synapse.api.errors import FederationError
from synapse.api.room_versions import RoomVersions
from synapse.events import make_event_from_dict from synapse.events import make_event_from_dict
from synapse.federation.federation_base import event_from_pdu_json
from synapse.logging.context import LoggingContext from synapse.logging.context import LoggingContext
from synapse.types import UserID, create_requester from synapse.types import UserID, create_requester
from synapse.util import Clock from synapse.util import Clock
@ -276,3 +278,73 @@ class MessageAcceptTests(unittest.HomeserverTestCase):
"ed25519:" + remote_self_signing_key in self_signing_key["keys"].keys(), "ed25519:" + remote_self_signing_key in self_signing_key["keys"].keys(),
) )
self.assertTrue(remote_self_signing_key in self_signing_key["keys"].values()) self.assertTrue(remote_self_signing_key in self_signing_key["keys"].values())
class StripUnsignedFromEventsTestCase(unittest.TestCase):
def test_strip_unauthorized_unsigned_values(self):
event1 = {
"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": [],
"unsigned": {"malicious garbage": "hackz", "more warez": "more hackz"},
}
filtered_event = event_from_pdu_json(event1, RoomVersions.V1)
# Make sure unauthorized fields are stripped from unsigned
self.assertNotIn("more warez", filtered_event.unsigned)
def test_strip_event_maintains_allowed_fields(self):
event2 = {
"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": [],
"content": {"membership": "join"},
"unsigned": {
"malicious garbage": "hackz",
"more warez": "more hackz",
"age": 14,
"invite_room_state": [],
},
}
filtered_event2 = event_from_pdu_json(event2, RoomVersions.V1)
self.assertIn("age", filtered_event2.unsigned)
self.assertEqual(14, filtered_event2.unsigned["age"])
self.assertNotIn("more warez", filtered_event2.unsigned)
# Invite_room_state is allowed in events of type m.room.member
self.assertIn("invite_room_state", filtered_event2.unsigned)
self.assertEqual([], filtered_event2.unsigned["invite_room_state"])
def test_strip_event_removes_fields_based_on_event_type(self):
event3 = {
"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": [],
"unsigned": {
"malicious garbage": "hackz",
"more warez": "more hackz",
"age": 14,
"invite_room_state": [],
},
}
filtered_event3 = event_from_pdu_json(event3, RoomVersions.V1)
self.assertIn("age", filtered_event3.unsigned)
# Invite_room_state field is only permitted in event type m.room.member
self.assertNotIn("invite_room_state", filtered_event3.unsigned)
self.assertNotIn("more warez", filtered_event3.unsigned)