diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 0c35b4a7ba..3dfea09b84 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -823,20 +823,24 @@ class HttpResponseException(CodeMessageException): super().__init__(code, msg) self.response = response - def to_synapse_error(self) -> SynapseError: - """Make a SynapseError based on an HTTPResponseException + def unsafe_to_verbatim_synapse_error(self) -> SynapseError: + """Make a SynapseError directly based on a TRUSTED HTTPResponseException. This is useful when a proxied request has failed, and we need to decide how to map the failure onto a matrix error to send back to the client. - An attempt is made to parse the body of the http response as a matrix + An attempt is made to parse the body of the HTTP response as a Matrix error. If that succeeds, the errcode and error message from the body - are used as the errcode and error message in the new synapse error. + are copied verbatim into the new Synapse error. Otherwise, the errcode is set to M_UNKNOWN, and the error message is set to the reason code from the HTTP response. + Safety: + This must ONLY be used on errors from TRUSTED sources, + such as other Synapse workers. + Returns: The error converted to a SynapseError. """ @@ -851,10 +855,68 @@ class HttpResponseException(CodeMessageException): j = {} errcode = j.pop("errcode", Codes.UNKNOWN) + if not isinstance(errcode, str): + errcode = Codes.UNKNOWN errmsg = j.pop("error", self.msg) + if not isinstance(errmsg, str): + errmsg = self.msg return ProxiedRequestError(self.code, errmsg, errcode, j) + def to_synapse_error(self) -> SynapseError: + """Make a SynapseError directly based on a TRUSTED HTTPResponseException. + + This is useful when a proxied request has failed, and we need to + decide how to map the failure onto a matrix error to send back to the + client. + + An attempt is made to parse the body of the HTTP response as a Matrix + error. If that succeeds, the errcode and error message from the body + are copied verbatim into the new Synapse error, unless it's of + a forbidden type. + + Otherwise, the errcode is set to M_UNKNOWN, and the error message is + set to the reason code from the HTTP response. + + Safety: + This is the correct method to use when forwarding errors + from upstream requests (e.g. federation, policy servers). + + FIXME: restrict forwarded errors further + + Returns: + The error converted to a SynapseError. + """ + # try to parse the body as json, to get better errcode/msg, but + # default to M_UNKNOWN with the HTTP status as the error text + try: + j = json_decoder.decode(self.response.decode("utf-8")) + except ValueError: + j = {} + + if not isinstance(j, dict): + j = {} + + status = self.code + errcode = j.pop("errcode", Codes.UNKNOWN) + if not isinstance(errcode, str): + errcode = Codes.UNKNOWN + errmsg = j.pop("error", self.msg) + if not isinstance(errmsg, str): + errmsg = self.msg + + if errcode == Codes.UNKNOWN_TOKEN: + # We must not relay this error code back down to clients, + # because clients interpret this code to mean that they + # have been logged out. + # See: https://github.com/element-hq/synapse/security/advisories/GHSA-95fh-hv8c-chvq + errcode = Codes.UNKNOWN + + if status == HTTPStatus.UNAUTHORIZED: + status = HTTPStatus.BAD_REQUEST + + return ProxiedRequestError(status, errmsg, errcode, j) + class HomeServerNotSetupException(Exception): """ diff --git a/synapse/replication/http/_base.py b/synapse/replication/http/_base.py index 87d6e80898..686c2351d2 100644 --- a/synapse/replication/http/_base.py +++ b/synapse/replication/http/_base.py @@ -344,7 +344,12 @@ class ReplicationEndpoint(metaclass=abc.ABCMeta): code=e.code, **{SERVER_NAME_LABEL: server_name}, ).inc() - raise e.to_synapse_error() + # This error is coming from another worker, so we trust it to be safe + # to relay to clients directly. + # In fact, we rely relaying verbatim at the very least to tell + # clients when they are rate-limited, + # but most likely other things too. + raise e.unsafe_to_verbatim_synapse_error() except Exception as e: _outgoing_request_counter.labels( name=cls.NAME, diff --git a/tests/handlers/test_room_policy.py b/tests/handlers/test_room_policy.py index c67ea9b0e0..6d912fd79d 100644 --- a/tests/handlers/test_room_policy.py +++ b/tests/handlers/test_room_policy.py @@ -12,17 +12,22 @@ # . # # +from http import HTTPStatus from unittest import mock import signedjson +from parameterized import parameterized from signedjson.key import encode_verify_key_base64, get_verify_key +from twisted.internet import defer from twisted.internet.testing import MemoryReactor +from twisted.web.client import Agent from synapse.api.constants import EventTypes from synapse.api.errors import HttpResponseException, SynapseError from synapse.crypto.event_signing import compute_event_signature from synapse.events import EventBase +from synapse.federation.transport.client import TransportLayerClient from synapse.handlers.room_policy import POLICY_SERVER_KEY_ID from synapse.rest import admin from synapse.rest.client import filter, login, room, sync @@ -31,7 +36,7 @@ from synapse.types import JsonDict, UserID from synapse.util.clock import Clock from tests import unittest -from tests.test_utils import event_injection +from tests.test_utils import FakeResponse, event_injection from tests.test_utils.event_builders import make_test_event @@ -546,3 +551,92 @@ class RoomPolicyTestCase(unittest.FederatingHomeserverTestCase): if ev["event_id"] == event_id: return ev return None + + def _mock_policy_server_response_with_http_error( + self, + status: HTTPStatus, + error_body: JsonDict, + ) -> None: + """ + Make the policy server reply to its `/sign` endpoint with an error. + + Args: + status: the HTTP status to return + error_body: the JSON error body to return + """ + + def request( + method: bytes, + uri: bytes, + headers: object = None, + bodyProducer: object = None, + ) -> "defer.Deferred": + # For our test, we don't expect any other outbound request + assert b"/_matrix/policy/v1/sign" in uri, ( + f"unexpected outbound request to {uri!r}" + ) + return defer.succeed( + FakeResponse.json( + code=status, + payload=error_body, + ) + ) + + fake_agent = mock.create_autospec(Agent, spec_set=True) + fake_agent.request.side_effect = request + self.handler._federation_client.transport_layer = TransportLayerClient(self.hs) + self.hs.get_federation_http_client().agent = fake_agent + + @parameterized.expand( + ( + ( + HTTPStatus.IM_A_TEAPOT, + {"errcode": "M_FORBIDDEN", "error": "No coffee here"}, + HTTPStatus.IM_A_TEAPOT, + {"errcode": "M_FORBIDDEN", "error": "No coffee here"}, + ), + # This case is https://github.com/element-hq/synapse/security/advisories/GHSA-95fh-hv8c-chvq + # The error is rewritten for safety. + ( + HTTPStatus.UNAUTHORIZED, + {"errcode": "M_UNKNOWN_TOKEN", "error": "unknown token"}, + HTTPStatus.BAD_REQUEST, + { + "errcode": "M_UNKNOWN", + "error": "unknown token", + }, + ), + ) + ) + def test_policy_server_error_bubbling_to_client( + self, + policy_server_error_status: HTTPStatus, + policy_server_error_body: JsonDict, + expected_client_facing_error_status: HTTPStatus, + expected_client_facing_error_body: JsonDict, + ) -> None: + """ + Tests how errors from the policy server are forwarded back to clients. + + Regression test for https://github.com/element-hq/synapse/security/advisories/GHSA-95fh-hv8c-chvq + """ + + verify_key_str = encode_verify_key_base64(get_verify_key(self.signing_key)) + self._add_policy_server_to_room(public_key=verify_key_str) + + # Mock the policy server (at the HTTP level) to return + # the configured error + self._mock_policy_server_response_with_http_error( + policy_server_error_status, + policy_server_error_body, + ) + + response_body = self.helper.send_event( + self.room_id, + "m.room.message", + {"body": "honk", "msgtype": "m.text"}, + tok=self.creator_token, + expect_code=expected_client_facing_error_status, + ) + + self.assertEqual(response_body, expected_client_facing_error_body) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 90063ccb06..ca95a3a099 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -26,12 +26,14 @@ import json from http import HTTPStatus from typing import Any, Iterable, Literal -from unittest.mock import AsyncMock, Mock, call, patch +from unittest.mock import AsyncMock, Mock, call, create_autospec, patch from urllib import parse as urlparse from parameterized import param, parameterized +from twisted.internet import defer from twisted.internet.testing import MemoryReactor +from twisted.web.client import Agent import synapse.rest.admin from synapse.api.constants import ( @@ -67,6 +69,7 @@ from synapse.util.stringutils import random_string from tests import unittest from tests.http.server._base import make_request_with_cancellation_test from tests.storage.test_stream import PaginationTestCase +from tests.test_utils import FakeResponse from tests.test_utils.event_injection import ( create_event, inject_event, @@ -5764,3 +5767,117 @@ class MSC4293RedactOnBanKickTestCase(unittest.FederatingHomeserverTestCase): expect_redaction=True, reason="being disruptive", ) + + +class CreateRoomRemoteInviteTestCase(unittest.FederatingHomeserverTestCase): + """ + Tests error propagation from remote invites during /createRoom. + + Regression test for https://github.com/element-hq/synapse/security/advisories/GHSA-95fh-hv8c-chvq. + """ + + servlets = [ + room.register_servlets, + login.register_servlets, + register.register_servlets, + admin.register_servlets, + ] + + hijack_auth = False + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.user_id = self.register_user("creator", "test") + self.token = self.login("creator", "test") + + def _mock_remote_invite_http_error( + self, + status: int, + error_body: JsonDict, + ) -> None: + """ + Make the remote homeserver reply to its `/invite` endpoint with an error. + + Args: + status: the HTTP status to return + error_body: the JSON error body to return + """ + federation_http_client = self.hs.get_federation_http_client() + + fake_agent = create_autospec(Agent, spec_set=True) + + def request( + method: bytes, + uri: bytes, + headers: object = None, + bodyProducer: object = None, + ) -> "defer.Deferred": + # For our test, we don't expect any other outbound request + assert b"/invite/" in uri, f"unexpected outbound request to {uri!r}" + return defer.succeed( + FakeResponse.json( + code=status, + payload=error_body, + ) + ) + + fake_agent.request.side_effect = request + federation_http_client.agent = fake_agent + + @parameterized.expand( + ( + ( + HTTPStatus.IM_A_TEAPOT, + { + "errcode": "M_FORBIDDEN", + "error": "You can't invite this user", + }, + HTTPStatus.IM_A_TEAPOT, + { + "errcode": "M_FORBIDDEN", + "error": "You can't invite this user", + }, + ), + # This case is https://github.com/element-hq/synapse/security/advisories/GHSA-95fh-hv8c-chvq + # The error is rewritten for safety. + ( + HTTPStatus.UNAUTHORIZED, + {"errcode": "M_UNKNOWN_TOKEN", "error": "unknown token"}, + HTTPStatus.BAD_REQUEST, + { + "errcode": "M_UNKNOWN", + "error": "unknown token", + }, + ), + ) + ) + def test_remote_invite_bubbles_errors( + self, + policy_server_error_status: HTTPStatus, + policy_server_error_body: JsonDict, + expected_client_facing_error_status: HTTPStatus, + expected_client_facing_error_body: JsonDict, + ) -> None: + """ + Test that, when creating a room involving a remote invite, + when the remote homeserver returns an error, we bubble it + to the client carefully. + + Regression test for https://github.com/element-hq/synapse/security/advisories/GHSA-95fh-hv8c-chvq + """ + # Mock the remote homeserver (at the HTTP level) to return the configured error + self._mock_remote_invite_http_error( + policy_server_error_status, + policy_server_error_body, + ) + + channel = self.make_request( + "POST", + "/createRoom", + {"invite": ["@alice:" + self.OTHER_SERVER_NAME]}, + access_token=self.token, + ) + + self.assertEqual( + channel.code, expected_client_facing_error_status, channel.result + ) + self.assertEqual(channel.json_body, expected_client_facing_error_body)