Skip to content

Commit

Permalink
don't tolerate wrong te headers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
benoitc committed Aug 6, 2024
1 parent 9a96e75 commit 555d2fa
Show file tree
Hide file tree
Showing 12 changed files with 54 additions and 176 deletions.
27 changes: 0 additions & 27 deletions gunicorn/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""
58 changes: 23 additions & 35 deletions gunicorn/http/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -172,48 +169,39 @@ 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:
# a) CL + TE (TE overrides CL.. only safe if the recipient sees it that way too)
# 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:
Expand Down
4 changes: 2 additions & 2 deletions tests/requests/invalid/chunked_03.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
from gunicorn.http.errors import UnsupportedTransferCoding
request = UnsupportedTransferCoding
from gunicorn.http.errors import InvalidHeader
request = InvalidHeader
4 changes: 2 additions & 2 deletions tests/requests/invalid/chunked_06.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
from gunicorn.http.errors import UnsupportedTransferCoding
request = UnsupportedTransferCoding
from gunicorn.http.errors import InvalidHeader
request = InvalidHeader
22 changes: 4 additions & 18 deletions tests/requests/valid/025.http
Original file line number Diff line number Diff line change
@@ -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
25 changes: 4 additions & 21 deletions tests/requests/valid/025.py
Original file line number Diff line number Diff line change
@@ -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]
9 changes: 9 additions & 0 deletions tests/requests/valid/025_line.http
Original file line number Diff line number Diff line change
@@ -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
10 changes: 10 additions & 0 deletions tests/requests/valid/025_line.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
request = {
"method": "POST",
"uri": uri("/chunked"),
"version": (1, 1),
"headers": [
('TRANSFER-ENCODING', 'gzip,chunked')

],
"body": b"hello world"
}
18 changes: 0 additions & 18 deletions tests/requests/valid/025compat.http

This file was deleted.

27 changes: 0 additions & 27 deletions tests/requests/valid/025compat.py

This file was deleted.

8 changes: 0 additions & 8 deletions tests/requests/valid/invalid_field_value_01_compat.http

This file was deleted.

18 changes: 0 additions & 18 deletions tests/requests/valid/invalid_field_value_01_compat.py

This file was deleted.

0 comments on commit 555d2fa

Please sign in to comment.