diff --git a/mitmproxy/net/http/http1/read.py b/mitmproxy/net/http/http1/read.py index 7700bc856..f48f7d132 100644 --- a/mitmproxy/net/http/http1/read.py +++ b/mitmproxy/net/http/http1/read.py @@ -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 diff --git a/mitmproxy/proxy/layers/http/_http1.py b/mitmproxy/proxy/layers/http/_http1.py index f43d1c6cf..a1397cae6 100644 --- a/mitmproxy/proxy/layers/http/_http1.py +++ b/mitmproxy/proxy/layers/http/_http1.py @@ -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__ = [ diff --git a/mitmproxy/proxy/layers/http/_http2.py b/mitmproxy/proxy/layers/http/_http2.py index 7277e93fb..3225bc20a 100644 --- a/mitmproxy/proxy/layers/http/_http2.py +++ b/mitmproxy/proxy/layers/http/_http2.py @@ -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 diff --git a/test/mitmproxy/net/http/http1/test_read.py b/test/mitmproxy/net/http/http1/test_read.py index 957735a5a..0928c89b3 100644 --- a/test/mitmproxy/net/http/http1/test_read.py +++ b/test/mitmproxy/net/http/http1/test_read.py @@ -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"): diff --git a/test/mitmproxy/proxy/layers/http/test_http.py b/test/mitmproxy/proxy/layers/http/test_http.py index 0bac02128..badfb8ffd 100644 --- a/test/mitmproxy/proxy/layers/http/test_http.py +++ b/test/mitmproxy/proxy/layers/http/test_http.py @@ -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() diff --git a/test/mitmproxy/proxy/layers/http/test_http2.py b/test/mitmproxy/proxy/layers/http/test_http2.py index acd8500da..5d76324cc 100644 --- a/test/mitmproxy/proxy/layers/http/test_http2.py +++ b/test/mitmproxy/proxy/layers/http/test_http2.py @@ -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()