Respond with useful error codes when Content-Length header/s are invalid

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
@@ -895,17 +895,8 @@ 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
+    # Baseline default for any request that isn't configured in the homeserver config
+    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
@@ -33,9 +34,11 @@
 from twisted.internet.protocol import Protocol
 from twisted.python.failure import Failure
 from twisted.web.http import HTTPChannel
+from twisted.web.http_headers import Headers
 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
@@ -46,7 +49,7 @@
     PreserveLoggingContext,
 )
 from synapse.metrics import SERVER_NAME_LABEL
-from synapse.types import ISynapseReactor, Requester
+from synapse.types import ISynapseReactor, JsonDict, Requester
 
 if TYPE_CHECKING:
     import opentracing
@@ -146,34 +149,130 @@
 
     # 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, throw a proper error to 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.
+        self.method, self.uri = command, path
+        self.clientproto = version
+
+        def get_content_length_from_headers(headers: Headers) -> int | None:
+            content_length_headers = headers.getRawHeaders(b"Content-Length")
+            if content_length_headers is not None:
+                # If there are multiple `Content-Length` headers return an error.
+                # We don't want to even try to pick the right one if there are multiple
+                # as we could run into problems similar to request smuggling vulnerabilities
+                # which rely on the mismatch of how different systems interpret information.
+                if len(content_length_headers) != 1:
+                    logger.warning(
+                        "Dropping request from %s because it contains multiple Content-Length headers: %s %s",
+                        self.client,
+                        self.get_method(),
+                        self.get_redacted_uri(),
+                    )
+                    raise Exception("Too many Content-Length headers.")
+
+                try:
+                    content_length = int(content_length_headers[0])
+                    return content_length
+                except Exception:
+                    raise Exception("Content-Length value invalid.")
+            return None
+
+        def respond_with_error(error_code: HTTPStatus, error_json: JsonDict):
+            self.code = error_code.value
+            self.code_message = bytes(error_code.phrase, "ascii")
+            error_response_bytes = (json.dumps(error_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)
+
+        try:
+            content_length = get_content_length_from_headers(self.requestHeaders)
+        except Exception as e:
+            error_response_json = {
+                "errcode": Codes.UNKNOWN,
+                "error": f"{str(e)}",
+            }
+            respond_with_error(HTTPStatus.BAD_REQUEST, error_response_json)
+            self.loseConnection()
+            return
+
+        if content_length is not None and self.content is not None:
+            if content_length < self.content.tell():
+                logger.info(
+                    "Rejecting request from %s because Content-Length %d is smaller than the request content size %d: %s %s",
+                    self.client,
+                    content_length,
+                    self.content.tell(),
+                    self.get_method(),
+                    self.get_redacted_uri(),
+                )
+
+                error_response_json = {
+                    "errcode": Codes.UNKNOWN,
+                    "error": "Request content is too small",
+                }
+                respond_with_error(HTTPStatus.BAD_REQUEST, error_response_json)
+                self.loseConnection()
+                return
+
+            if content_length > self._max_request_body_size:
+                logger.info(
+                    "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(),
+                )
+
+                error_response_json = {
+                    "errcode": Codes.TOO_LARGE,
+                    "error": "Request content is too large",
+                }
+                respond_with_error(
+                    HTTPStatus.REQUEST_ENTITY_TOO_LARGE, error_response_json
+                )
+                self.loseConnection()
+                return
+
+        # 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,
-                )
+                # FIXME: Return a better error response here similar to the
+                # `error_response_json` returned in other code paths here.
+                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,104 @@ 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)
+
+    def test_too_many_content_length_headers(self) -> None:
+        """HTTP requests with multiple Content-Length headers should be rejected with 400"""
+        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)
+
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Content-Length: " + str(5).encode() + b"\r\n"
+            b"Content-Length: " + str(5).encode() + b"\r\n"
+            b"\r\n"
+            b"" + b"xxxxx" + 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 400
+        response = transport.value().decode()
+        self.assertRegex(response, r"^HTTP/1\.1 400 ")
+
+    def test_invalid_content_length_headers(self) -> None:
+        """HTTP requests with invalid Content-Length header should be rejected with 400"""
+        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)
+
+        protocol.dataReceived(
+            b"POST / HTTP/1.1\r\n"
+            b"Connection: close\r\n"
+            b"Content-Length: eight\r\n"
+            b"\r\n"
+            b"" + b"xxxxx" + 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 400
+        response = transport.value().decode()
+        self.assertRegex(response, r"^HTTP/1\.1 400 ")

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: