Apply extra checks to incoming requests

Author: richvdh

synapse/api/constants.py

@@ -17,6 +17,9 @@
 
 """Contains constants from the specification."""
 
+# the max size of a (canonical-json-encoded) event
+MAX_PDU_SIZE = 65536
+
 # the "depth" field on events is limited to 2**63 - 1
 MAX_DEPTH = 2 ** 63 - 1
 

synapse/event_auth.py

@@ -21,7 +21,7 @@
 from signedjson.sign import SignatureVerifyException, verify_signed_json
 from unpaddedbase64 import decode_base64
 
-from synapse.api.constants import EventTypes, JoinRules, Membership
+from synapse.api.constants import MAX_PDU_SIZE, EventTypes, JoinRules, Membership
 from synapse.api.errors import AuthError, EventSizeError, SynapseError
 from synapse.api.room_versions import (
     KNOWN_ROOM_VERSIONS,
@@ -205,7 +205,7 @@ def too_big(field):
         too_big("type")
     if len(event.event_id) > 255:
         too_big("event_id")
-    if len(encode_canonical_json(event.get_pdu_json())) > 65536:
+    if len(encode_canonical_json(event.get_pdu_json())) > MAX_PDU_SIZE:
         too_big("event")
 
 

synapse/http/site.py

@@ -23,6 +23,7 @@
 from twisted.python.failure import Failure
 from twisted.web.server import Request, Site
 
+from synapse.api.constants import MAX_PDU_SIZE
 from synapse.config.server import ListenerConfig
 from synapse.http import get_request_user_agent, redact_uri
 from synapse.http.request_metrics import RequestMetrics, requests_counter
@@ -59,6 +60,16 @@ class SynapseRequest(Request):
         logcontext: the log context for this request
     """
 
+    # the biggest request we expect to see is a fully-loaded federation/send request.
+    # Other than a bit of metadata, the main thing in such a request is up to 50 PDUs,
+    # and up to 100 EDUs. PDUs are limited to 65535 bytes (possibly slightly more if
+    # the sender didn't use canonical encoding); there is no specced limit to EDUs
+    # (see https://github.com/matrix-org/matrix-doc/issues/3121).
+    #
+    # in short, we somewhat arbitrarily limit requests to 200 * 64K.
+    #
+    MAX_REQUEST_SIZE = 200 * MAX_PDU_SIZE
+
     def __init__(self, channel, *args, **kw):
         Request.__init__(self, channel, *args, **kw)
         self.site = channel.site  # type: SynapseSite
@@ -97,6 +108,16 @@ def __repr__(self):
             self.site.site_tag,
         )
 
+    def handleContentChunk(self, data):
+        if self.content.tell() + len(data) > self.MAX_REQUEST_SIZE:
+            logger.warning(
+                "Aborting connection from %s because the request exceeds maximum size",
+                self.client,
+            )
+            self.transport.abortConnection()
+            return
+        super().handleContentChunk(data)
+
     @property
     def requester(self) -> Optional[Union[Requester, str]]:
         return self._requester

tests/http/test_site.py

@@ -0,0 +1,95 @@
+# Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+# 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.
+import twisted.internet.base
+from twisted.internet.address import IPv6Address
+from twisted.internet.testing import StringTransport
+
+from synapse.app.homeserver import SynapseHomeServer
+
+from tests.unittest import HomeserverTestCase
+
+
+class SynapseRequestTestCase(HomeserverTestCase):
+    def make_homeserver(self, reactor, clock):
+        return self.setup_test_homeserver(homeserver_to_use=SynapseHomeServer)
+
+    def test_large_request(self):
+        """overlarge HTTP requests should be rejected"""
+        self.hs.start_listening()
+
+        # find the HTTP server which is configured to listen on port 0
+        (port, factory, _backlog, interface) = self.reactor.tcpServers[0]
+        self.assertEqual(interface, "::")
+        self.assertEqual(port, 0)
+
+        # as a control case, first send a regular request.
+
+        # complete the connection and wire it up to a fake transport
+        client_address = IPv6Address("TCP", "::1", "2345")
+        protocol = factory.buildProtocol(client_address)
+        transport = StringTransport()
+        protocol.makeConnection(transport)
+
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Transfer-Encoding: chunked\r\n"
+            b"\r\n"
+            b"0\r\n"
+            b"\r\n"
+        )
+
+        while not transport.disconnecting:
+            self.reactor.advance(1)
+
+        # we should get a 404
+        self.assertRegex(transport.value().decode(), r"^HTTP/1\.1 404 ")
+
+        # now send an oversized request
+        protocol = factory.buildProtocol(client_address)
+        transport = StringTransport()
+        protocol.makeConnection(transport)
+
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Transfer-Encoding: chunked\r\n"
+            b"\r\n"
+        )
+
+        # we deliberately send all the data in one big chunk, to ensure that
+        # twisted isn't buffering the data in the chunked transfer decoder.
+        protocol.dataReceived(b"10000000\r\n")
+        for i in range(0, 0x1000):
+            protocol.dataReceived(b"\0" * 0x1000)
+
+        self.assertTrue(transport.disconnected, "connection did not drop")
+
+        # this is a bit of a hack. The problem is that the HTTPChannel sets a timeout
+        # on receiving the request, which would normally be cleared once the entire
+        # request is received. Of course, the entire request is *not* received, so
+        # the timeout is never cancelled.
+        #
+        # Ideally then, we would tick the reactor past that timeout, to check what
+        # happens. Unfortunately, twisted's `TimeoutMixin` (as used by HTTPChannel)
+        # seems to be hardcoded to use the default twisted reactor, so the timeout is
+        # set on the real reactor, which means we'd have to *actually* sleep here while
+        # we wait for the timeout to elapse.
+        #
+        # So instead, let's just gut-wrench into twisted to cancel the timeout
+        # ourselves.
+        #
+        # (Note that `protocol` here is actually a _GenericHTTPChannelProtocol - the
+        # real HTTPChannel is `protocol._channel`)
+        protocol._channel.setTimeout(None)