CVE-2022-24766: Inconsistent Interpretation of HTTP Requests ('HTTP Request/Response Smuggling') (CWE-444)
value: mitmproxy is an interactive, SSL/TLS-capable intercepting proxy. In mitmproxy 7.0.4 and below, a malicious client or server is able to perform HTTP request smuggling attacks through mitmproxy. This means that a malicious client/server could smuggle a request/response through mitmproxy as part of another request/response's HTTP message body. While mitmproxy would only see one request, the target server would see multiple requests. A smuggled request is still captured as part of another request's body, but it does not appear in the request list and does not go through the usual mitmproxy event hooks, where users may have implemented custom access control checks or input sanitization. Unless mitmproxy is used to protect an HTTP/1 service, no action is required. The vulnerability has been fixed in mitmproxy 8.0.0 and above. There are currently no known workarounds.
CWE description: The product acts as an intermediary HTTP agent
         (such as a proxy or firewall) in the data flow between two
         entities such as a client and server, but it does not
         interpret malformed HTTP requests or responses in ways that
         are consistent with how the messages will be processed by
         those entities that are at the ultimate destination.
CVSS v3 base score: 9.8
Published: 2022-03-21T19:15Z
Repository: https://github.com/mitmproxy/mitmproxy
Language: Python
Files changed: mitmproxy/addons/proxyserver.py, mitmproxy/net/http/http1/__init__.py, mitmproxy/net/http/http1/read.py, mitmproxy/proxy/layers/http/_http1.py, mitmproxy/proxy/layers/http/_http2.py, test/mitmproxy/net/http/http1/test_read.py, test/mitmproxy/proxy/layers/http/test_http.py, test/mitmproxy/proxy/layers/http/test_http2.py
Commit date: 2022-03-19 17:20:32 +0100
Commit message: security: reject whitespace in HTTP/1 header names

This commit fixes GHSA-gcx2-gvj7-pxv3 by making mitmproxy
reject header names that contain whitespace characters by default.
A new `validate_inbound_headers` option is provided to turn this behavior
off at the expense of allowing HTTP smuggling vulnerabilities.

Diff: diff --git a/mitmproxy/addons/proxyserver.py b/mitmproxy/addons/proxyserver.py
index db1ce43c8..3f3099c86 100644
--- a/mitmproxy/addons/proxyserver.py
+++ b/mitmproxy/addons/proxyserver.py
@@ -96,10 +96,17 @@ class Proxyserver:
             Normalize outgoing HTTP/2 header names, but emit a warning when doing so.
             HTTP/2 does not allow uppercase header names. This option makes sure that HTTP/2 headers set
             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
         self.options = ctx.options
         self.is_running = True
diff --git a/mitmproxy/net/http/http1/__init__.py b/mitmproxy/net/http/http1/__init__.py
index d05ef2fdc..5de7a1ab9 100644
--- a/mitmproxy/net/http/http1/__init__.py
+++ b/mitmproxy/net/http/http1/__init__.py
@@ -1,10 +1,11 @@
 from .read import (
     read_request_head,
     read_response_head,
     connection_close,
     expected_http_body_size,
+    validate_headers,
 )
 from .assemble import (
     assemble_request, assemble_request_head,
     assemble_response, assemble_response_head,
     assemble_body,
@@ -14,9 +15,10 @@ from .assemble import (
 __all__ = [
     "read_request_head",
     "read_response_head",
     "connection_close",
     "expected_http_body_size",
+    "validate_headers",
     "assemble_request", "assemble_request_head",
     "assemble_response", "assemble_response_head",
     "assemble_body",
 ]
diff --git a/mitmproxy/net/http/http1/read.py b/mitmproxy/net/http/http1/read.py
index 4ac21698b..30840e975 100644
--- a/mitmproxy/net/http/http1/read.py
+++ b/mitmproxy/net/http/http1/read.py
@@ -36,10 +36,42 @@ def connection_close(http_version, headers):
         "HTTP/1.1", b"HTTP/1.1",
         "HTTP/2.0", b"HTTP/2.0",
     )
 
 
+# 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
 ) -> Optional[int]:
     """
@@ -99,14 +131,12 @@ def expected_http_body_size(
     #        (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.")
+        # 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():
             # guard against .lower() transforming non-ascii to ascii
             raise ValueError(f"Invalid transfer encoding: {te!r}")
diff --git a/mitmproxy/proxy/layers/http/_http1.py b/mitmproxy/proxy/layers/http/_http1.py
index 3fff8faea..a312570a5 100644
--- a/mitmproxy/proxy/layers/http/_http1.py
+++ b/mitmproxy/proxy/layers/http/_http1.py
@@ -232,10 +232,12 @@ class Http1Server(Http1Connection):
             request_head = self.buf.maybe_extract_lines()
             if request_head:
                 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)))
                     yield commands.CloseConnection(self.conn)
                     if self.request:
@@ -328,10 +330,12 @@ class Http1Client(Http1Connection):
             response_head = self.buf.maybe_extract_lines()
             if response_head:
                 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)
                     yield ReceiveHttp(ResponseProtocolError(self.stream_id, f"Cannot parse HTTP response: {e}"))
                     return
diff --git a/mitmproxy/proxy/layers/http/_http2.py b/mitmproxy/proxy/layers/http/_http2.py
index cf71e8425..d72b18436 100644
--- a/mitmproxy/proxy/layers/http/_http2.py
+++ b/mitmproxy/proxy/layers/http/_http2.py
@@ -38,11 +38,11 @@ CATCH_HYPER_H2_ERRORS = (ValueError, IndexError)
 class Http2Connection(HttpConnection):
     h2_conf: ClassVar[h2.config.H2Configuration]
     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,
     )
     h2_conn: BufferedH2Connection
     streams: Dict[int, StreamState]
@@ -56,10 +56,11 @@ class Http2Connection(HttpConnection):
     def __init__(self, context: Context, conn: Connection):
         super().__init__(context, conn)
         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 = {}
 
     def is_closed(self, stream_id: int) -> bool:
         """Check if a non-idle stream is closed"""
diff --git a/test/mitmproxy/net/http/http1/test_read.py b/test/mitmproxy/net/http/http1/test_read.py
index 576093896..e8170ae85 100644
--- a/test/mitmproxy/net/http/http1/test_read.py
+++ b/test/mitmproxy/net/http/http1/test_read.py
@@ -2,11 +2,11 @@ import pytest
 
 from mitmproxy.http import Headers
 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
 
 
 def test_get_header_tokens():
@@ -57,10 +57,23 @@ def test_read_response_head():
     assert r.status_code == 418
     assert r.headers["Content-Length"] == "4"
     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(
         treq(headers=Headers(expect="100-continue", content_length="42")),
     ) == 42
@@ -89,15 +102,10 @@ def test_expected_http_body_size():
         treq(headers=Headers(transfer_encoding="chunked")),
     ) is None
     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")),  # "chunKed".lower() == "chunked"
         )
     with pytest.raises(ValueError, match="Unknown transfer encoding"):
diff --git a/test/mitmproxy/proxy/layers/http/test_http.py b/test/mitmproxy/proxy/layers/http/test_http.py
index dfeb35f62..32b1d07ae 100644
--- a/test/mitmproxy/proxy/layers/http/test_http.py
+++ b/test/mitmproxy/proxy/layers/http/test_http.py
@@ -1259,10 +1259,41 @@ def test_request_smuggling(tctx):
         << CloseConnection(tctx.client)
     )
     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)
     assert (
         Playbook(http.HttpLayer(tctx, HTTPMode.regular), hooks=False)
diff --git a/test/mitmproxy/proxy/layers/http/test_http2.py b/test/mitmproxy/proxy/layers/http/test_http2.py
index 60ed36369..2044b0bb1 100644
--- a/test/mitmproxy/proxy/layers/http/test_http2.py
+++ b/test/mitmproxy/proxy/layers/http/test_http2.py
@@ -350,15 +350,15 @@ def test_http2_client_aborts(tctx, stream, when, how):
         assert "stream reset" in flow().error.msg
     else:
         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)
     playbook, cff = start_h2_client(tctx)
 

Vulnerable code:         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.")
        validate_inbound_headers=True,
    _read_request_line, _read_response_line, _read_headers, get_header_tokens
    # 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")),
        )
@pytest.mark.xfail(reason="inbound validation turned on to protect against request smuggling")
Fixed code:         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.
            """,
        )
    validate_headers,
    "validate_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.")


        # we should make sure that there isn't also a content-length header.
        # this is already handled in validate_headers.
                    if self.context.options.validate_inbound_headers:
                        http1.validate_headers(self.request.headers)
                    if self.context.options.validate_inbound_headers:
                        http1.validate_headers(self.response.headers)
        # validate_inbound_headers is controlled by the validate_inbound_headers option.
        self.h2_conf.validate_inbound_headers = self.context.options.validate_inbound_headers
    _read_request_line, _read_response_line, _read_headers, get_header_tokens, validate_headers
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_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))
    )


    tctx.options.validate_inbound_headers = False