vee1e/mitmproxy
1
0
Fork 0
mirror of https://github.com/vee1e/mitmproxy.git synced 2026-09-01 18:27:18 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

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

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

For details, see https://github.com/mitmproxy/mitmproxy/security/advisories/GHSA-22gh-3r9q-xf38
This commit is contained in:
Maximilian Hils 2021-09-16 11:12:59 +02:00 • committed by GitHub
parent 2323630d40
commit 9fed8ae554
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 210 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

104
mitmproxy/net/http/http1/read.py
View file

@ -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

18
mitmproxy/proxy/layers/http/_http1.py
View file

@ -212,17 +212,7 @@ class Http1Server(Http1Connection):
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 @@ class Http1Server(Http1Connection):
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__ = [

4
mitmproxy/proxy/layers/http/_http2.py
View file

@ -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

32
test/mitmproxy/net/http/http1/test_read.py
View file

@ -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"):

29
test/mitmproxy/proxy/layers/http/test_http.py
View file

@ -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()

49
test/mitmproxy/proxy/layers/http/test_http2.py
View file

@ -352,6 +352,7 @@ def test_http2_client_aborts(tctx, stream, when, how):
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()