Merge pull request from GHSA-22gh-3r9q-xf38

This commit makes mitmproxy hard-fail when it encounters any attempts
at request/response smuggling.

For details, see GHSA-22gh-3r9q-xf38

mitmproxy/net/http/http1/read.py

@@ -52,23 +52,105 @@ def expected_http_body_size(
         Raises:
             ValueError, if the content length header is invalid
     """
-    # Determine response size according to
-    # http://tools.ietf.org/html/rfc7230#section-3.3
+    # Determine response size according to http://tools.ietf.org/html/rfc7230#section-3.3, which is inlined below.
     if not response:
         headers = request.headers
-        if request.method.upper() == "CONNECT":
-            return 0
     else:
         headers = response.headers
+
+        #    1.  Any response to a HEAD request and any response with a 1xx
+        #        (Informational), 204 (No Content), or 304 (Not Modified) status
+        #        code is always terminated by the first empty line after the
+        #        header fields, regardless of the header fields present in the
+        #        message, and thus cannot contain a message body.
         if request.method.upper() == "HEAD":
             return 0
         if 100 <= response.status_code <= 199:
             return 0
-        if response.status_code == 200 and request.method.upper() == "CONNECT":
-            return 0
         if response.status_code in (204, 304):
             return 0
 
+        #    2.  Any 2xx (Successful) response to a CONNECT request implies that
+        #        the connection will become a tunnel immediately after the empty
+        #        line that concludes the header fields.  A client MUST ignore any
+        #        Content-Length or Transfer-Encoding header fields received in
+        #        such a message.
+        if 200 <= response.status_code <= 299 and request.method.upper() == "CONNECT":
+            return 0
+
+    #    3.  If a Transfer-Encoding header field is present and the chunked
+    #        transfer coding (Section 4.1) is the final encoding, the message
+    #        body length is determined by reading and decoding the chunked
+    #        data until the transfer coding indicates the data is complete.
+    #
+    #        If a Transfer-Encoding header field is present in a response and
+    #        the chunked transfer coding is not the final encoding, the
+    #        message body length is determined by reading the connection until
+    #        it is closed by the server.  If a Transfer-Encoding header field
+    #        is present in a request and the chunked transfer coding is not
+    #        the final encoding, the message body length cannot be determined
+    #        reliably; the server MUST respond with the 400 (Bad Request)
+    #        status code and then close the connection.
+    #
+    #        If a message is received with both a Transfer-Encoding and a
+    #        Content-Length header field, the Transfer-Encoding overrides the
+    #        Content-Length.  Such a message might indicate an attempt to
+    #        perform request smuggling (Section 9.5) or response splitting
+    #        (Section 9.4) and ought to be handled as an error.  A sender MUST
+    #        remove the received Content-Length field prior to forwarding such
+    #        a message downstream.
+    #
+    if "transfer-encoding" in headers:
+        if "content-length" in headers:
+            raise ValueError("Received both a Transfer-Encoding and a Content-Length header, "
+                             "refusing as recommended in RFC 7230 Section 3.3.3. "
+                             "See https://github.com/mitmproxy/mitmproxy/issues/4799 for details.")
+
+        te: str = headers["transfer-encoding"]
+        if not te.isascii():
+            # guard against .lower() transforming non-ascii to ascii
+            raise ValueError(f"Invalid transfer encoding: {te!r}")
+        te = te.lower().strip("\t ")
+        te = re.sub(r"[\t ]*,[\t ]*", ",", te)
+        if te in (
+            "chunked",
+            "compress,chunked",
+            "deflate,chunked",
+            "gzip,chunked",
+        ):
+            return None
+        elif te in (
+            "compress",
+            "deflate",
+            "gzip",
+            "identity",
+        ):
+            if response:
+                return -1
+            else:
+                raise ValueError(f"Invalid request transfer encoding, message body cannot be determined reliably.")
+        else:
+            raise ValueError(f"Unknown transfer encoding: {headers['transfer-encoding']!r}")
+
+    #    4.  If a message is received without Transfer-Encoding and with
+    #        either multiple Content-Length header fields having differing
+    #        field-values or a single Content-Length header field having an
+    #        invalid value, then the message framing is invalid and the
+    #        recipient MUST treat it as an unrecoverable error.  If this is a
+    #        request message, the server MUST respond with a 400 (Bad Request)
+    #        status code and then close the connection.  If this is a response
+    #        message received by a proxy, the proxy MUST close the connection
+    #        to the server, discard the received response, and send a 502 (Bad
+    #        Gateway) response to the client.  If this is a response message
+    #        received by a user agent, the user agent MUST close the
+    #        connection to the server and discard the received response.
+    #
+    #    5.  If a valid Content-Length header field is present without
+    #        Transfer-Encoding, its decimal value defines the expected message
+    #        body length in octets.  If the sender closes the connection or
+    #        the recipient times out before the indicated number of octets are
+    #        received, the recipient MUST consider the message to be
+    #        incomplete and close the connection.
     if "content-length" in headers:
         sizes = headers.get_all("content-length")
         different_content_length_headers = any(x != sizes[0] for x in sizes)
@@ -78,10 +160,16 @@ def expected_http_body_size(
         if size < 0:
             raise ValueError("Negative Content Length")
         return size
-    if "chunked" in headers.get("transfer-encoding", "").lower():
-        return None
+
+    #    6.  If this is a request message and none of the above are true, then
+    #        the message body length is zero (no message body is present).
     if not response:
         return 0
+
+    #    7.  Otherwise, this is a response message without a declared message
+    #        body length, so the message body length is determined by the
+    #        number of octets received prior to the server closing the
+    #        connection.
     return -1
 
 

mitmproxy/proxy/layers/http/_http1.py

@@ -212,17 +212,7 @@ def send(self, event: HttpEvent) -> layer.CommandGenerator[None]:
             yield from self.mark_done(response=True)
         elif isinstance(event, ResponseProtocolError):
             if not self.response and event.code != status_codes.NO_RESPONSE:
-                resp = http.Response.make(
-                    event.code,
-                    format_error(event.code, event.message),
-                    http.Headers(
-                        Server=version.MITMPROXY,
-                        Connection="close",
-                        Content_Type="text/html",
-                    )
-                )
-                raw = http1.assemble_response(resp)
-                yield commands.SendData(self.conn, raw)
+                yield commands.SendData(self.conn, make_error_response(event.code, event.message))
             if self.conn.state & ConnectionState.CAN_WRITE:
                 yield commands.CloseConnection(self.conn)
         else:
@@ -238,6 +228,7 @@ def read_headers(self, event: events.ConnectionEvent) -> layer.CommandGenerator[
                     expected_body_size = http1.expected_http_body_size(self.request)
                 except ValueError as e:
                     yield commands.Log(f"{human.format_address(self.conn.peername)}: {e}")
+                    yield commands.SendData(self.conn, make_error_response(400, str(e)))
                     yield commands.CloseConnection(self.conn)
                     self.state = self.done
                     return
@@ -376,8 +367,8 @@ def make_body_reader(expected_size: Optional[int]) -> TBodyReader:
 def make_error_response(
     status_code: int,
     message: str = "",
-) -> http.Response:
-    return http.Response.make(
+) -> bytes:
+    resp = http.Response.make(
         status_code,
         format_error(status_code, message),
         http.Headers(
@@ -386,6 +377,7 @@ def make_error_response(
             Content_Type="text/html",
         )
     )
+    return http1.assemble_response(resp)
 
 
 __all__ = [

mitmproxy/proxy/layers/http/_http2.py

@@ -37,8 +37,8 @@ class Http2Connection(HttpConnection):
     h2_conf_defaults = dict(
         header_encoding=False,
         validate_outbound_headers=False,
-        validate_inbound_headers=False,  # changing these two to True is required to pass h2spec
-        normalize_inbound_headers=False,  # changing these two to True is required to pass h2spec
+        validate_inbound_headers=True,
+        normalize_inbound_headers=False,  # changing this to True is required to pass h2spec
         normalize_outbound_headers=False,
     )
     h2_conn: BufferedH2Connection

test/mitmproxy/net/http/http1/test_read.py

@@ -71,7 +71,7 @@ def test_expected_http_body_size():
         tresp(headers=Headers(content_length="42"))
     ) == 0
     assert expected_http_body_size(
-        treq(method=b"CONNECT"),
+        treq(method=b"CONNECT", headers=Headers()),
         None,
     ) == 0
     assert expected_http_body_size(
@@ -89,8 +89,34 @@ def test_expected_http_body_size():
         treq(headers=Headers(transfer_encoding="chunked")),
     ) is None
     assert expected_http_body_size(
-        treq(headers=Headers(transfer_encoding="chunked", content_length="42")),
-    ) == 42
+        treq(headers=Headers(transfer_encoding="gzip,\tchunked")),
+    ) is None
+    # both content-length and chunked (possible request smuggling)
+    with pytest.raises(ValueError, match="Received both a Transfer-Encoding and a Content-Length header"):
+        expected_http_body_size(
+            treq(headers=Headers(transfer_encoding="chunked", content_length="42")),
+        )
+    with pytest.raises(ValueError, match="Invalid transfer encoding"):
+        expected_http_body_size(
+            treq(headers=Headers(transfer_encoding="chun\u212Aed")),  # "chunKed".lower() == "chunked"
+        )
+    with pytest.raises(ValueError, match="Unknown transfer encoding"):
+        expected_http_body_size(
+            treq(headers=Headers(transfer_encoding="chun ked")),  # "chunKed".lower() == "chunked"
+        )
+    with pytest.raises(ValueError, match="Unknown transfer encoding"):
+        expected_http_body_size(
+            treq(headers=Headers(transfer_encoding="qux")),
+        )
+    # transfer-encoding: gzip
+    with pytest.raises(ValueError, match="Invalid request transfer encoding"):
+        expected_http_body_size(
+            treq(headers=Headers(transfer_encoding="gzip")),
+        )
+    assert expected_http_body_size(
+        treq(),
+        tresp(headers=Headers(transfer_encoding="gzip")),
+    ) == -1
 
     # explicit length
     for val in (b"foo", b"-7"):

test/mitmproxy/proxy/layers/http/test_http.py

@@ -1228,3 +1228,32 @@ def test_original_server_disconnects(tctx):
             >> ConnectionClosed(tctx.server)
             << CloseConnection(tctx.server)
     )
+
+
+def test_request_smuggling(tctx):
+    """Test that we reject request smuggling"""
+    err = Placeholder(bytes)
+    assert (
+        Playbook(http.HttpLayer(tctx, HTTPMode.regular), hooks=False)
+        >> DataReceived(tctx.client, b"GET http://example.com/ HTTP/1.1\r\n"
+                                     b"Host: example.com\r\n"
+                                     b"Content-Length: 42\r\n"
+                                     b"Transfer-Encoding: chunked\r\n\r\n")
+        << SendData(tctx.client, err)
+        << CloseConnection(tctx.client)
+    )
+    assert b"Received both a Transfer-Encoding and a Content-Length header" in err()
+
+
+def test_request_smuggling_te_te(tctx):
+    """Test that we reject transfer-encoding headers that are weird in some way"""
+    err = Placeholder(bytes)
+    assert (
+        Playbook(http.HttpLayer(tctx, HTTPMode.regular), hooks=False)
+        >> DataReceived(tctx.client, ("GET http://example.com/ HTTP/1.1\r\n"
+                                      "Host: example.com\r\n"
+                                      "Transfer-Encoding: chunKed\r\n\r\n").encode())  # note the non-standard "K"
+        << SendData(tctx.client, err)
+        << CloseConnection(tctx.client)
+    )
+    assert b"Invalid transfer encoding" in err()

test/mitmproxy/proxy/layers/http/test_http2.py

@@ -352,6 +352,7 @@ def enable_response_streaming(flow: HTTPFlow):
         assert "peer closed connection" in flow().error.msg
 
 
+@pytest.mark.xfail(reason="inbound validation turned on to protect against request smuggling")
 def test_no_normalization(tctx):
     """Test that we don't normalize headers when we just pass them through."""
 
@@ -727,3 +728,51 @@ def test_early_server_data(tctx):
     assert [type(x) for x in decode_frames(server2())] == [
         hyperframe.frame.HeadersFrame,
     ]
+
+
+def test_request_smuggling_cl(tctx):
+    playbook, cff = start_h2_client(tctx)
+    playbook.hooks = False
+    err = Placeholder(bytes)
+
+    headers = (
+        (b':method', b'POST'),
+        (b':scheme', b'http'),
+        (b':path', b'/'),
+        (b':authority', b'example.com'),
+        (b'content-length', b'3')
+    )
+
+    assert (
+            playbook
+            >> DataReceived(tctx.client,
+                            cff.build_headers_frame(headers).serialize())
+            >> DataReceived(tctx.client,
+                            cff.build_data_frame(b"abcPOST / HTTP/1.1 ...", flags=["END_STREAM"]).serialize())
+            << SendData(tctx.client, err)
+            << CloseConnection(tctx.client)
+    )
+    assert b"InvalidBodyLengthError" in err()
+
+
+def test_request_smuggling_te(tctx):
+    playbook, cff = start_h2_client(tctx)
+    playbook.hooks = False
+    err = Placeholder(bytes)
+
+    headers = (
+        (b':method', b'POST'),
+        (b':scheme', b'http'),
+        (b':path', b'/'),
+        (b':authority', b'example.com'),
+        (b'transfer-encoding', b'chunked')
+    )
+
+    assert (
+            playbook
+            >> DataReceived(tctx.client,
+                            cff.build_headers_frame(headers, flags=["END_STREAM"]).serialize())
+            << SendData(tctx.client, err)
+            << CloseConnection(tctx.client)
+    )
+    assert b"Connection-specific header field present" in err()