Skip to content

Fix three RFC 9112 response framing and header issues - #502

Open
digitalresistor wants to merge 3 commits into
mainfrom
bugfix/rfc9112-response-framing
Open

Fix three RFC 9112 response framing and header issues#502
digitalresistor wants to merge 3 commits into
mainfrom
bugfix/rfc9112-response-framing

Conversation

@digitalresistor

Copy link
Copy Markdown
Member

Three independent findings from an RFC 9112 audit of the response path. None overlap with #498. Each is its own commit with its own tests and changelog entry.

Do not close the connection after a bodiless response

RFC 9112 section 6.3: a response with a 1xx, 204 or 304 status carries no message body and is terminated by the first empty line after the header fields. It is framed unambiguously without a Content-Length or a Transfer-Encoding, so there is nothing for closing the connection to delimit.

Waitress closed it anyway:

$ printf 'GET / HTTP/1.1\r\nHost: localhost\r\nIf-None-Match: "abc"\r\n\r\n' | nc 127.0.0.1 8080
HTTP/1.1 304 Not Modified
Connection: close

Every 304 Not Modified was answered with Connection: close, so a cache revalidating against Waitress had to open a fresh connection for each one.

The close now happens only where it does work. A response that should have carried a body but has no length to declare is delimited by the chunked coding and the connection is still closed after it — that path is unchanged, and there is a new test pinning it so this does not get loosened by accident later.

Related: #197 is the other half of this code path.

Include the received-protocol in the Via response header

RFC 9110 section 7.6.3 defines Via = #( received-protocol RWS received-by [ RWS comment ] ). The received-protocol is not optional, so the pseudonym alone is not a well formed field value. Waitress wrote Via: waitress; it now writes Via: 1.1 waitress, eliding the protocol-name as the grammar allows for HTTP and using the version of the request received.

This header is only added when the WSGI application supplies its own Server header. Whether that is the right trigger, and whether an origin server should be adding a Via at all, are deliberately left alone here.

Document why header names containing an underscore are dropped

No behaviour change. This replaces the TODO(xistence): Should we drop this request instead? with the answer and the reasoning, plus a section in docs/reverse-proxy.rst where a deployer relying on proxy-set headers would look.

Short version: it should not reject. An underscore is a valid tchar, so the field name is legal, but the CGI style mapping is ambiguous for it — both X-Forwarded-For and X_Forwarded_For arrive as HTTP_X_FORWARDED_FOR, letting a client forge a header the proxy believes only it can set. Dropping the field removes that ambiguity, and having dropped it there is nothing further a 400 would protect against; it would only risk legitimate traffic.

It is also what everyone else settled on. nginx drops via underscores_in_headers plus ignore_invalid_headers; Apache httpd since 2.4; mod_wsgi since 4.3.0; Werkzeug's development server; and gunicorn, whose header_map documents drop as the safe default and offers refuse only as an opt-in. Django applied the same rule at the framework level in CVE-2015-0219, where this class of bug was first described.

Verification

Full default tox envlist green: lint, py39 through py314, pypy39, pypy310, pypy311, coverage (at its --fail-under=100 gate), and docs.

New tests: a functional NotModifiedTests (TCP and Unix) that sends two pipelined requests and asserts both 304s — and both 204s — arrive on a single connection; a unit test pinning that a bodyless-but-unframed response still chunks and still closes; and a Via test covering the request version. Four existing tests that pinned the previous 204/1xx/304 and Via behaviour are updated.

Branched off current main, so it includes the PyPy test fix from #501.

RFC 9112 section 6.3: a response with a 1xx, 204 or 304 status carries no
message body and is terminated by the first empty line after the header
fields. It is therefore framed unambiguously without a Content-Length or a
Transfer-Encoding, and there is nothing for closing the connection to
delimit.

Waitress closed it anyway. Every 304 Not Modified was answered with
Connection: close, so a cache revalidating against Waitress had to open a
fresh connection for each one.

The close now happens only where it is doing work: a response that should
have carried a body but has no length to declare is delimited by the chunked
coding, and the connection is still closed after it. That is unchanged.

See #197 for the other half of this
code path.
RFC 9110 section 7.6.3 defines Via as

    Via = #( received-protocol RWS received-by [ RWS comment ] )

The received-protocol is not optional, so the pseudonym on its own is not a
well formed field value. Waitress wrote "Via: waitress"; it now writes
"Via: 1.1 waitress", eliding the protocol-name as the grammar allows for
HTTP and using the version of the request received.

This header is only added when the WSGI application supplies its own Server
header. Whether that is the right trigger, and whether an origin server
should be adding a Via at all, are left alone here.
Replaces the TODO asking whether such a request should be rejected instead.
It should not, and the reasoning is worth recording where the next person
looks rather than rediscovering it.

An underscore is a valid tchar, so the field name is legal, but the CGI
style mapping the WSGI environ uses is ambiguous for it: both
X-Forwarded-For and X_Forwarded_For arrive as HTTP_X_FORWARDED_FOR, which
lets a client forge a header the proxy in front of us believes only it can
set. Dropping the field removes that ambiguity, and having dropped it there
is nothing further a 400 would protect against -- it would only risk
legitimate traffic, and rejecting outright is itself hazardous in front of
pipelining or proxies.

This is also what everyone else settled on: nginx via underscores_in_headers
plus ignore_invalid_headers, Apache httpd since 2.4, mod_wsgi since 4.3.0,
Werkzeug's development server, and gunicorn, whose header_map setting
documents "drop" as its safe default and offers refusal only as an opt-in.
Django applied the same rule at the framework level in CVE-2015-0219, where
this class of bug was first described.

No behaviour change.

@kgaughan kgaughan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not that you need it, but approved!

@digitalresistor

Copy link
Copy Markdown
Member Author

@kgaughan I still appreciate the reviews. I personally read and validate the changes as best I can, but even AI assisted I will miss certain things or get them wrong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants