From ec766b25303b420850e6d2875f156f23109acf6a Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 29 Jun 2018 10:33:31 +0100 Subject: clarification on what "real names" are --- CONTRIBUTING.rst | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 954758afdc..6c295cfbfe 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -130,11 +130,15 @@ If you agree to this for your contribution, then all that's needed is to include the line in your commit or pull request comment:: Signed-off-by: Your Name - -...using your real name; unfortunately pseudonyms and anonymous contributions -can't be accepted. Git makes this trivial - just use the -s flag when you do -``git commit``, having first set ``user.name`` and ``user.email`` git configs -(which you should have done anyway :) + +We accept contributions under a legally identifiable name, such as +your name on government documentation or common-law names (names +claimed by legitimate usage or repute). Unfortunately, we cannot +accept anonymous contributions at this time. + +Git allows you to add this signoff automatically when using the ``-s`` +flag to ``git commit``, which uses the name and email set in your +``user.name`` and ``user.email`` git configs. Conclusion ~~~~~~~~~~ -- cgit 1.5.1 From 7c0cdd330fe4431488e478a7667c517f13394854 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 29 Jun 2018 14:13:15 +0100 Subject: topfile --- changelog.d/3467.misc | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 changelog.d/3467.misc diff --git a/changelog.d/3467.misc b/changelog.d/3467.misc new file mode 100644 index 0000000000..e69de29bb2 -- cgit 1.5.1 From f131bf8d3e934bb548615214239cfe131f8e33f5 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 1 Jul 2018 05:08:51 +0100 Subject: don't mix unicode strings with utf8-in-byte-strings otherwise we explode with: ``` Traceback (most recent call last): File /usr/lib/python2.7/logging/handlers.py, line 78, in emit logging.FileHandler.emit(self, record) File /usr/lib/python2.7/logging/__init__.py, line 950, in emit StreamHandler.emit(self, record) File /usr/lib/python2.7/logging/__init__.py, line 887, in emit self.handleError(record) File /usr/lib/python2.7/logging/__init__.py, line 810, in handleError None, sys.stderr) File /usr/lib/python2.7/traceback.py, line 124, in print_exception _print(file, 'Traceback (most recent call last):') File /usr/lib/python2.7/traceback.py, line 13, in _print file.write(str+terminator) File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_io.py, line 170, in write self.log.emit(self.level, format=u{log_io}, log_io=line) File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_logger.py, line 144, in emit self.observer(event) File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_observer.py, line 136, in __call__ errorLogger = self._errorLoggerForObserver(brokenObserver) File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_observer.py, line 156, in _errorLoggerForObserver if obs is not observer File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_observer.py, line 81, in __init__ self.log = Logger(observer=self) File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_logger.py, line 64, in __init__ namespace = self._namespaceFromCallingContext() File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/logger/_logger.py, line 42, in _namespaceFromCallingContext return currentframe(2).f_globals[__name__] File /home/matrix/.synapse/local/lib/python2.7/site-packages/twisted/python/compat.py, line 93, in currentframe for x in range(n + 1): RuntimeError: maximum recursion depth exceeded while calling a Python object Logged from file site.py, line 129 File /usr/lib/python2.7/logging/__init__.py, line 859, in emit msg = self.format(record) File /usr/lib/python2.7/logging/__init__.py, line 732, in format return fmt.format(record) File /usr/lib/python2.7/logging/__init__.py, line 471, in format record.message = record.getMessage() File /usr/lib/python2.7/logging/__init__.py, line 335, in getMessage msg = msg % self.args UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 4: ordinal not in range(128) Logged from file site.py, line 129 ``` ...where the logger apparently recurses whilst trying to log the error, hitting the maximum recursion depth and killing everything badly. --- synapse/http/site.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/synapse/http/site.py b/synapse/http/site.py index 74a752d6cf..2ab0d8ff78 100644 --- a/synapse/http/site.py +++ b/synapse/http/site.py @@ -113,7 +113,9 @@ class SynapseRequest(Request): " %sB %s \"%s %s %s\" \"%s\" [%d dbevts]", self.getClientIP(), self.site.site_tag, - self.authenticated_entity, + # need to decode as it could be raw utf-8 bytes + # from a IDN servname in an auth header + self.authenticated_entity.decode("utf-8"), end_time - self.start_time, ru_utime, ru_stime, @@ -125,7 +127,12 @@ class SynapseRequest(Request): self.method, self.get_redacted_uri(), self.clientproto, - self.get_user_agent(), + # need to decode as could be raw utf-8 bytes + # from a utf-8 user-agent. + # N.B. if you don't do this, the logger explodes + # with maximum recursion trying to log errors about + # the charset problem. + self.get_user_agent().decode("utf-8"), evt_db_fetch_count, ) -- cgit 1.5.1 From 1c867f5391bff1b060290f834b177f4c339af65d Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 1 Jul 2018 11:56:33 +0100 Subject: a fix which doesn't NPE everywhere --- synapse/http/site.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/synapse/http/site.py b/synapse/http/site.py index 2ab0d8ff78..14ea9c21c8 100644 --- a/synapse/http/site.py +++ b/synapse/http/site.py @@ -107,15 +107,28 @@ class SynapseRequest(Request): end_time = time.time() + # need to decode as it could be raw utf-8 bytes + # from a IDN servname in an auth header + authenticated_entity = self.authenticated_entity + if authenticated_entity is not None: + authenticated_entity = authenticated_entity.decode("utf-8") + + # ...or could be raw utf-8 bytes in the User-Agent header. + # N.B. if you don't do this, the logger explodes cryptically + # with maximum recursion trying to log errors about + # the charset problem. + # c.f. https://github.com/matrix-org/synapse/issues/3471 + user_agent = self.get_user_agent() + if user_agent is not None: + user_agent = user_agent.decode("utf-8") + self.site.access_logger.info( "%s - %s - {%s}" " Processed request: %.3fsec (%.3fsec, %.3fsec) (%.3fsec/%.3fsec/%d)" " %sB %s \"%s %s %s\" \"%s\" [%d dbevts]", self.getClientIP(), self.site.site_tag, - # need to decode as it could be raw utf-8 bytes - # from a IDN servname in an auth header - self.authenticated_entity.decode("utf-8"), + authenticated_entity, end_time - self.start_time, ru_utime, ru_stime, @@ -127,12 +140,7 @@ class SynapseRequest(Request): self.method, self.get_redacted_uri(), self.clientproto, - # need to decode as could be raw utf-8 bytes - # from a utf-8 user-agent. - # N.B. if you don't do this, the logger explodes - # with maximum recursion trying to log errors about - # the charset problem. - self.get_user_agent().decode("utf-8"), + user_agent, evt_db_fetch_count, ) -- cgit 1.5.1 From fc4f8f33be7bcfa8fca5d1aa1f07a1507e8d56e7 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 2 Jul 2018 11:33:02 +0100 Subject: replace invalid utf8 with \ufffd --- synapse/http/site.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/http/site.py b/synapse/http/site.py index 14ea9c21c8..fe93643b1e 100644 --- a/synapse/http/site.py +++ b/synapse/http/site.py @@ -111,7 +111,7 @@ class SynapseRequest(Request): # from a IDN servname in an auth header authenticated_entity = self.authenticated_entity if authenticated_entity is not None: - authenticated_entity = authenticated_entity.decode("utf-8") + authenticated_entity = authenticated_entity.decode("utf-8", "replace") # ...or could be raw utf-8 bytes in the User-Agent header. # N.B. if you don't do this, the logger explodes cryptically @@ -120,7 +120,7 @@ class SynapseRequest(Request): # c.f. https://github.com/matrix-org/synapse/issues/3471 user_agent = self.get_user_agent() if user_agent is not None: - user_agent = user_agent.decode("utf-8") + user_agent = user_agent.decode("utf-8", "replace") self.site.access_logger.info( "%s - %s - {%s}" -- cgit 1.5.1 From 3905c693c5c256d2380cf8aa94cd24fe64ac974b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Jul 2018 11:36:44 +0100 Subject: Invalidate cache on correct thread --- synapse/storage/events.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index d816d4883c..a54abb9edd 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -801,7 +801,8 @@ class EventsStore(EventsWorkerStore): ] ) - self._curr_state_delta_stream_cache.entity_has_changed( + txn.call_after( + self._curr_state_delta_stream_cache.entity_has_changed, room_id, max_stream_order, ) -- cgit 1.5.1 From cbf82dddf1b8a4dec6f20820faf374f49ab7011c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Jul 2018 11:37:57 +0100 Subject: Ensure that we define sender_domain --- synapse/event_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index f512d88145..9fc8b34346 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -76,6 +76,7 @@ def check(event, auth_events, do_sig_check=True, do_size_check=True): return if event.type == EventTypes.Create: + sender_domain = get_domain_from_id(event.sender) room_id_domain = get_domain_from_id(event.room_id) if room_id_domain != sender_domain: raise AuthError( -- cgit 1.5.1 From 2c33b55738d54186d7655972a6707336b2168534 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Jul 2018 11:39:28 +0100 Subject: Avoid relying on int vs None comparison Python 3 doesn't support comparing None to ints --- synapse/event_auth.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 9fc8b34346..cdf99fd140 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -525,7 +525,11 @@ def _check_power_levels(event, auth_events): "to your own" ) - if old_level > user_level or new_level > user_level: + # Check if the old and new levels are greater than the user level + # (if defined) + old_level_too_big = old_level is not None and old_level > user_level + new_level_too_big = new_level is not None and new_level > user_level + if old_level_too_big or new_level_too_big: raise AuthError( 403, "You don't have permission to add ops level greater " -- cgit 1.5.1 From cea4662b1322b8c17999c39a3f2560d4da120011 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Jul 2018 11:43:58 +0100 Subject: Newsfile --- changelog.d/3473.misc | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 changelog.d/3473.misc diff --git a/changelog.d/3473.misc b/changelog.d/3473.misc new file mode 100644 index 0000000000..e69de29bb2 -- cgit 1.5.1 From f88dea577d0231b5fd090d4b6046c9a391c5221b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Jul 2018 11:43:31 +0100 Subject: Newsfile --- changelog.d/3474.misc | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 changelog.d/3474.misc diff --git a/changelog.d/3474.misc b/changelog.d/3474.misc new file mode 100644 index 0000000000..e69de29bb2 -- cgit 1.5.1 From abb183438c7294d7599f6a6bcdb2ae5f55448e58 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Jul 2018 13:12:38 +0100 Subject: Correct newsfile --- changelog.d/3473.bugfix | 1 + changelog.d/3473.misc | 0 2 files changed, 1 insertion(+) create mode 100644 changelog.d/3473.bugfix delete mode 100644 changelog.d/3473.misc diff --git a/changelog.d/3473.bugfix b/changelog.d/3473.bugfix new file mode 100644 index 0000000000..7244ab59f2 --- /dev/null +++ b/changelog.d/3473.bugfix @@ -0,0 +1 @@ +Invalidate cache on correct thread to avoid race diff --git a/changelog.d/3473.misc b/changelog.d/3473.misc deleted file mode 100644 index e69de29bb2..0000000000 -- cgit 1.5.1 From 6ec3aa2f72b2a0cc8d58a2448de7d7af5ed5f05e Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 2 Jul 2018 13:43:34 +0100 Subject: news snippet --- changelog.d/3470.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3470.bugfix diff --git a/changelog.d/3470.bugfix b/changelog.d/3470.bugfix new file mode 100644 index 0000000000..1308931191 --- /dev/null +++ b/changelog.d/3470.bugfix @@ -0,0 +1 @@ +Fix bug where synapse would explode when receiving unicode in HTTP User-Agent header -- cgit 1.5.1 From 508196e08a834496daa1bfc5f561e69a430e270c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 3 Jul 2018 14:36:14 +0100 Subject: Reject invalid server names (#3480) Make sure that server_names used in auth headers are sane, and reject them with a sensible error code, before they disappear off into the depths of the system. --- changelog.d/3480.feature | 1 + synapse/federation/transport/server.py | 66 ++++++++++++++++++++++------------ synapse/http/endpoint.py | 34 ++++++++++++++++-- tests/http/__init__.py | 0 tests/http/test_endpoint.py | 46 ++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 25 deletions(-) create mode 100644 changelog.d/3480.feature create mode 100644 tests/http/__init__.py create mode 100644 tests/http/test_endpoint.py diff --git a/changelog.d/3480.feature b/changelog.d/3480.feature new file mode 100644 index 0000000000..a21580943d --- /dev/null +++ b/changelog.d/3480.feature @@ -0,0 +1 @@ +Reject invalid server names in federation requests diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 19d09f5422..1180d4b69d 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -18,6 +18,7 @@ from twisted.internet import defer from synapse.api.urls import FEDERATION_PREFIX as PREFIX from synapse.api.errors import Codes, SynapseError, FederationDeniedError +from synapse.http.endpoint import parse_server_name from synapse.http.server import JsonResource from synapse.http.servlet import ( parse_json_object_from_request, parse_integer_from_args, parse_string_from_args, @@ -99,26 +100,6 @@ class Authenticator(object): origin = None - def parse_auth_header(header_str): - try: - params = auth.split(" ")[1].split(",") - param_dict = dict(kv.split("=") for kv in params) - - def strip_quotes(value): - if value.startswith("\""): - return value[1:-1] - else: - return value - - origin = strip_quotes(param_dict["origin"]) - key = strip_quotes(param_dict["key"]) - sig = strip_quotes(param_dict["sig"]) - return (origin, key, sig) - except Exception: - raise AuthenticationError( - 400, "Malformed Authorization header", Codes.UNAUTHORIZED - ) - auth_headers = request.requestHeaders.getRawHeaders(b"Authorization") if not auth_headers: @@ -127,8 +108,8 @@ class Authenticator(object): ) for auth in auth_headers: - if auth.startswith("X-Matrix"): - (origin, key, sig) = parse_auth_header(auth) + if auth.startswith(b"X-Matrix"): + (origin, key, sig) = _parse_auth_header(auth) json_request["origin"] = origin json_request["signatures"].setdefault(origin, {})[key] = sig @@ -165,6 +146,47 @@ class Authenticator(object): logger.exception("Error resetting retry timings on %s", origin) +def _parse_auth_header(header_bytes): + """Parse an X-Matrix auth header + + Args: + header_bytes (bytes): header value + + Returns: + Tuple[str, str, str]: origin, key id, signature. + + Raises: + AuthenticationError if the header could not be parsed + """ + try: + header_str = header_bytes.decode('utf-8') + params = header_str.split(" ")[1].split(",") + param_dict = dict(kv.split("=") for kv in params) + + def strip_quotes(value): + if value.startswith(b"\""): + return value[1:-1] + else: + return value + + origin = strip_quotes(param_dict["origin"]) + # ensure that the origin is a valid server name + parse_server_name(origin) + + key = strip_quotes(param_dict["key"]) + sig = strip_quotes(param_dict["sig"]) + return origin, key, sig + except Exception as e: + logger.warn( + "Error parsing auth header '%s': %s", + header_bytes.decode('ascii', 'replace'), + e, + ) + raise AuthenticationError( + 400, "Malformed Authorization header", Codes.UNAUTHORIZED, + ) + + class BaseFederationServlet(object): REQUIRE_AUTH = True diff --git a/synapse/http/endpoint.py b/synapse/http/endpoint.py index 80da870584..5a9cbb3324 100644 --- a/synapse/http/endpoint.py +++ b/synapse/http/endpoint.py @@ -38,6 +38,36 @@ _Server = collections.namedtuple( ) +def parse_server_name(server_name): + """Split a server name into host/port parts. + + Does some basic sanity checking of the + + Args: + server_name (str): server name to parse + + Returns: + Tuple[str, int|None]: host/port parts. + + Raises: + ValueError if the server name could not be parsed. + """ + try: + if server_name[-1] == ']': + # ipv6 literal, hopefully + if server_name[0] != '[': + raise Exception() + + return server_name, None + + domain_port = server_name.rsplit(":", 1) + domain = domain_port[0] + port = int(domain_port[1]) if domain_port[1:] else None + return domain, port + except Exception: + raise ValueError("Invalid server name '%s'" % server_name) + + def matrix_federation_endpoint(reactor, destination, ssl_context_factory=None, timeout=None): """Construct an endpoint for the given matrix destination. @@ -50,9 +80,7 @@ def matrix_federation_endpoint(reactor, destination, ssl_context_factory=None, timeout (int): connection timeout in seconds """ - domain_port = destination.split(":") - domain = domain_port[0] - port = int(domain_port[1]) if domain_port[1:] else None + domain, port = parse_server_name(destination) endpoint_kw_args = {} diff --git a/tests/http/__init__.py b/tests/http/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/http/test_endpoint.py b/tests/http/test_endpoint.py new file mode 100644 index 0000000000..cd74825c85 --- /dev/null +++ b/tests/http/test_endpoint.py @@ -0,0 +1,46 @@ +# -*- coding: utf-8 -*- +# Copyright 2018 New Vector Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from synapse.http.endpoint import parse_server_name +from tests import unittest + + +class ServerNameTestCase(unittest.TestCase): + def test_parse_server_name(self): + test_data = { + 'localhost': ('localhost', None), + 'my-example.com:1234': ('my-example.com', 1234), + '1.2.3.4': ('1.2.3.4', None), + '[0abc:1def::1234]': ('[0abc:1def::1234]', None), + '1.2.3.4:1': ('1.2.3.4', 1), + '[0abc:1def::1234]:8080': ('[0abc:1def::1234]', 8080), + } + + for i, o in test_data.items(): + self.assertEqual(parse_server_name(i), o) + + def test_parse_bad_server_names(self): + test_data = [ + "", # empty + "localhost:http", # non-numeric port + "1234]", # smells like ipv6 literal but isn't + ] + for i in test_data: + try: + parse_server_name(i) + self.fail( + "Expected parse_server_name(\"%s\") to throw" % i, + ) + except ValueError: + pass -- cgit 1.5.1 From ea555d56331ad01edc9871ec7bf879df7d24dc7d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Jul 2018 09:35:40 +0100 Subject: Reinstate lost run_on_reactor in unit test a61738b removed a call to run_on_reactor from a unit test, but that call was doing something useful, in making the function in question asynchronous. Reinstate the call and add a check that we are testing what we wanted to be testing. --- changelog.d/3385.misc | 1 + tests/util/caches/test_descriptors.py | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 changelog.d/3385.misc diff --git a/changelog.d/3385.misc b/changelog.d/3385.misc new file mode 100644 index 0000000000..92a91a1ca5 --- /dev/null +++ b/changelog.d/3385.misc @@ -0,0 +1 @@ +Reinstate lost run_on_reactor in unit tests diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py index 24754591df..a94d566c96 100644 --- a/tests/util/caches/test_descriptors.py +++ b/tests/util/caches/test_descriptors.py @@ -19,13 +19,19 @@ import logging import mock from synapse.api.errors import SynapseError from synapse.util import logcontext -from twisted.internet import defer +from twisted.internet import defer, reactor from synapse.util.caches import descriptors from tests import unittest logger = logging.getLogger(__name__) +def run_on_reactor(): + d = defer.Deferred() + reactor.callLater(0, d.callback, 0) + return logcontext.make_deferred_yieldable(d) + + class CacheTestCase(unittest.TestCase): def test_invalidate_all(self): cache = descriptors.Cache("testcache") @@ -194,6 +200,8 @@ class DescriptorTestCase(unittest.TestCase): def fn(self, arg1): @defer.inlineCallbacks def inner_fn(): + # we want this to behave like an asynchronous function + yield run_on_reactor() raise SynapseError(400, "blah") return inner_fn() @@ -203,7 +211,12 @@ class DescriptorTestCase(unittest.TestCase): with logcontext.LoggingContext() as c1: c1.name = "c1" try: - yield obj.fn(1) + d = obj.fn(1) + self.assertEqual( + logcontext.LoggingContext.current_context(), + logcontext.LoggingContext.sentinel, + ) + yield d self.fail("No exception thrown") except SynapseError: pass -- cgit 1.5.1 From 546bc9e28b3d7758c732df8e120639d58d455164 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Jul 2018 18:15:03 +0100 Subject: More server_name validation We need to do a bit more validation when we get a server name, but don't want to be re-doing it all over the shop, so factor out a separate parse_and_validate_server_name, and do the extra validation. Also, use it to verify the server name in the config file. --- changelog.d/3483.feature | 1 + synapse/config/server.py | 11 ++++++-- synapse/federation/transport/server.py | 5 ++-- synapse/http/endpoint.py | 47 ++++++++++++++++++++++++++++++---- tests/http/test_endpoint.py | 17 +++++++++--- 5 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 changelog.d/3483.feature diff --git a/changelog.d/3483.feature b/changelog.d/3483.feature new file mode 100644 index 0000000000..afa2fbbcba --- /dev/null +++ b/changelog.d/3483.feature @@ -0,0 +1 @@ +Reject invalid server names in homeserver.yaml \ No newline at end of file diff --git a/synapse/config/server.py b/synapse/config/server.py index 968ecd9ea0..71fd51e4bc 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -16,6 +16,7 @@ import logging +from synapse.http.endpoint import parse_and_validate_server_name from ._base import Config, ConfigError logger = logging.Logger(__name__) @@ -25,6 +26,12 @@ class ServerConfig(Config): def read_config(self, config): self.server_name = config["server_name"] + + try: + parse_and_validate_server_name(self.server_name) + except ValueError as e: + raise ConfigError(str(e)) + self.pid_file = self.abspath(config.get("pid_file")) self.web_client = config["web_client"] self.web_client_location = config.get("web_client_location", None) @@ -162,8 +169,8 @@ class ServerConfig(Config): }) def default_config(self, server_name, **kwargs): - if ":" in server_name: - bind_port = int(server_name.split(":")[1]) + _, bind_port = parse_and_validate_server_name(server_name) + if bind_port is not None: unsecure_port = bind_port - 400 else: bind_port = 8448 diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 1180d4b69d..e1fdcc89dc 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -18,7 +18,7 @@ from twisted.internet import defer from synapse.api.urls import FEDERATION_PREFIX as PREFIX from synapse.api.errors import Codes, SynapseError, FederationDeniedError -from synapse.http.endpoint import parse_server_name +from synapse.http.endpoint import parse_and_validate_server_name from synapse.http.server import JsonResource from synapse.http.servlet import ( parse_json_object_from_request, parse_integer_from_args, parse_string_from_args, @@ -170,8 +170,9 @@ def _parse_auth_header(header_bytes): return value origin = strip_quotes(param_dict["origin"]) + # ensure that the origin is a valid server name - parse_server_name(origin) + parse_and_validate_server_name(origin) key = strip_quotes(param_dict["key"]) sig = strip_quotes(param_dict["sig"]) diff --git a/synapse/http/endpoint.py b/synapse/http/endpoint.py index 5a9cbb3324..1b1123b292 100644 --- a/synapse/http/endpoint.py +++ b/synapse/http/endpoint.py @@ -12,6 +12,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import re + from twisted.internet.endpoints import HostnameEndpoint, wrapClientTLS from twisted.internet import defer from twisted.internet.error import ConnectError @@ -41,8 +43,6 @@ _Server = collections.namedtuple( def parse_server_name(server_name): """Split a server name into host/port parts. - Does some basic sanity checking of the - Args: server_name (str): server name to parse @@ -55,9 +55,6 @@ def parse_server_name(server_name): try: if server_name[-1] == ']': # ipv6 literal, hopefully - if server_name[0] != '[': - raise Exception() - return server_name, None domain_port = server_name.rsplit(":", 1) @@ -68,6 +65,46 @@ def parse_server_name(server_name): raise ValueError("Invalid server name '%s'" % server_name) +VALID_HOST_REGEX = re.compile( + "\\A[0-9a-zA-Z.-]+\\Z", +) + + +def parse_and_validate_server_name(server_name): + """Split a server name into host/port parts and do some basic validation. + + Args: + server_name (str): server name to parse + + Returns: + Tuple[str, int|None]: host/port parts. + + Raises: + ValueError if the server name could not be parsed. + """ + host, port = parse_server_name(server_name) + + # these tests don't need to be bulletproof as we'll find out soon enough + # if somebody is giving us invalid data. What we *do* need is to be sure + # that nobody is sneaking IP literals in that look like hostnames, etc. + + # look for ipv6 literals + if host[0] == '[': + if host[-1] != ']': + raise ValueError("Mismatched [...] in server name '%s'" % ( + server_name, + )) + return host, port + + # otherwise it should only be alphanumerics. + if not VALID_HOST_REGEX.match(host): + raise ValueError("Server name '%s' contains invalid characters" % ( + server_name, + )) + + return host, port + + def matrix_federation_endpoint(reactor, destination, ssl_context_factory=None, timeout=None): """Construct an endpoint for the given matrix destination. diff --git a/tests/http/test_endpoint.py b/tests/http/test_endpoint.py index cd74825c85..b8a48d20a4 100644 --- a/tests/http/test_endpoint.py +++ b/tests/http/test_endpoint.py @@ -12,7 +12,10 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from synapse.http.endpoint import parse_server_name +from synapse.http.endpoint import ( + parse_server_name, + parse_and_validate_server_name, +) from tests import unittest @@ -30,17 +33,23 @@ class ServerNameTestCase(unittest.TestCase): for i, o in test_data.items(): self.assertEqual(parse_server_name(i), o) - def test_parse_bad_server_names(self): + def test_validate_bad_server_names(self): test_data = [ "", # empty "localhost:http", # non-numeric port "1234]", # smells like ipv6 literal but isn't + "[1234", + "underscore_.com", + "percent%65.com", + "1234:5678:80", # too many colons ] for i in test_data: try: - parse_server_name(i) + parse_and_validate_server_name(i) self.fail( - "Expected parse_server_name(\"%s\") to throw" % i, + "Expected parse_and_validate_server_name('%s') to throw" % ( + i, + ), ) except ValueError: pass -- cgit 1.5.1