From 8739c0df915525d783b7955727a6a760b7bc2a41 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Fri, 15 Jul 2022 20:16:30 -0700 Subject: [PATCH 1/6] validate destinations --- synapse/http/matrixfederationclient.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index c63d068f74c6..d0cb992b9b21 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -79,6 +79,7 @@ from synapse.util import json_decoder from synapse.util.async_helpers import AwakenableSleeper, timeout_deferred from synapse.util.metrics import Measure +from synapse.util.stringutils import parse_and_validate_server_name if TYPE_CHECKING: from synapse.server import HomeServer @@ -97,7 +98,6 @@ MAX_SHORT_RETRIES = 3 MAXINT = sys.maxsize - _next_id = 1 T = TypeVar("T") @@ -479,6 +479,14 @@ async def _send_request( RequestSendFailed: If there were problems connecting to the remote, due to e.g. DNS failures, connection timeouts etc. """ + # Validate server name and log if it is an invalid destination, this is + # partially to help track down code paths where we haven't validated before here + try: + parse_and_validate_server_name(request.destination) + except ValueError: + logger.info(f"Invalid destination: {request.destination}") + raise FederationDeniedError(request.destination) + if timeout: _sec_timeout = timeout / 1000 else: @@ -557,7 +565,6 @@ async def _send_request( ) headers_dict[b"Authorization"] = auth_headers - logger.debug( "{%s} [%s] Sending request: %s %s; timeout %fs", request.txn_id, From 7f0c89d801d20cd354c10c700361ded885454871 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Fri, 15 Jul 2022 20:16:51 -0700 Subject: [PATCH 2/6] update test to pass destination validation --- tests/federation/test_federation_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/federation/test_federation_client.py b/tests/federation/test_federation_client.py index d2bda0719872..cf6b130e4fd8 100644 --- a/tests/federation/test_federation_client.py +++ b/tests/federation/test_federation_client.py @@ -102,7 +102,7 @@ def test_get_room_state(self): # now fire off the request state_resp, auth_resp = self.get_success( self.hs.get_federation_client().get_room_state( - "yet_another_server", + "yet.another.server", test_room_id, "event_id", RoomVersions.V9, @@ -112,7 +112,7 @@ def test_get_room_state(self): # check the right call got made to the agent self._mock_agent.request.assert_called_once_with( b"GET", - b"matrix://yet_another_server/_matrix/federation/v1/state/%21room_id?event_id=event_id", + b"matrix://yet.another.server/_matrix/federation/v1/state/%21room_id?event_id=event_id", headers=mock.ANY, bodyProducer=None, ) From b468a3cbb34e5ba90664155e119d705ffc4c58c4 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 18 Jul 2022 12:54:13 -0700 Subject: [PATCH 3/6] newsfragment + typo --- changelog.d/13318.misc | 1 + synapse/http/matrixfederationclient.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/13318.misc diff --git a/changelog.d/13318.misc b/changelog.d/13318.misc new file mode 100644 index 000000000000..0b7b02d71dfc --- /dev/null +++ b/changelog.d/13318.misc @@ -0,0 +1 @@ +Validate federation destination and log an error if destination is invalid. diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index d0cb992b9b21..1bd0a54694c0 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -484,7 +484,7 @@ async def _send_request( try: parse_and_validate_server_name(request.destination) except ValueError: - logger.info(f"Invalid destination: {request.destination}") + logger.info(f"Invalid destination: {request.destination}.") raise FederationDeniedError(request.destination) if timeout: From 79745f470c2c877dab2c2016753be1b8ff626789 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 18 Jul 2022 13:02:31 -0700 Subject: [PATCH 4/6] random line changes fix --- synapse/http/matrixfederationclient.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1bd0a54694c0..49c7e09b83f5 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -98,6 +98,7 @@ MAX_SHORT_RETRIES = 3 MAXINT = sys.maxsize + _next_id = 1 T = TypeVar("T") @@ -565,6 +566,7 @@ async def _send_request( ) headers_dict[b"Authorization"] = auth_headers + logger.debug( "{%s} [%s] Sending request: %s %s; timeout %fs", request.txn_id, From a85c648206fdbb6cc77ce5fb3d5cc4246e76b533 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 20 Jul 2022 09:21:52 -0700 Subject: [PATCH 5/6] suggested changes --- changelog.d/13318.misc | 2 +- synapse/http/matrixfederationclient.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/changelog.d/13318.misc b/changelog.d/13318.misc index 0b7b02d71dfc..f5cd26b862d4 100644 --- a/changelog.d/13318.misc +++ b/changelog.d/13318.misc @@ -1 +1 @@ -Validate federation destination and log an error if destination is invalid. +Validate federation destinations and log an error if a destination is invalid. diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 49c7e09b83f5..fb7240e8d6db 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -485,7 +485,9 @@ async def _send_request( try: parse_and_validate_server_name(request.destination) except ValueError: - logger.info(f"Invalid destination: {request.destination}.") + logger.error( + f"Invalid destination: {request.destination}.", stack_info=True + ) raise FederationDeniedError(request.destination) if timeout: From 30d55038b059e417ad72f9ace2a6eb7ee98fc24f Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 20 Jul 2022 10:28:41 -0700 Subject: [PATCH 6/6] error -> exception --- synapse/http/matrixfederationclient.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fb7240e8d6db..3c35b1d2c7af 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -485,9 +485,7 @@ async def _send_request( try: parse_and_validate_server_name(request.destination) except ValueError: - logger.error( - f"Invalid destination: {request.destination}.", stack_info=True - ) + logger.exception(f"Invalid destination: {request.destination}.") raise FederationDeniedError(request.destination) if timeout: