Space in a token: request smuggling in mitmproxy's HTTP/1.1 parser

mitmproxy has one job: show the operator what actually went over the wire. So this sentence, from its own advisory, is an unpleasant one:

While mitmproxy would only see one request, the target server would see multiple requests.

Zeyu Zhang reported it on 15 March 2022. A patch existed the next day, the fix shipped on the 19th in mitmproxy 8.0.0, and the whole thing is tracked as CVE-2022-24766, or GHSA-gcx2-gvj7-pxv3 if you prefer.

Addons and event hooks#

mitmproxy started life as a thing you point your phone at to find out why the app is doing something stupid. It has since grown addons: Python scripts that get called on hooks like request and response, can rewrite headers, block things, log things. Once you have that, people build enforcement out of it. A gateway in front of a legacy service. A recording proxy in a test harness. A filter that drops requests to /admin from outside a subnet.

The advisory names that use case and the way it fails. A smuggled request "is still captured as part of another request's body". It "does not appear in the request list and does not go through the usual mitmproxy event hooks". Those hooks are "where users may have implemented custom access control checks or input sanitization."

So the smuggled bytes aren't hidden. They're right there in the flow, sitting inside the body of a request you can click on. They just never become a request as far as mitmproxy is concerned, so nothing you wrote ever runs against them.

HTTP/1.1 message framing#

HTTP/1.1 sends many messages down one TCP connection with nothing between them but agreement about length. Two headers decide where a body ends. Content-Length: 42 means the next 42 bytes. Transfer-Encoding: chunked means a series of size-prefixed chunks terminated by a zero-length one. If both are present the spec says transfer-encoding wins. It also says such a message might be an attempt at smuggling and ought to be handled as an error.

Everything downstream of that agreement is a byte stream that two programs are cutting into pieces. Request smuggling is what happens when they cut in different places. The proxy decides request one is 42 bytes of body and the server decides it was 12. The remaining 30 bytes are, to the server, the start of a new request that nobody upstream ever saw or inspected. It gets prepended to whatever real request comes next down the same connection.

mitmproxy already knew about the obvious version of this. Here is version 7.0.4:

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.")

Good. There was also a check for weird transfer-encoding values, including a nice one for chunKed, which lowercases to chunked because U+212A is KELVIN SIGN. Somebody had clearly thought about this.

Splitting the header line#

RFC 7230, section 3.2.4:

No whitespace is allowed between the header field-name and colon. In the past, differences in the handling of such whitespace have led to security vulnerabilities in request routing and response handling.

Now here is how mitmproxy 7.0.4 split a header line:

name, value = line.split(b":", 1)
value = value.strip()
if not name:
    raise ValueError()
ret.append((name, value))

The value gets stripped. The name does not. Feed it Content-Length : 42 and you get the pair (b"Content-Length ", b"42"), with the space welded onto the name.

Nothing then rejects that. And nothing recognises it either, because mitmproxy's Headers class is a multidict whose key-normalising function is exactly one operation:

@staticmethod
def _kconv(key) -> str:
    # Headers are case-insensitive
    return key.lower()

b"content-length " is not b"content-length". So "content-length" in headers is false. As far as mitmproxy is concerned that header does not exist, and neither does the conflict its existing check was looking for. Meanwhile the header is still in headers.fields. When mitmproxy rebuilds the header block to forward it, it joins the raw pairs back together untouched, space and all.

That's the entire mechanism. mitmproxy computes body length from the headers it recognises and forwards the headers it doesn't recognise verbatim. Anything downstream that is more forgiving about whitespace than mitmproxy is gets a different answer about where the body ends. The advisory doesn't publish an exploit and I won't invent one. The shape is clear enough from the regression test the fix added. It sends exactly Content-Length : 42 and expects mitmproxy to answer with an error saying "Received an invalid header name" and close the connection.

The response direction is worse. A malicious server can play the same trick backwards. Then the extra response is the one your browser or client consumes, while mitmproxy shows you a single clean exchange. For a tool people run specifically to find out what a server is really sending them, that's the wrong way round.

What the patch does#

The fix adds validate_headers. It runs on both request heads and response heads, with the token grammar from the RFC written out as a regex:

# https://datatracker.ietf.org/doc/html/rfc7230#section-3.2: Header fields are tokens.
_valid_header_name = re.compile(rb"^[!#$%&'*+\-.^_`|~0-9a-zA-Z]+$")

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.")

The transfer-encoding-plus-content-length check moved into the same function. Both now live in one place that runs before anything tries to work out a body size.

There's a new option, validate_inbound_headers, on by default, documented as "Disabling this option makes mitmproxy vulnerable to HTTP smuggling attacks". It exists because mitmproxy is also a research tool, and sometimes you do want to send garbage on purpose. The same switch now controls h2's inbound validation, which had previously been hardcoded on.

One loose end worth knowing about if you ever grep for this CVE. The GitHub advisory rates it Moderate. NVD and the CNA record both carry a CVSS 3.1 base score of 9.8, AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H, which is the same vector you'd assign to unauthenticated remote code execution. The advisory's own line is the more useful one: "Unless you use mitmproxy to protect an HTTP/1 service, no action is required."