Insufficient Protection against HTTP Request Smuggling in mitmproxy

@@ -98,6 +98,13 @@ def load(self, loader):
             in custom scripts are lowercased before they are sent.
             """,
         )
+        loader.add_option(
+            "validate_inbound_headers", bool, True,
+            """
+            Make sure that incoming HTTP requests are not malformed.
+            Disabling this option makes mitmproxy vulnerable to HTTP smuggling attacks.
+            """,
+        )
 
     async def running(self):
         self.master = ctx.master

@@ -3,6 +3,7 @@
     read_response_head,
     connection_close,
     expected_http_body_size,
+    validate_headers,
 )
 from .assemble import (
     assemble_request, assemble_request_head,
@@ -16,6 +17,7 @@
     "read_response_head",
     "connection_close",
     "expected_http_body_size",
+    "validate_headers",
     "assemble_request", "assemble_request_head",
     "assemble_response", "assemble_response_head",
     "assemble_body",

@@ -38,6 +38,38 @@ def connection_close(http_version, headers):
     )
 
 
+# https://datatracker.ietf.org/doc/html/rfc7230#section-3.2: Header fields are tokens.
+# "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /  "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
+_valid_header_name = re.compile(rb"^[!#$%&'*+\-.^_`|~0-9a-zA-Z]+$")
+
+
+def validate_headers(
+    headers: Headers
+) -> None:
+    """
+    Validate headers to avoid request smuggling attacks. Raises a ValueError if they are malformed.
+    """
+
+    te_found = False
+    cl_found = False
+
+    for (name, value) in headers.fields:
+        if not _valid_header_name.match(name):
+            raise ValueError(f"Received an invalid header name: {name!r}. Invalid header names may introduce "
+                             f"request smuggling vulnerabilities. Disable the validate_inbound_headers option "
+                             f"to skip this security check.")
+
+        name_lower = name.lower()
+        te_found = te_found or name_lower == b"transfer-encoding"
+        cl_found = cl_found or name_lower == b"content-length"
+
+    if te_found and cl_found:
+        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. "
+                         "Disable the validate_inbound_headers option to skip this security check.")
+
+
 def expected_http_body_size(
         request: Request,
         response: Optional[Response] = None
@@ -101,10 +133,8 @@ def expected_http_body_size(
     #        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.")
+        # we should make sure that there isn't also a content-length header.
+        # this is already handled in validate_headers.
 
         te: str = headers["transfer-encoding"]
         if not te.isascii():

@@ -234,6 +234,8 @@ def read_headers(self, event: events.ConnectionEvent) -> layer.CommandGenerator[
                 request_head = [bytes(x) for x in request_head]  # TODO: Make url.parse compatible with bytearrays
                 try:
                     self.request = http1.read_request_head(request_head)
+                    if self.context.options.validate_inbound_headers:
+                        http1.validate_headers(self.request.headers)
                     expected_body_size = http1.expected_http_body_size(self.request)
                 except ValueError as e:
                     yield commands.SendData(self.conn, make_error_response(400, str(e)))
@@ -330,6 +332,8 @@ def read_headers(self, event: events.ConnectionEvent) -> layer.CommandGenerator[
                 response_head = [bytes(x) for x in response_head]  # TODO: Make url.parse compatible with bytearrays
                 try:
                     self.response = http1.read_response_head(response_head)
+                    if self.context.options.validate_inbound_headers:
+                        http1.validate_headers(self.response.headers)
                     expected_size = http1.expected_http_body_size(self.request, self.response)
                 except ValueError as e:
                     yield commands.CloseConnection(self.conn)

@@ -40,7 +40,7 @@ class Http2Connection(HttpConnection):
     h2_conf_defaults = dict(
         header_encoding=False,
         validate_outbound_headers=False,
-        validate_inbound_headers=True,
+        # validate_inbound_headers is controlled by the validate_inbound_headers option.
         normalize_inbound_headers=False,  # changing this to True is required to pass h2spec
         normalize_outbound_headers=False,
     )
@@ -58,6 +58,7 @@ def __init__(self, context: Context, conn: Connection):
         if self.debug:
             self.h2_conf.logger = H2ConnectionLogger(f"{human.format_address(self.context.client.peername)}: "
                                                      f"{self.__class__.__name__}")
+        self.h2_conf.validate_inbound_headers = self.context.options.validate_inbound_headers
         self.h2_conn = BufferedH2Connection(self.h2_conf)
         self.streams = {}
 

@@ -4,7 +4,7 @@
 from mitmproxy.net.http.http1.read import (
     read_request_head,
     read_response_head, connection_close, expected_http_body_size,
-    _read_request_line, _read_response_line, _read_headers, get_header_tokens
+    _read_request_line, _read_response_line, _read_headers, get_header_tokens, validate_headers
 )
 from mitmproxy.test.tutils import treq, tresp
 
@@ -59,6 +59,19 @@ def test_read_response_head():
     assert r.content is None
 
 
+def test_validate_headers():
+    # both content-length and chunked (possible request smuggling)
+    with pytest.raises(ValueError, match="Received both a Transfer-Encoding and a Content-Length header"):
+        validate_headers(
+            Headers(transfer_encoding="chunked", content_length="42"),
+        )
+
+    with pytest.raises(ValueError, match="Received an invalid header name"):
+        validate_headers(
+            Headers([(b"content-length ", b"42")]),
+        )
+
+
 def test_expected_http_body_size():
     # Expect: 100-continue
     assert expected_http_body_size(
@@ -91,11 +104,6 @@ def test_expected_http_body_size():
     assert expected_http_body_size(
         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"

@@ -352,11 +352,11 @@ 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")
 @pytest.mark.parametrize("normalize", [True, False])
 def test_no_normalization(tctx, normalize):
     """Test that we don't normalize headers when we just pass them through."""
     tctx.options.normalize_outbound_headers = normalize
+    tctx.options.validate_inbound_headers = False
 
     server = Placeholder(Server)
     flow = Placeholder(HTTPFlow)

@@ -1261,6 +1261,37 @@ def test_request_smuggling(tctx):
     assert b"Received both a Transfer-Encoding and a Content-Length header" in err()
 
 
+def test_request_smuggling_whitespace(tctx):
+    """Test that we reject header names with whitespace"""
+    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\r\n")
+        << SendData(tctx.client, err)
+        << CloseConnection(tctx.client)
+    )
+    assert b"Received an invalid header name" in err()
+
+
+def test_request_smuggling_validation_disabled(tctx):
+    """Test that we don't reject request smuggling when validation is disabled."""
+    tctx.options.validate_inbound_headers = False
+    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: 4\r\n"
+                                     b"Transfer-Encoding: chunked\r\n\r\n"
+                                     b"4\r\n"
+                                     b"abcd\r\n"
+                                     b"0\r\n"
+                                     b"\r\n")
+        << OpenConnection(Placeholder(Server))
+    )
+
+
 def test_request_smuggling_te_te(tctx):
     """Test that we reject transfer-encoding headers that are weird in some way"""
     err = Placeholder(bytes)