Merge pull request #3483 from matrix-org/rav/more_server_name_validation

More server_name validation

5 files changed, 68 insertions, 13 deletions

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