Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't tolerate wrong te headers #3260

Merged
merged 1 commit into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
benoitc marked this conversation as resolved.
Show resolved Hide resolved

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(',')]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was wrong earlier. This is not immediately incorrect (as everything containing quotes is rejected below). Might send a patch later to re-verbosify the comments warning about the complex nature of the list.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pajod o for me. I think it would be goo dif we can make a release later this week :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what complex nature of the list though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benoitc we split the list in unexpected places, because of the comma in the quoted-string inside transfer-parameter inside transfer-coding, I suggested comments in #3273

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'):
Copy link
Contributor

@pajod pajod Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still do not see how this leads to anything but trouble. We are reading a list of encodings, handle one (even though they all are, as hop-by-hop headers our responsibility only), act like the other does not matter.. then leave it up to the app to figure out how to interpret the body. How could an application possibly correctly deal with this behavior?

WSGI servers must handle any supported inbound “hop-by-hop” headers on their own

# chunked should be the last one
if chunked:
raise InvalidHeader("TRANSFER-ENCODING", req=self)
self.force_close()
else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now empty headers (or leading/trailing comma) are still reported as Unsupported, while you deliberately changed the other cases where the spec sanctions our outright refusal to Invalid. Maybe worth adding back the extra branch to avoid the (not wrong, but also not immediately clear) Unsupported transfer coding: "" message?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean we need to support empty value for Transfer-Encoding?

Ie. Put back this:

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

Shouldn't we rather handle a special parsing for trailers instead ? Though why an empty value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not suggest a change in what we accept and what we refuse (here). I merely suggest revisiting the choice of Exception.

Some cases could be (theoretically) valid per the spec, yet unsupported by us. Others are Invalid. I reckon the first instance of encountering an empty val is always a case of Invalid, as the rfc9112 ABNF for Transfer-Encoding does not allow for empty headers (or consecutive commas outside quoting). So that branch (otherwise processed in else) should be added back. Not changing the fact that we refuse those. Merely swapping the exception.

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.

Loading