Skip to content

Commit b06fb6d

Browse files
mhilsmhils
committed
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.
1 parent 9243ba4 commit b06fb6d

8 files changed

Lines changed: 95 additions & 12 deletions

File tree

‎mitmproxy/addons/proxyserver.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,13 @@ def load(self, loader):
9898
in custom scripts are lowercased before they are sent.
9999
""",
100100
)
101+
loader.add_option(
102+
"validate_inbound_headers", bool, True,
103+
"""
104+
Make sure that incoming HTTP requests are not malformed.
105+
Disabling this option makes mitmproxy vulnerable to HTTP smuggling attacks.
106+
""",
107+
)
101108

102109
async def running(self):
103110
self.master = ctx.master

‎mitmproxy/net/http/http1/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
read_response_head,
44
connection_close,
55
expected_http_body_size,
6+
validate_headers,
67
)
78
from .assemble import (
89
assemble_request, assemble_request_head,
@@ -16,6 +17,7 @@
1617
"read_response_head",
1718
"connection_close",
1819
"expected_http_body_size",
20+
"validate_headers",
1921
"assemble_request", "assemble_request_head",
2022
"assemble_response", "assemble_response_head",
2123
"assemble_body",

‎mitmproxy/net/http/http1/read.py‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,38 @@ def connection_close(http_version, headers):
3838
)
3939

4040

41+
# https://datatracker.ietf.org/doc/html/rfc7230#section-3.2: Header fields are tokens.
42+
# "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
43+
_valid_header_name = re.compile(rb"^[!#$%&'*+\-.^_`|~0-9a-zA-Z]+$")
44+
45+
46+
def validate_headers(
47+
headers: Headers
48+
) -> None:
49+
"""
50+
Validate headers to avoid request smuggling attacks. Raises a ValueError if they are malformed.
51+
"""
52+
53+
te_found = False
54+
cl_found = False
55+
56+
for (name, value) in headers.fields:
57+
if not _valid_header_name.match(name):
58+
raise ValueError(f"Received an invalid header name: {name!r}. Invalid header names may introduce "
59+
f"request smuggling vulnerabilities. Disable the validate_inbound_headers option "
60+
f"to skip this security check.")
61+
62+
name_lower = name.lower()
63+
te_found = te_found or name_lower == b"transfer-encoding"
64+
cl_found = cl_found or name_lower == b"content-length"
65+
66+
if te_found and cl_found:
67+
raise ValueError("Received both a Transfer-Encoding and a Content-Length header, "
68+
"refusing as recommended in RFC 7230 Section 3.3.3. "
69+
"See https://github.com/mitmproxy/mitmproxy/issues/4799 for details. "
70+
"Disable the validate_inbound_headers option to skip this security check.")
71+
72+
4173
def expected_http_body_size(
4274
request: Request,
4375
response: Optional[Response] = None
@@ -101,10 +133,8 @@ def expected_http_body_size(
101133
# a message downstream.
102134
#
103135
if "transfer-encoding" in headers:
104-
if "content-length" in headers:
105-
raise ValueError("Received both a Transfer-Encoding and a Content-Length header, "
106-
"refusing as recommended in RFC 7230 Section 3.3.3. "
107-
"See https://github.com/mitmproxy/mitmproxy/issues/4799 for details.")
136+
# we should make sure that there isn't also a content-length header.
137+
# this is already handled in validate_headers.
108138

109139
te: str = headers["transfer-encoding"]
110140
if not te.isascii():

‎mitmproxy/proxy/layers/http/_http1.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,8 @@ def read_headers(self, event: events.ConnectionEvent) -> layer.CommandGenerator[
234234
request_head = [bytes(x) for x in request_head] # TODO: Make url.parse compatible with bytearrays
235235
try:
236236
self.request = http1.read_request_head(request_head)
237+
if self.context.options.validate_inbound_headers:
238+
http1.validate_headers(self.request.headers)
237239
expected_body_size = http1.expected_http_body_size(self.request)
238240
except ValueError as e:
239241
yield commands.SendData(self.conn, make_error_response(400, str(e)))
@@ -330,6 +332,8 @@ def read_headers(self, event: events.ConnectionEvent) -> layer.CommandGenerator[
330332
response_head = [bytes(x) for x in response_head] # TODO: Make url.parse compatible with bytearrays
331333
try:
332334
self.response = http1.read_response_head(response_head)
335+
if self.context.options.validate_inbound_headers:
336+
http1.validate_headers(self.response.headers)
333337
expected_size = http1.expected_http_body_size(self.request, self.response)
334338
except ValueError as e:
335339
yield commands.CloseConnection(self.conn)

‎mitmproxy/proxy/layers/http/_http2.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class Http2Connection(HttpConnection):
4040
h2_conf_defaults = dict(
4141
header_encoding=False,
4242
validate_outbound_headers=False,
43-
validate_inbound_headers=True,
43+
# validate_inbound_headers is controlled by the validate_inbound_headers option.
4444
normalize_inbound_headers=False, # changing this to True is required to pass h2spec
4545
normalize_outbound_headers=False,
4646
)
@@ -58,6 +58,7 @@ def __init__(self, context: Context, conn: Connection):
5858
if self.debug:
5959
self.h2_conf.logger = H2ConnectionLogger(f"{human.format_address(self.context.client.peername)}: "
6060
f"{self.__class__.__name__}")
61+
self.h2_conf.validate_inbound_headers = self.context.options.validate_inbound_headers
6162
self.h2_conn = BufferedH2Connection(self.h2_conf)
6263
self.streams = {}
6364

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

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
from mitmproxy.net.http.http1.read import (
55
read_request_head,
66
read_response_head, connection_close, expected_http_body_size,
7-
_read_request_line, _read_response_line, _read_headers, get_header_tokens
7+
_read_request_line, _read_response_line, _read_headers, get_header_tokens, validate_headers
88
)
99
from mitmproxy.test.tutils import treq, tresp
1010

@@ -59,6 +59,19 @@ def test_read_response_head():
5959
assert r.content is None
6060

6161

62+
def test_validate_headers():
63+
# both content-length and chunked (possible request smuggling)
64+
with pytest.raises(ValueError, match="Received both a Transfer-Encoding and a Content-Length header"):
65+
validate_headers(
66+
Headers(transfer_encoding="chunked", content_length="42"),
67+
)
68+
69+
with pytest.raises(ValueError, match="Received an invalid header name"):
70+
validate_headers(
71+
Headers([(b"content-length ", b"42")]),
72+
)
73+
74+
6275
def test_expected_http_body_size():
6376
# Expect: 100-continue
6477
assert expected_http_body_size(
@@ -91,11 +104,6 @@ def test_expected_http_body_size():
91104
assert expected_http_body_size(
92105
treq(headers=Headers(transfer_encoding="gzip,\tchunked")),
93106
) is None
94-
# both content-length and chunked (possible request smuggling)
95-
with pytest.raises(ValueError, match="Received both a Transfer-Encoding and a Content-Length header"):
96-
expected_http_body_size(
97-
treq(headers=Headers(transfer_encoding="chunked", content_length="42")),
98-
)
99107
with pytest.raises(ValueError, match="Invalid transfer encoding"):
100108
expected_http_body_size(
101109
treq(headers=Headers(transfer_encoding="chun\u212Aed")), # "chunKed".lower() == "chunked"

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1261,6 +1261,37 @@ def test_request_smuggling(tctx):
12611261
assert b"Received both a Transfer-Encoding and a Content-Length header" in err()
12621262

12631263

1264+
def test_request_smuggling_whitespace(tctx):
1265+
"""Test that we reject header names with whitespace"""
1266+
err = Placeholder(bytes)
1267+
assert (
1268+
Playbook(http.HttpLayer(tctx, HTTPMode.regular), hooks=False)
1269+
>> DataReceived(tctx.client, b"GET http://example.com/ HTTP/1.1\r\n"
1270+
b"Host: example.com\r\n"
1271+
b"Content-Length : 42\r\n\r\n")
1272+
<< SendData(tctx.client, err)
1273+
<< CloseConnection(tctx.client)
1274+
)
1275+
assert b"Received an invalid header name" in err()
1276+
1277+
1278+
def test_request_smuggling_validation_disabled(tctx):
1279+
"""Test that we don't reject request smuggling when validation is disabled."""
1280+
tctx.options.validate_inbound_headers = False
1281+
assert (
1282+
Playbook(http.HttpLayer(tctx, HTTPMode.regular), hooks=False)
1283+
>> DataReceived(tctx.client, b"GET http://example.com/ HTTP/1.1\r\n"
1284+
b"Host: example.com\r\n"
1285+
b"Content-Length: 4\r\n"
1286+
b"Transfer-Encoding: chunked\r\n\r\n"
1287+
b"4\r\n"
1288+
b"abcd\r\n"
1289+
b"0\r\n"
1290+
b"\r\n")
1291+
<< OpenConnection(Placeholder(Server))
1292+
)
1293+
1294+
12641295
def test_request_smuggling_te_te(tctx):
12651296
"""Test that we reject transfer-encoding headers that are weird in some way"""
12661297
err = Placeholder(bytes)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,11 +352,11 @@ def enable_response_streaming(flow: HTTPFlow):
352352
assert "peer closed connection" in flow().error.msg
353353

354354

355-
@pytest.mark.xfail(reason="inbound validation turned on to protect against request smuggling")
356355
@pytest.mark.parametrize("normalize", [True, False])
357356
def test_no_normalization(tctx, normalize):
358357
"""Test that we don't normalize headers when we just pass them through."""
359358
tctx.options.normalize_outbound_headers = normalize
359+
tctx.options.validate_inbound_headers = False
360360

361361
server = Placeholder(Server)
362362
flow = Placeholder(HTTPFlow)

0 commit comments

Comments
 (0)