From 555d2fa27f2d891f23bd03890e4a826b5018c6b4 Mon Sep 17 00:00:00 2001 From: benoitc Date: Tue, 6 Aug 2024 23:02:06 +0200 Subject: [PATCH] don't tolerate wrong te headers changes: - Just follow the new TE specification (https://datatracker.ietf.org/doc/html/rfc9112#name-transfer-encoding) here and accept to introduce a breaking change. - gandle multiple TE on one line ** breaking changes ** : invalid headers and position will now return an error. --- gunicorn/config.py | 27 --------- gunicorn/http/message.py | 58 ++++++++----------- tests/requests/invalid/chunked_03.py | 4 +- tests/requests/invalid/chunked_06.py | 4 +- tests/requests/valid/025.http | 22 ++----- tests/requests/valid/025.py | 25 ++------ tests/requests/valid/025_line.http | 9 +++ tests/requests/valid/025_line.py | 10 ++++ tests/requests/valid/025compat.http | 18 ------ tests/requests/valid/025compat.py | 27 --------- .../valid/invalid_field_value_01_compat.http | 8 --- .../valid/invalid_field_value_01_compat.py | 18 ------ 12 files changed, 54 insertions(+), 176 deletions(-) create mode 100644 tests/requests/valid/025_line.http create mode 100644 tests/requests/valid/025_line.py delete mode 100644 tests/requests/valid/025compat.http delete mode 100644 tests/requests/valid/025compat.py delete mode 100644 tests/requests/valid/invalid_field_value_01_compat.http delete mode 100644 tests/requests/valid/invalid_field_value_01_compat.py diff --git a/gunicorn/config.py b/gunicorn/config.py index a117a73e0..79662e4c0 100644 --- a/gunicorn/config.py +++ b/gunicorn/config.py @@ -2369,30 +2369,3 @@ class HeaderMap(Setting): .. versionadded:: 22.0.0 """ - - -class TolerateDangerousFraming(Setting): - name = "tolerate_dangerous_framing" - section = "Server Mechanics" - cli = ["--tolerate-dangerous-framing"] - validator = validate_bool - action = "store_true" - default = False - desc = """\ - Process requests with both Transfer-Encoding and Content-Length - - This is known to induce vulnerabilities, but not strictly forbidden by RFC9112. - - In any case, the connection is closed after the malformed request, - as it is unclear if and at which boundary additional requests start. - - Use with care and only if necessary. - Temporary; will be changed or removed in a future version. - - .. versionadded:: 22.0.0 - .. versionchanged: 22.1.0 - The newly added rejection of invalid and dangerous characters CR, LF and NUL in - header field values is also controlled with this setting. rfc9110 permits both - rejecting and SP-replacing. With this option set, Gunicorn passes the field value - unchanged. With this option unset, Gunicorn rejects the request. - """ diff --git a/gunicorn/http/message.py b/gunicorn/http/message.py index 7e66d57e5..1cb48ef30 100644 --- a/gunicorn/http/message.py +++ b/gunicorn/http/message.py @@ -123,10 +123,7 @@ def parse_headers(self, data, from_trailer=False): value = " ".join(value) if RFC9110_5_5_INVALID_AND_DANGEROUS.search(value): - if not self.cfg.tolerate_dangerous_framing: - raise InvalidHeader(name) - # value = RFC9110_5_5_INVALID_AND_DANGEROUS.sub(" ", value) - self.force_close() + raise InvalidHeader(name) if header_length > self.limit_request_field_size > 0: raise LimitRequestHeaders("limit request headers fields size") @@ -172,31 +169,27 @@ def set_body_reader(self): raise InvalidHeader("CONTENT-LENGTH", req=self) content_length = value elif name == "TRANSFER-ENCODING": - if value.lower() == "chunked": - # DANGER: transfer codings stack, and stacked chunking is never intended - if chunked: - raise InvalidHeader("TRANSFER-ENCODING", req=self) - chunked = True - elif value.lower() == "identity": - # does not do much, could still plausibly desync from what the proxy does - # safe option: nuke it, its never needed - if chunked: - raise InvalidHeader("TRANSFER-ENCODING", req=self) - elif value.lower() == "": - # lacking security review on this case - # offer the option to restore previous behaviour, but refuse by default, for now - self.force_close() - if not self.cfg.tolerate_dangerous_framing: + # T-E can be a list + # https://datatracker.ietf.org/doc/html/rfc9112#name-transfer-encoding + vals = [v.strip() for v in value.split(',')] + for val in vals: + if val.lower() == "chunked": + # DANGER: transfer codings stack, and stacked chunking is never intended + if chunked: + raise InvalidHeader("TRANSFER-ENCODING", req=self) + chunked = True + elif val.lower() == "identity": + # does not do much, could still plausibly desync from what the proxy does + # safe option: nuke it, its never needed + if chunked: + raise InvalidHeader("TRANSFER-ENCODING", req=self) + elif val.lower() in ('compress', 'deflate', 'gzip'): + # chunked should be the last one + if chunked: + raise InvalidHeader("TRANSFER-ENCODING", req=self) + self.force_close() + else: raise UnsupportedTransferCoding(value) - # DANGER: do not change lightly; ref: request smuggling - # T-E is a list and we *could* support correctly parsing its elements - # .. but that is only safe after getting all the edge cases right - # .. for which no real-world need exists, so best to NOT open that can of worms - else: - self.force_close() - # even if parser is extended, retain this branch: - # the "chunked not last" case remains to be rejected! - raise UnsupportedTransferCoding(value) if chunked: # two potentially dangerous cases: @@ -204,16 +197,11 @@ def set_body_reader(self): # b) chunked HTTP/1.0 (always faulty) if self.version < (1, 1): # framing wonky, see RFC 9112 Section 6.1 - self.force_close() - if not self.cfg.tolerate_dangerous_framing: - raise InvalidHeader("TRANSFER-ENCODING", req=self) + raise InvalidHeader("TRANSFER-ENCODING", req=self) if content_length is not None: # we cannot be certain the message framing we understood matches proxy intent # -> whatever happens next, remaining input must not be trusted - self.force_close() - # either processing or rejecting is permitted in RFC 9112 Section 6.1 - if not self.cfg.tolerate_dangerous_framing: - raise InvalidHeader("CONTENT-LENGTH", req=self) + raise InvalidHeader("CONTENT-LENGTH", req=self) self.body = Body(ChunkedReader(self, self.unreader)) elif content_length is not None: try: diff --git a/tests/requests/invalid/chunked_03.py b/tests/requests/invalid/chunked_03.py index 58a34600c..e4d574187 100644 --- a/tests/requests/invalid/chunked_03.py +++ b/tests/requests/invalid/chunked_03.py @@ -1,2 +1,2 @@ -from gunicorn.http.errors import UnsupportedTransferCoding -request = UnsupportedTransferCoding +from gunicorn.http.errors import InvalidHeader +request = InvalidHeader diff --git a/tests/requests/invalid/chunked_06.py b/tests/requests/invalid/chunked_06.py index 58a34600c..e4d574187 100644 --- a/tests/requests/invalid/chunked_06.py +++ b/tests/requests/invalid/chunked_06.py @@ -1,2 +1,2 @@ -from gunicorn.http.errors import UnsupportedTransferCoding -request = UnsupportedTransferCoding +from gunicorn.http.errors import InvalidHeader +request = InvalidHeader diff --git a/tests/requests/valid/025.http b/tests/requests/valid/025.http index 214f3094c..e1cc2b580 100644 --- a/tests/requests/valid/025.http +++ b/tests/requests/valid/025.http @@ -1,24 +1,10 @@ -POST /chunked_cont_h_at_first HTTP/1.1\r\n +POST /chunked HTTP/1.1\r\n +Transfer-Encoding: gzip\r\n Transfer-Encoding: chunked\r\n \r\n -5; some; parameters=stuff\r\n +5\r\n hello\r\n -6 \t;\tblahblah; blah\r\n +6\r\n world\r\n 0\r\n \r\n -PUT /chunked_cont_h_at_last HTTP/1.1\r\n -Transfer-Encoding: chunked\r\n -Content-Length: -1\r\n -\r\n -5; some; parameters=stuff\r\n -hello\r\n -6; blahblah; blah\r\n - world\r\n -0\r\n -\r\n -PUT /ignored_after_dangerous_framing HTTP/1.1\r\n -Content-Length: 3\r\n -\r\n -foo\r\n -\r\n diff --git a/tests/requests/valid/025.py b/tests/requests/valid/025.py index 33f5845cb..d23870680 100644 --- a/tests/requests/valid/025.py +++ b/tests/requests/valid/025.py @@ -1,27 +1,10 @@ -from gunicorn.config import Config - -cfg = Config() -cfg.set("tolerate_dangerous_framing", True) - -req1 = { +request = { "method": "POST", - "uri": uri("/chunked_cont_h_at_first"), + "uri": uri("/chunked"), "version": (1, 1), "headers": [ - ("TRANSFER-ENCODING", "chunked") + ('TRANSFER-ENCODING', 'gzip'), + ('TRANSFER-ENCODING', 'chunked') ], "body": b"hello world" } - -req2 = { - "method": "PUT", - "uri": uri("/chunked_cont_h_at_last"), - "version": (1, 1), - "headers": [ - ("TRANSFER-ENCODING", "chunked"), - ("CONTENT-LENGTH", "-1"), - ], - "body": b"hello world" -} - -request = [req1, req2] diff --git a/tests/requests/valid/025_line.http b/tests/requests/valid/025_line.http new file mode 100644 index 000000000..bde502671 --- /dev/null +++ b/tests/requests/valid/025_line.http @@ -0,0 +1,9 @@ +POST /chunked HTTP/1.1\r\n +Transfer-Encoding: gzip,chunked\r\n +\r\n +5\r\n +hello\r\n +6\r\n + world\r\n +0\r\n +\r\n diff --git a/tests/requests/valid/025_line.py b/tests/requests/valid/025_line.py new file mode 100644 index 000000000..b2ebc652e --- /dev/null +++ b/tests/requests/valid/025_line.py @@ -0,0 +1,10 @@ +request = { + "method": "POST", + "uri": uri("/chunked"), + "version": (1, 1), + "headers": [ + ('TRANSFER-ENCODING', 'gzip,chunked') + + ], + "body": b"hello world" +} diff --git a/tests/requests/valid/025compat.http b/tests/requests/valid/025compat.http deleted file mode 100644 index 828f6fb71..000000000 --- a/tests/requests/valid/025compat.http +++ /dev/null @@ -1,18 +0,0 @@ -POST /chunked_cont_h_at_first HTTP/1.1\r\n -Transfer-Encoding: chunked\r\n -\r\n -5; some; parameters=stuff\r\n -hello\r\n -6; blahblah; blah\r\n - world\r\n -0\r\n -\r\n -PUT /chunked_cont_h_at_last HTTP/1.1\r\n -Transfer-Encoding: chunked\r\n -Content-Length: -1\r\n -\r\n -5; some; parameters=stuff\r\n -hello\r\n -6; blahblah; blah\r\n - world\r\n -0\r\n diff --git a/tests/requests/valid/025compat.py b/tests/requests/valid/025compat.py deleted file mode 100644 index 33f5845cb..000000000 --- a/tests/requests/valid/025compat.py +++ /dev/null @@ -1,27 +0,0 @@ -from gunicorn.config import Config - -cfg = Config() -cfg.set("tolerate_dangerous_framing", True) - -req1 = { - "method": "POST", - "uri": uri("/chunked_cont_h_at_first"), - "version": (1, 1), - "headers": [ - ("TRANSFER-ENCODING", "chunked") - ], - "body": b"hello world" -} - -req2 = { - "method": "PUT", - "uri": uri("/chunked_cont_h_at_last"), - "version": (1, 1), - "headers": [ - ("TRANSFER-ENCODING", "chunked"), - ("CONTENT-LENGTH", "-1"), - ], - "body": b"hello world" -} - -request = [req1, req2] diff --git a/tests/requests/valid/invalid_field_value_01_compat.http b/tests/requests/valid/invalid_field_value_01_compat.http deleted file mode 100644 index eab2b5f1f..000000000 --- a/tests/requests/valid/invalid_field_value_01_compat.http +++ /dev/null @@ -1,8 +0,0 @@ -GET / HTTP/1.1\r\n -Host: x\r\n -Newline: a\n -Content-Length: 26\r\n -X-Forwarded-By: broken-proxy\r\n\r\n -GET / HTTP/1.1\n -Host: x\r\n -\r\n diff --git a/tests/requests/valid/invalid_field_value_01_compat.py b/tests/requests/valid/invalid_field_value_01_compat.py deleted file mode 100644 index 9621bc0f4..000000000 --- a/tests/requests/valid/invalid_field_value_01_compat.py +++ /dev/null @@ -1,18 +0,0 @@ -from gunicorn.config import Config - -cfg = Config() -cfg.set("tolerate_dangerous_framing", True) - -req1 = { - "method": "GET", - "uri": uri("/"), - "version": (1, 1), - "headers": [ - ("HOST", "x"), - ("NEWLINE", "a\nContent-Length: 26"), - ("X-FORWARDED-BY", "broken-proxy"), - ], - "body": b"" -} - -request = [req1]