From 4f332f6c2739b7b6c3fd74561f35e8a7b053d096 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 13 Apr 2021 16:12:30 +0200 Subject: [PATCH 1/5] Sanity check identity server passed to bind/unbind. Signed-off-by: Denis Kasak --- synapse/handlers/identity.py | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index d89fa5fb305d..ef51c79bf263 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -16,7 +16,6 @@ # limitations under the License. """Utilities for interacting with Identity Servers""" - import logging import urllib.parse from typing import Awaitable, Callable, Dict, List, Optional, Tuple @@ -35,7 +34,11 @@ from synapse.types import JsonDict, Requester from synapse.util import json_decoder from synapse.util.hash import sha256_and_url_safe_base64 -from synapse.util.stringutils import assert_valid_client_secret, random_string +from synapse.util.stringutils import ( + assert_valid_client_secret, + parse_and_validate_server_name, + random_string, +) from ._base import BaseHandler @@ -74,10 +77,7 @@ def __init__(self, hs): ) async def ratelimit_request_token_requests( - self, - request: SynapseRequest, - medium: str, - address: str, + self, request: SynapseRequest, medium: str, address: str, ): """Used to ratelimit requests to `/requestToken` by IP and address. @@ -173,6 +173,11 @@ async def bind_threepid( server with, if necessary. Required if use_v2 is true use_v2: Whether to use v2 Identity Service API endpoints. Defaults to True + Raises: + SynapseError: On any of the following conditions + - the supplied id_server is not a valid Matrix server name + - we failed to contact the supplied identity server + Returns: The response from the identity server """ @@ -182,6 +187,11 @@ async def bind_threepid( if id_access_token is None: use_v2 = False + try: + parse_and_validate_server_name(id_server) + except ValueError: + raise SynapseError(400, "id_server must be a valid Matrix server name") + # Decide which API endpoint URLs to use headers = {} bind_data = {"sid": sid, "client_secret": client_secret, "mxid": mxid} @@ -270,12 +280,20 @@ async def try_unbind_threepid_with_id_server( id_server: Identity server to unbind from Raises: - SynapseError: If we failed to contact the identity server + SynapseError: On any of the following conditions + - the supplied id_server is not a valid Matrix server name + - we failed to contact the supplied identity server Returns: True on success, otherwise False if the identity server doesn't support unbinding """ + + try: + parse_and_validate_server_name(id_server) + except ValueError: + raise SynapseError(400, "id_server must be a valid Matrix server name") + url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,) url_bytes = "/_matrix/identity/api/v1/3pid/unbind".encode("ascii") From 0a8bc1df805a6454db981d268a8a0e76e04d6654 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 13 Apr 2021 17:16:57 +0200 Subject: [PATCH 2/5] Add changelog. --- changelog.d/9802.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9802.bugfix diff --git a/changelog.d/9802.bugfix b/changelog.d/9802.bugfix new file mode 100644 index 000000000000..0c72f7be473f --- /dev/null +++ b/changelog.d/9802.bugfix @@ -0,0 +1 @@ +Add some sanity checks to identity server passed to 3PID bind/unbind endpoints. From 3ac6fe5f631f470049f094e716c5287bf4d13375 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 13 Apr 2021 17:30:59 +0200 Subject: [PATCH 3/5] Reformat with new version of black. --- synapse/handlers/identity.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index ef51c79bf263..eb33c089c8c2 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -77,7 +77,10 @@ def __init__(self, hs): ) async def ratelimit_request_token_requests( - self, request: SynapseRequest, medium: str, address: str, + self, + request: SynapseRequest, + medium: str, + address: str, ): """Used to ratelimit requests to `/requestToken` by IP and address. From f57b424d861201278b133240b6febd192fbd3334 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 16 Apr 2021 12:25:42 +0200 Subject: [PATCH 4/5] Allow optional path components, to serve as a root path. --- synapse/handlers/identity.py | 24 +++++++++++++----------- synapse/util/stringutils.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index eb33c089c8c2..8be9ba475363 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -36,8 +36,8 @@ from synapse.util.hash import sha256_and_url_safe_base64 from synapse.util.stringutils import ( assert_valid_client_secret, - parse_and_validate_server_name, random_string, + valid_id_server_location, ) from ._base import BaseHandler @@ -178,7 +178,7 @@ async def bind_threepid( Raises: SynapseError: On any of the following conditions - - the supplied id_server is not a valid Matrix server name + - the supplied id_server is not a valid identity server name - we failed to contact the supplied identity server Returns: @@ -190,10 +190,11 @@ async def bind_threepid( if id_access_token is None: use_v2 = False - try: - parse_and_validate_server_name(id_server) - except ValueError: - raise SynapseError(400, "id_server must be a valid Matrix server name") + if not valid_id_server_location(id_server): + raise SynapseError( + 400, + "id_server must be a valid hostname with optional port and path components", + ) # Decide which API endpoint URLs to use headers = {} @@ -284,7 +285,7 @@ async def try_unbind_threepid_with_id_server( Raises: SynapseError: On any of the following conditions - - the supplied id_server is not a valid Matrix server name + - the supplied id_server is not a valid identity server name - we failed to contact the supplied identity server Returns: @@ -292,10 +293,11 @@ async def try_unbind_threepid_with_id_server( server doesn't support unbinding """ - try: - parse_and_validate_server_name(id_server) - except ValueError: - raise SynapseError(400, "id_server must be a valid Matrix server name") + if not valid_id_server_location(id_server): + raise SynapseError( + 400, + "id_server must be a valid hostname with optional port and path components", + ) url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,) url_bytes = "/_matrix/identity/api/v1/3pid/unbind".encode("ascii") diff --git a/synapse/util/stringutils.py b/synapse/util/stringutils.py index 9ce7873ab573..678a7392353e 100644 --- a/synapse/util/stringutils.py +++ b/synapse/util/stringutils.py @@ -133,6 +133,35 @@ def parse_and_validate_server_name(server_name: str) -> Tuple[str, Optional[int] return host, port +def valid_id_server_location(id_server: str) -> bool: + """Check whether an identity server location, such as the one passed as the + `id_server` parameter to `/_matrix/client/r0/account/3pid/bind`, is valid. + + A valid identity server location consists of a valid hostname and optional + port number, optionally followed by any number of `/` delimited path + components, without any fragment or query string parts. + + Args: + id_server: identity server location string to validate + + Returns: + True if valid, False otherwise. + """ + + components = id_server.split("/") + + host = components[0] + + try: + parse_and_validate_server_name(host) + except ValueError: + return False + + path = components[1:] + + return not path or all("#" not in p and "?" not in p for p in path) + + def parse_and_validate_mxc_uri(mxc: str) -> Tuple[str, Optional[int], str]: """Parse the given string as an MXC URI From 2de7474eb0ca613eb31e977e0bc9b140fa7d9271 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 16 Apr 2021 15:26:56 +0200 Subject: [PATCH 5/5] Simplify path check. Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/util/stringutils.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/synapse/util/stringutils.py b/synapse/util/stringutils.py index 678a7392353e..ba25bf15d238 100644 --- a/synapse/util/stringutils.py +++ b/synapse/util/stringutils.py @@ -148,7 +148,7 @@ def valid_id_server_location(id_server: str) -> bool: True if valid, False otherwise. """ - components = id_server.split("/") + components = id_server.split("/", 1) host = components[0] @@ -157,9 +157,12 @@ def valid_id_server_location(id_server: str) -> bool: except ValueError: return False - path = components[1:] + if len(components) < 2: + # no path + return True - return not path or all("#" not in p and "?" not in p for p in path) + path = components[1] + return "#" not in path and "?" not in path def parse_and_validate_mxc_uri(mxc: str) -> Tuple[str, Optional[int], str]: