CVE-2020-26257: Uncontrolled Resource Consumption (CWE-400)
value: Matrix is an ecosystem for open federated Instant Messaging and VoIP. Synapse is a reference "homeserver" implementation of Matrix. A malicious or poorly-implemented homeserver can inject malformed events into a room by specifying a different room id in the path of a `/send_join`, `/send_leave`, `/invite` or `/exchange_third_party_invite` request. This can lead to a denial of service in which future events will not be correctly sent to other servers over federation. This affects any server which accepts federation requests from untrusted servers. The Matrix Synapse reference implementation before version 1.23.1 the implementation is vulnerable to this injection attack. Issue is fixed in version 1.23.1. As a workaround homeserver administrators could limit access to the federation API to trusted servers (for example via `federation_domain_whitelist`).
CWE description: The product does not properly control the allocation and maintenance of a limited resource, thereby enabling an actor to influence the amount of resources consumed, eventually leading to the exhaustion of available resources.
CVSS v3 base score: 6.5
Published: 2020-12-09T19:15Z
Repository: https://github.com/matrix-org/synapse
Language: Python
Files changed: changelog.d/8776.bugfix, synapse/federation/federation_server.py, synapse/federation/transport/server.py, synapse/handlers/federation.py, tests/handlers/test_federation.py
Commit date: 2020-12-09 10:38:39 +0000
Commit message: Consistently use room_id from federation request body (#8776)

* Consistently use room_id from federation request body

Some federation APIs have a redundant `room_id` path param (see
https://github.com/matrix-org/matrix-doc/issues/2330). We should make sure we
consistently use either the path param or the body param, and the body param is
easier.

* Kill off some references to "context"

Once upon a time, "rooms" were known as "contexts". I think this kills of the
last references to "contexts".
Diff: diff --git a/changelog.d/8776.bugfix b/changelog.d/8776.bugfix
new file mode 100644
index 000000000..dd7ebbeb8
--- /dev/null
+++ b/changelog.d/8776.bugfix
@@ -0,0 +1 @@
+Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body.
diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py
index 23278e36b..4b6ab470d 100644
--- a/synapse/federation/federation_server.py
+++ b/synapse/federation/federation_server.py
@@ -47,10 +47,11 @@ from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
 from synapse.events import EventBase
 from synapse.federation.federation_base import FederationBase, event_from_pdu_json
 from synapse.federation.persistence import TransactionActions
 from synapse.federation.units import Edu, Transaction
 from synapse.http.endpoint import parse_server_name
+from synapse.http.servlet import assert_params_in_dict
 from synapse.logging.context import (
     make_deferred_yieldable,
     nested_logging_context,
     run_in_background,
 )
@@ -389,11 +390,11 @@ class FederationServer(FederationBase):
             _process_edu,
             getattr(transaction, "edus", []),
             TRANSACTION_CONCURRENCY_LIMIT,
         )
 
-    async def on_context_state_request(
+    async def on_room_state_request(
         self, origin: str, room_id: str, event_id: str
     ) -> Tuple[int, Dict[str, Any]]:
         origin_host, _ = parse_server_name(origin)
         await self.check_server_matches_acl(origin_host, room_id)
 
@@ -512,15 +513,16 @@ class FederationServer(FederationBase):
         ret_pdu = await self.handler.on_invite_request(origin, pdu, room_version)
         time_now = self._clock.time_msec()
         return {"event": ret_pdu.get_pdu_json(time_now)}
 
     async def on_send_join_request(
-        self, origin: str, content: JsonDict, room_id: str
+        self, origin: str, content: JsonDict
     ) -> Dict[str, Any]:
         logger.debug("on_send_join_request: content: %s", content)
 
-        room_version = await self.store.get_room_version(room_id)
+        assert_params_in_dict(content, ["room_id"])
+        room_version = await self.store.get_room_version(content["room_id"])
         pdu = event_from_pdu_json(content, room_version)
 
         origin_host, _ = parse_server_name(origin)
         await self.check_server_matches_acl(origin_host, pdu.room_id)
 
@@ -545,16 +547,15 @@ class FederationServer(FederationBase):
         room_version = await self.store.get_room_version_id(room_id)
 
         time_now = self._clock.time_msec()
         return {"event": pdu.get_pdu_json(time_now), "room_version": room_version}
 
-    async def on_send_leave_request(
-        self, origin: str, content: JsonDict, room_id: str
-    ) -> dict:
+    async def on_send_leave_request(self, origin: str, content: JsonDict) -> dict:
         logger.debug("on_send_leave_request: content: %s", content)
 
-        room_version = await self.store.get_room_version(room_id)
+        assert_params_in_dict(content, ["room_id"])
+        room_version = await self.store.get_room_version(content["room_id"])
         pdu = event_from_pdu_json(content, room_version)
 
         origin_host, _ = parse_server_name(origin)
         await self.check_server_matches_acl(origin_host, pdu.room_id)
 
@@ -746,16 +747,12 @@ class FederationServer(FederationBase):
         ret = await self.handler.exchange_third_party_invite(
             sender_user_id, target_user_id, room_id, signed
         )
         return ret
 
-    async def on_exchange_third_party_invite_request(
-        self, room_id: str, event_dict: Dict
-    ):
-        ret = await self.handler.on_exchange_third_party_invite_request(
-            room_id, event_dict
-        )
+    async def on_exchange_third_party_invite_request(self, event_dict: Dict):
+        ret = await self.handler.on_exchange_third_party_invite_request(event_dict)
         return ret
 
     async def check_server_matches_acl(self, server_name: str, room_id: str):
         """Check if the given server is allowed by the server ACLs in the room
 
diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py
index a0933fae8..b53e7a20e 100644
--- a/synapse/federation/transport/server.py
+++ b/synapse/federation/transport/server.py
@@ -438,17 +438,17 @@ class FederationEventServlet(BaseFederationServlet):
     async def on_GET(self, origin, content, query, event_id):
         return await self.handler.on_pdu_request(origin, event_id)
 
 
 class FederationStateV1Servlet(BaseFederationServlet):
-    PATH = "/state/(?P<context>[^/]*)/?"
+    PATH = "/state/(?P<room_id>[^/]*)/?"
 
-    # This is when someone asks for all data for a given context.
-    async def on_GET(self, origin, content, query, context):
-        return await self.handler.on_context_state_request(
+    # This is when someone asks for all data for a given room.
+    async def on_GET(self, origin, content, query, room_id):
+        return await self.handler.on_room_state_request(
             origin,
-            context,
+            room_id,
             parse_string_from_args(query, "event_id", None, required=False),
         )
 
 
 class FederationStateIdsServlet(BaseFederationServlet):
@@ -461,20 +461,20 @@ class FederationStateIdsServlet(BaseFederationServlet):
             parse_string_from_args(query, "event_id", None, required=True),
         )
 
 
 class FederationBackfillServlet(BaseFederationServlet):
-    PATH = "/backfill/(?P<context>[^/]*)/?"
+    PATH = "/backfill/(?P<room_id>[^/]*)/?"
 
-    async def on_GET(self, origin, content, query, context):
+    async def on_GET(self, origin, content, query, room_id):
         versions = [x.decode("ascii") for x in query[b"v"]]
         limit = parse_integer_from_args(query, "limit", None)
 
         if not limit:
             return 400, {"error": "Did not include limit param"}
 
-        return await self.handler.on_backfill_request(origin, context, versions, limit)
+        return await self.handler.on_backfill_request(origin, room_id, versions, limit)
 
 
 class FederationQueryServlet(BaseFederationServlet):
     PATH = "/query/(?P<query_type>[^/]*)"
 
@@ -485,13 +485,13 @@ class FederationQueryServlet(BaseFederationServlet):
             {k.decode("utf8"): v[0].decode("utf-8") for k, v in query.items()},
         )
 
 
 class FederationMakeJoinServlet(BaseFederationServlet):
-    PATH = "/make_join/(?P<context>[^/]*)/(?P<user_id>[^/]*)"
+    PATH = "/make_join/(?P<room_id>[^/]*)/(?P<user_id>[^/]*)"
 
-    async def on_GET(self, origin, _content, query, context, user_id):
+    async def on_GET(self, origin, _content, query, room_id, user_id):
         """
         Args:
             origin (unicode): The authenticated server_name of the calling server
 
             _content (None): (GETs don't have bodies)
@@ -509,74 +509,74 @@ class FederationMakeJoinServlet(BaseFederationServlet):
             supported_versions = [v.decode("utf-8") for v in versions]
         else:
             supported_versions = ["1"]
 
         content = await self.handler.on_make_join_request(
-            origin, context, user_id, supported_versions=supported_versions
+            origin, room_id, user_id, supported_versions=supported_versions
         )
         return 200, content
 
 
 class FederationMakeLeaveServlet(BaseFederationServlet):
-    PATH = "/make_leave/(?P<context>[^/]*)/(?P<user_id>[^/]*)"
+    PATH = "/make_leave/(?P<room_id>[^/]*)/(?P<user_id>[^/]*)"
 
-    async def on_GET(self, origin, content, query, context, user_id):
-        content = await self.handler.on_make_leave_request(origin, context, user_id)
+    async def on_GET(self, origin, content, query, room_id, user_id):
+        content = await self.handler.on_make_leave_request(origin, room_id, user_id)
         return 200, content
 
 
 class FederationV1SendLeaveServlet(BaseFederationServlet):
     PATH = "/send_leave/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
     async def on_PUT(self, origin, content, query, room_id, event_id):
-        content = await self.handler.on_send_leave_request(origin, content, room_id)
+        content = await self.handler.on_send_leave_request(origin, content)
         return 200, (200, content)
 
 
 class FederationV2SendLeaveServlet(BaseFederationServlet):
     PATH = "/send_leave/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
     PREFIX = FEDERATION_V2_PREFIX
 
     async def on_PUT(self, origin, content, query, room_id, event_id):
-        content = await self.handler.on_send_leave_request(origin, content, room_id)
+        content = await self.handler.on_send_leave_request(origin, content)
         return 200, content
 
 
 class FederationEventAuthServlet(BaseFederationServlet):
-    PATH = "/event_auth/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
+    PATH = "/event_auth/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
-    async def on_GET(self, origin, content, query, context, event_id):
-        return await self.handler.on_event_auth(origin, context, event_id)
+    async def on_GET(self, origin, content, query, room_id, event_id):
+        return await self.handler.on_event_auth(origin, room_id, event_id)
 
 
 class FederationV1SendJoinServlet(BaseFederationServlet):
-    PATH = "/send_join/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
+    PATH = "/send_join/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
-    async def on_PUT(self, origin, content, query, context, event_id):
-        # TODO(paul): assert that context/event_id parsed from path actually
+    async def on_PUT(self, origin, content, query, room_id, event_id):
+        # TODO(paul): assert that room_id/event_id parsed from path actually
         #   match those given in content
-        content = await self.handler.on_send_join_request(origin, content, context)
+        content = await self.handler.on_send_join_request(origin, content)
         return 200, (200, content)
 
 
 class FederationV2SendJoinServlet(BaseFederationServlet):
-    PATH = "/send_join/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
+    PATH = "/send_join/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
     PREFIX = FEDERATION_V2_PREFIX
 
-    async def on_PUT(self, origin, content, query, context, event_id):
-        # TODO(paul): assert that context/event_id parsed from path actually
+    async def on_PUT(self, origin, content, query, room_id, event_id):
+        # TODO(paul): assert that room_id/event_id parsed from path actually
         #   match those given in content
-        content = await self.handler.on_send_join_request(origin, content, context)
+        content = await self.handler.on_send_join_request(origin, content)
         return 200, content
 
 
 class FederationV1InviteServlet(BaseFederationServlet):
-    PATH = "/invite/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
+    PATH = "/invite/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
-    async def on_PUT(self, origin, content, query, context, event_id):
+    async def on_PUT(self, origin, content, query, room_id, event_id):
         # We don't get a room version, so we have to assume its EITHER v1 or
         # v2. This is "fine" as the only difference between V1 and V2 is the
         # state resolution algorithm, and we don't use that for processing
         # invites
         content = await self.handler.on_invite_request(
@@ -587,16 +587,16 @@ class FederationV1InviteServlet(BaseFederationServlet):
         # due to a historical bug.
         return 200, (200, content)
 
 
 class FederationV2InviteServlet(BaseFederationServlet):
-    PATH = "/invite/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
+    PATH = "/invite/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
 
     PREFIX = FEDERATION_V2_PREFIX
 
-    async def on_PUT(self, origin, content, query, context, event_id):
-        # TODO(paul): assert that context/event_id parsed from path actually
+    async def on_PUT(self, origin, content, query, room_id, event_id):
+        # TODO(paul): assert that room_id/event_id parsed from path actually
         #   match those given in content
 
         room_version = content["room_version"]
         event = content["event"]
         invite_room_state = content["invite_room_state"]
@@ -614,13 +614,11 @@ class FederationV2InviteServlet(BaseFederationServlet):
 
 class FederationThirdPartyInviteExchangeServlet(BaseFederationServlet):
     PATH = "/exchange_third_party_invite/(?P<room_id>[^/]*)"
 
     async def on_PUT(self, origin, content, query, room_id):
-        content = await self.handler.on_exchange_third_party_invite_request(
-            room_id, content
-        )
+        content = await self.handler.on_exchange_third_party_invite_request(content)
         return 200, content
 
 
 class FederationClientKeysQueryServlet(BaseFederationServlet):
     PATH = "/user/keys/query"
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index c38695770..e03caea25 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -53,10 +53,11 @@ from synapse.crypto.event_signing import compute_event_signature
 from synapse.event_auth import auth_types_for_event
 from synapse.events import EventBase
 from synapse.events.snapshot import EventContext
 from synapse.events.validator import EventValidator
 from synapse.handlers._base import BaseHandler
+from synapse.http.servlet import assert_params_in_dict
 from synapse.logging.context import (
     make_deferred_yieldable,
     nested_logging_context,
     preserve_fn,
     run_in_background,
@@ -2684,24 +2685,23 @@ class FederationHandler(BaseHandler):
             await self.federation_client.forward_third_party_invite(
                 destinations, room_id, event_dict
             )
 
     async def on_exchange_third_party_invite_request(
-        self, room_id: str, event_dict: JsonDict
+        self, event_dict: JsonDict
     ) -> None:
         """Handle an exchange_third_party_invite request from a remote server
 
         The remote server will call this when it wants to turn a 3pid invite
         into a normal m.room.member invite.
 
         Args:
-            room_id: The ID of the room.
-
-            event_dict (dict[str, Any]): Dictionary containing the event body.
+            event_dict: Dictionary containing the event body.
 
         """
-        room_version = await self.store.get_room_version_id(room_id)
+        assert_params_in_dict(event_dict, ["room_id"])
+        room_version = await self.store.get_room_version_id(event_dict["room_id"])
 
         # NB: event_dict has a particular specced format we might need to fudge
         # if we change event formats too much.
         builder = self.event_builder_factory.new(room_version, event_dict)
 
diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py
index 9ef80fe50..bf866dacf 100644
--- a/tests/handlers/test_federation.py
+++ b/tests/handlers/test_federation.py
@@ -57,11 +57,10 @@ class FederationTestCase(unittest.HomeserverTestCase):
             body={},
             tok=tok,
         )
 
         d = self.handler.on_exchange_third_party_invite_request(
-            room_id=room_id,
             event_dict={
                 "type": EventTypes.Member,
                 "room_id": room_id,
                 "sender": user_id,
                 "state_key": "@someone:example.org",

Vulnerable code:     async def on_context_state_request(
        self, origin: str, content: JsonDict, room_id: str
        room_version = await self.store.get_room_version(room_id)
    async def on_send_leave_request(
        self, origin: str, content: JsonDict, room_id: str
    ) -> dict:
        room_version = await self.store.get_room_version(room_id)
    async def on_exchange_third_party_invite_request(
        self, room_id: str, event_dict: Dict
    ):
        ret = await self.handler.on_exchange_third_party_invite_request(
            room_id, event_dict
        )
    PATH = "/state/(?P<context>[^/]*)/?"
    # This is when someone asks for all data for a given context.
    async def on_GET(self, origin, content, query, context):
        return await self.handler.on_context_state_request(
            context,
    PATH = "/backfill/(?P<context>[^/]*)/?"
    async def on_GET(self, origin, content, query, context):
        return await self.handler.on_backfill_request(origin, context, versions, limit)
    PATH = "/make_join/(?P<context>[^/]*)/(?P<user_id>[^/]*)"
    async def on_GET(self, origin, _content, query, context, user_id):
            origin, context, user_id, supported_versions=supported_versions
    PATH = "/make_leave/(?P<context>[^/]*)/(?P<user_id>[^/]*)"
    async def on_GET(self, origin, content, query, context, user_id):
        content = await self.handler.on_make_leave_request(origin, context, user_id)
        content = await self.handler.on_send_leave_request(origin, content, room_id)
        content = await self.handler.on_send_leave_request(origin, content, room_id)
    PATH = "/event_auth/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
    async def on_GET(self, origin, content, query, context, event_id):
        return await self.handler.on_event_auth(origin, context, event_id)
    PATH = "/send_join/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, context, event_id):
        # TODO(paul): assert that context/event_id parsed from path actually
        content = await self.handler.on_send_join_request(origin, content, context)
    PATH = "/send_join/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, context, event_id):
        # TODO(paul): assert that context/event_id parsed from path actually
        content = await self.handler.on_send_join_request(origin, content, context)
    PATH = "/invite/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, context, event_id):
    PATH = "/invite/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, context, event_id):
        # TODO(paul): assert that context/event_id parsed from path actually
        content = await self.handler.on_exchange_third_party_invite_request(
            room_id, content
        )
        self, room_id: str, event_dict: JsonDict
            room_id: The ID of the room.

            event_dict (dict[str, Any]): Dictionary containing the event body.
        room_version = await self.store.get_room_version_id(room_id)
            room_id=room_id,
Fixed code: Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body.
from synapse.http.servlet import assert_params_in_dict
    async def on_room_state_request(
        self, origin: str, content: JsonDict
        assert_params_in_dict(content, ["room_id"])
        room_version = await self.store.get_room_version(content["room_id"])
    async def on_send_leave_request(self, origin: str, content: JsonDict) -> dict:
        assert_params_in_dict(content, ["room_id"])
        room_version = await self.store.get_room_version(content["room_id"])
    async def on_exchange_third_party_invite_request(self, event_dict: Dict):
        ret = await self.handler.on_exchange_third_party_invite_request(event_dict)
    PATH = "/state/(?P<room_id>[^/]*)/?"
    # This is when someone asks for all data for a given room.
    async def on_GET(self, origin, content, query, room_id):
        return await self.handler.on_room_state_request(
            room_id,
    PATH = "/backfill/(?P<room_id>[^/]*)/?"
    async def on_GET(self, origin, content, query, room_id):
        return await self.handler.on_backfill_request(origin, room_id, versions, limit)
    PATH = "/make_join/(?P<room_id>[^/]*)/(?P<user_id>[^/]*)"
    async def on_GET(self, origin, _content, query, room_id, user_id):
            origin, room_id, user_id, supported_versions=supported_versions
    PATH = "/make_leave/(?P<room_id>[^/]*)/(?P<user_id>[^/]*)"
    async def on_GET(self, origin, content, query, room_id, user_id):
        content = await self.handler.on_make_leave_request(origin, room_id, user_id)
        content = await self.handler.on_send_leave_request(origin, content)
        content = await self.handler.on_send_leave_request(origin, content)
    PATH = "/event_auth/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
    async def on_GET(self, origin, content, query, room_id, event_id):
        return await self.handler.on_event_auth(origin, room_id, event_id)
    PATH = "/send_join/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, room_id, event_id):
        # TODO(paul): assert that room_id/event_id parsed from path actually
        content = await self.handler.on_send_join_request(origin, content)
    PATH = "/send_join/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, room_id, event_id):
        # TODO(paul): assert that room_id/event_id parsed from path actually
        content = await self.handler.on_send_join_request(origin, content)
    PATH = "/invite/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, room_id, event_id):
    PATH = "/invite/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
    async def on_PUT(self, origin, content, query, room_id, event_id):
        # TODO(paul): assert that room_id/event_id parsed from path actually
        content = await self.handler.on_exchange_third_party_invite_request(content)
from synapse.http.servlet import assert_params_in_dict
        self, event_dict: JsonDict
            event_dict: Dictionary containing the event body.
        assert_params_in_dict(event_dict, ["room_id"])
        room_version = await self.store.get_room_version_id(event_dict["room_id"])