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/http/site.py

@@ -146,17 +146,52 @@
 
     # 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:
+            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
+                    self.code = HTTPStatus.REQUEST_ENTITY_TOO_LARGE.value
+                    self.code_message = bytes(
+                        HTTPStatus.REQUEST_ENTITY_TOO_LARGE.phrase, "ascii"
+                    )
+                    self.responseHeaders.setRawHeaders(b"content-length", [b"0"])
+
+                    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,
+                        command.decode("ascii", errors="replace"),
+                        self.get_redacted_uri(),
+                    )
+                    self.write(b"")
+                    self.loseConnection()
+                    return
+            except (ValueError, IndexError):
+                # 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 we Twisted releases a fix and we update to a
+        # version that is patched
         if command == b"POST":
             ctype = self.requestHeaders.getRawHeaders(b"content-type")
             if ctype and b"multipart/form-data" in ctype[0]:
@@ -171,8 +206,8 @@
                 logger.warning(
                     "Aborting connection from %s because `content-type: multipart/form-data` is unsupported: %s %s",
                     self.client,
-                    command,
-                    path,
+                    command.decode("ascii", errors="replace"),
+                    self.get_redacted_uri(),
                 )
                 self.write(b"")
                 self.loseConnection()

tests/http/test_site.py

@@ -143,3 +143,37 @@ 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 = 60 * 1024 * 1024
+        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"
+        )
+        protocol.dataReceived(b"x" * oversized_length)
+
+        # Advance the reactor to process the request
+        while not transport.disconnecting:
+            self.reactor.advance(1)
+
+        # we should get a 413 Payload Too Large
+        response = transport.value().decode()
+        self.assertRegex(response, r"^HTTP/1\.1 413 ")

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_PDU_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=200 * MAX_PDU_SIZE,
+    )
     channel.request = req
 
     req.content = BytesIO(content)