Add HTTP 413 response when incoming request is too large

changelog.d/19212.misc

@@ -0,0 +1 @@
+Add HTTP 413 response when incoming request is too large.

synapse/api/constants.py

@@ -29,6 +29,19 @@
 # the max size of a (canonical-json-encoded) event
 MAX_PDU_SIZE = 65536
 
+# The maximum allowed size of an HTTP request.
+# Other than media uploads, the biggest request we expect to see is a fully-loaded
+# /federation/v1/send request.
+#
+# The main thing in such a request is up to 50 PDUs, and up to 100 EDUs. PDUs are
+# limited to 65536 bytes (possibly slightly more if the sender didn't use canonical
+# json 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 (about 12.5M)
+#
+MAX_REQUEST_SIZE = 200 * MAX_PDU_SIZE
+
 # Max/min size of ints in canonical JSON
 CANONICALJSON_MAX_INT = (2**53) - 1
 CANONICALJSON_MIN_INT = -CANONICALJSON_MAX_INT

synapse/app/_base.py

@@ -59,7 +59,7 @@
 from twisted.web.resource import Resource
 
 import synapse.util.caches
-from synapse.api.constants import MAX_PDU_SIZE
+from synapse.api.constants import MAX_REQUEST_SIZE
 from synapse.app import check_bind_error
 from synapse.config import ConfigError
 from synapse.config._base import format_config_error
@@ -843,17 +843,7 @@ def sdnotify(state: bytes) -> None:
 def max_request_body_size(config: HomeServerConfig) -> int:
     """Get a suitable maximum size for incoming HTTP requests"""
 
-    # Other than media uploads, the biggest request we expect to see is a fully-loaded
-    # /federation/v1/send request.
-    #
-    # The main thing in such a request is up to 50 PDUs, and up to 100 EDUs. PDUs are
-    # limited to 65536 bytes (possibly slightly more if the sender didn't use canonical
-    # json 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 (about 12.5M)
-    #
-    max_request_size = 200 * MAX_PDU_SIZE
+    max_request_size = MAX_REQUEST_SIZE
 
     # if we have a media repo enabled, we may need to allow larger uploads than that
     if config.media.can_load_media_repo:

synapse/http/site.py

@@ -19,6 +19,7 @@
 #
 #
 import contextlib
+import json
 import logging
 import time
 from http import HTTPStatus
@@ -36,6 +37,7 @@
 from twisted.web.resource import IResource, Resource
 from twisted.web.server import Request
 
+from synapse.api.errors import Codes
 from synapse.config.server import ListenerConfig
 from synapse.http import get_request_user_agent, redact_uri
 from synapse.http.proxy import ProxySite
@@ -146,34 +148,93 @@ def __repr__(self) -> str:
 
     # Twisted machinery: this method is called by the Channel once the full request has
     # been received, to dispatch the request to a resource.
-    #
-    # We're patching Twisted to bail/abort early when we see someone trying to upload
-    # `multipart/form-data` so we can avoid Twisted parsing the entire request body into
-    # in-memory (specific problem of this specific `Content-Type`). This protects us
-    # from an attacker uploading something bigger than the available RAM and crashing
-    # the server with a `MemoryError`, or carefully block just enough resources to cause
-    # all other requests to fail.
-    #
-    # FIXME: This can be removed once we Twisted releases a fix and we update to a
-    # version that is patched
     def requestReceived(self, command: bytes, path: bytes, version: bytes) -> None:
+        # In the case of a Content-Length header being present, and it's value being too
+        # large, this will make debugging issues due to overly large requests much
+        # easier. Currently we handle such cases in `handleContentChunk` and abort the
+        # connection without providing a proper HTTP response.
+        #
+        # Attempting to write an HTTP response from within `handleContentChunk` does not
+        # work, so the code here has been added to at least provide a response in the
+        # case of the Content-Length header being present.
+        content_length_headers = self.requestHeaders.getRawHeaders(b"Content-Length")
+        if content_length_headers is not None:
+            if len(content_length_headers) != 1:
+                logger.warning(
+                    "Dropping request from %s because it contains multiple Content-Length headers: %s %s",
+                    self.client,
+                    command.decode("ascii", errors="replace"),
+                    self.get_redacted_uri(),
+                )
+                self.loseConnection()
+                return
+
+            try:
+                content_length = int(content_length_headers[0])
+                if content_length > self._max_request_body_size:
+                    self.method, self.uri = command, path
+                    self.clientproto = version
+                    logger.warning(
+                        "Rejecting request from %s because Content-Length %d exceeds maximum size %d: %s %s",
+                        self.client,
+                        content_length,
+                        self._max_request_body_size,
+                        self.get_method(),
+                        self.get_redacted_uri(),
+                    )
+
+                    self.code = HTTPStatus.REQUEST_ENTITY_TOO_LARGE.value
+                    self.code_message = bytes(
+                        HTTPStatus.REQUEST_ENTITY_TOO_LARGE.phrase, "ascii"
+                    )
+
+                    error_response_json = {
+                        "errcode": Codes.TOO_LARGE,
+                        "error": "Request content is too large",
+                    }
+                    error_response_bytes = (json.dumps(error_response_json)).encode()
+
+                    self.responseHeaders.setRawHeaders(
+                        b"Content-Type", [b"application/json"]
+                    )
+                    self.responseHeaders.setRawHeaders(
+                        b"content-length", [f"{len(error_response_bytes)}"]
+                    )
+                    self.write(error_response_bytes)
+                    self.loseConnection()
+                    return
+            except ValueError:
+                # Invalid Content-Length header, let it proceed
+                pass
+
+        # We're patching Twisted to bail/abort early when we see someone trying to upload
+        # `multipart/form-data` so we can avoid Twisted parsing the entire request body into
+        # in-memory (specific problem of this specific `Content-Type`). This protects us
+        # from an attacker uploading something bigger than the available RAM and crashing
+        # the server with a `MemoryError`, or carefully block just enough resources to cause
+        # all other requests to fail.
+        #
+        # FIXME: This can be removed once Twisted releases a fix and we update to a
+        # version that is patched
+        # See: https://github.com/element-hq/synapse/security/advisories/GHSA-rfq8-j7rh-8hf2
         if command == b"POST":
             ctype = self.requestHeaders.getRawHeaders(b"content-type")
             if ctype and b"multipart/form-data" in ctype[0]:
                 self.method, self.uri = command, path
                 self.clientproto = version
+                logger.warning(
+                    "Aborting connection from %s because `content-type: multipart/form-data` is unsupported: %s %s",
+                    self.client,
+                    self.get_method(),
+                    self.get_redacted_uri(),
+                )
+
                 self.code = HTTPStatus.UNSUPPORTED_MEDIA_TYPE.value
                 self.code_message = bytes(
                     HTTPStatus.UNSUPPORTED_MEDIA_TYPE.phrase, "ascii"
                 )
-                self.responseHeaders.setRawHeaders(b"content-length", [b"0"])
 
-                logger.warning(
-                    "Aborting connection from %s because `content-type: multipart/form-data` is unsupported: %s %s",
-                    self.client,
-                    command,
-                    path,
-                )
+                self.responseHeaders.setRawHeaders(b"content-length", [b"0"])
                 self.write(b"")
                 self.loseConnection()
                 return

tests/http/test_site.py

@@ -22,6 +22,7 @@
 from twisted.internet.address import IPv6Address
 from twisted.internet.testing import MemoryReactor, StringTransport
 
+from synapse.app._base import max_request_body_size
 from synapse.app.homeserver import SynapseHomeServer
 from synapse.server import HomeServer
 from synapse.util.clock import Clock
@@ -143,3 +144,39 @@ def test_content_type_multipart(self) -> None:
 
         # we should get a 415
         self.assertRegex(transport.value().decode(), r"^HTTP/1\.1 415 ")
+
+    def test_content_length_too_large(self) -> None:
+        """HTTP requests with Content-Length exceeding max size should be rejected with 413"""
+        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)
+
+        # 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)
+
+        # Send a request with Content-Length header that exceeds the limit.
+        # Default max is 50MB (from media max_upload_size), so send something larger.
+        oversized_length = 1 + max_request_body_size(self.hs.config)
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Content-Length: " + str(oversized_length).encode() + b"\r\n"
+            b"\r\n"
+            b"" + b"x" * oversized_length + b"\r\n"
+            b"\r\n"
+        )
+
+        # Advance the reactor to process the request
+        while not transport.disconnecting:
+            self.reactor.advance(1)
+
+        # We should get a 413 Content Too Large
+        response = transport.value().decode()
+        self.assertRegex(response, r"^HTTP/1\.1 413 ")
+        self.assertSubstring("M_TOO_LARGE", response)

tests/rest/client/test_login.py

@@ -1728,9 +1728,6 @@ def test_username_picker_use_displayname_avatar_and_email(self) -> None:
             content_is_form=True,
             custom_headers=[
                 ("Cookie", "username_mapping_session=" + session_id),
-                # old versions of twisted don't do form-parsing without a valid
-                # content-length header.
-                ("Content-Length", str(len(content))),
             ],
         )
         self.assertEqual(chan.code, 302, chan.result)
@@ -1818,9 +1815,6 @@ def test_username_picker_dont_use_displayname_avatar_or_email(self) -> None:
             content_is_form=True,
             custom_headers=[
                 ("Cookie", "username_mapping_session=" + session_id),
-                # old versions of twisted don't do form-parsing without a valid
-                # content-length header.
-                ("Content-Length", str(len(content))),
             ],
         )
         self.assertEqual(chan.code, 302, chan.result)

tests/rest/client/test_media.py

@@ -2590,7 +2590,6 @@ def test_authenticated_media(self) -> None:
             self.tok,
             shorthand=False,
             content_type=b"image/png",
-            custom_headers=[("Content-Length", str(67))],
         )
         self.assertEqual(channel.code, 200)
         res = channel.json_body.get("content_uri")
@@ -2750,7 +2749,6 @@ def test_authenticated_media_etag(self) -> None:
             self.tok,
             shorthand=False,
             content_type=b"image/png",
-            custom_headers=[("Content-Length", str(67))],
         )
         self.assertEqual(channel.code, 200)
         res = channel.json_body.get("content_uri")
@@ -2909,7 +2907,6 @@ def upload_media(self, size: int) -> FakeChannel:
             access_token=self.tok,
             shorthand=False,
             content_type=b"text/plain",
-            custom_headers=[("Content-Length", str(size))],
         )
 
     def test_upload_under_limit(self) -> None:
@@ -3074,7 +3071,6 @@ def upload_media(self, size: int, tok: str) -> FakeChannel:
             access_token=tok,
             shorthand=False,
             content_type=b"text/plain",
-            custom_headers=[("Content-Length", str(size))],
         )
 
     def test_upload_under_limit(self) -> None:

tests/rest/client/utils.py

@@ -612,7 +612,6 @@ def upload_media(
             filename: The filename of the media to be uploaded
             expect_code: The return code to expect from attempting to upload the media
         """
-        image_length = len(image_data)
         path = "/_matrix/media/r0/upload?filename=%s" % (filename,)
         channel = make_request(
             self.reactor,
@@ -621,7 +620,6 @@ def upload_media(
             path,
             content=image_data,
             access_token=tok,
-            custom_headers=[("Content-Length", str(image_length))],
         )
 
         assert channel.code == expect_code, "Expected: %d, got: %d, resp: %r" % (

tests/server.py

@@ -81,6 +81,7 @@
 from twisted.web.resource import IResource
 from twisted.web.server import Request, Site
 
+from synapse.api.constants import MAX_REQUEST_SIZE
 from synapse.config.database import DatabaseConnectionConfig
 from synapse.config.homeserver import HomeServerConfig
 from synapse.events.auto_accept_invites import InviteAutoAccepter
@@ -241,7 +242,6 @@ def writeSequence(self, data: Iterable[bytes]) -> None:
 
     def loseConnection(self) -> None:
         self.unregisterProducer()
-        self.transport.loseConnection()
 
     # Type ignore: mypy doesn't like the fact that producer isn't an IProducer.
     def registerProducer(self, producer: IProducer, streaming: bool) -> None:
@@ -428,7 +428,13 @@ def make_request(
 
     channel = FakeChannel(site, reactor, ip=client_ip)
 
-    req = request(channel, site, our_server_name="test_server")
+    # `max_request_body_size` copied from `synapse/app/_base.py -> max_request_body_size()`
+    req = request(
+        channel,
+        site,
+        our_server_name="test_server",
+        max_request_body_size=MAX_REQUEST_SIZE,
+    )
     channel.request = req
 
     req.content = BytesIO(content)