fix: reject HTTP requests with Transfer-Encoding not ending in chunked#63251
fix: reject HTTP requests with Transfer-Encoding not ending in chunked#63251mohammadmseet-hue wants to merge 2 commits intodart-lang:mainfrom
Conversation
`_HttpParser` set `_transferEncoding = true` for any non-empty Transfer-Encoding header value, which causes the subsequent code at http_parser.dart:802-803 to drop any received Content-Length header. But `_chunked = true` was set only when one of the comma-separated tokens was exactly "chunked"; nothing checked that chunked was the FINAL transfer coding. Result for a request like: POST / HTTP/1.1 Transfer-Encoding: gzip Content-Length: 38 GET /admin HTTP/1.1\r\nHost: backend\r\n\r\n - `_transferEncoding = true` - `_chunked = false` - Content-Length: 38 is dropped (line 803) - `_transferLength = 0` (set in `_headersEnd`, lines 414-417) - `_reset()` runs (line 442); next bytes parsed as a new request A live PoC against a stock dart:stable HttpServer confirms two requests are dispatched on the same TCP connection from the single payload above: [req#1] POST /pay CL=null TE=gzip body=0B [req#2] GET /admin CL=null TE=null body=0B Any RFC-compliant upstream proxy that honours Content-Length when the declared Transfer-Encoding is unknown — and falls back to identity for unsupported codings, as RFC 7230 permits — therefore disagrees with dart on message boundaries: classic CL/TE request smuggling. An attacker can smuggle a second request past the front-door auth/WAF the proxy applies. The existing inner-loop check accepted "chunked" anywhere in the comma-separated list (e.g. "chunked, gzip"). Per RFC 7230 section 3.3.3 rule 3, chunked must be the FINAL transfer coding for the body length to be determinable in a request; otherwise the server MUST respond with 400 and close the connection. Fix: only set `_chunked = true` when the LAST token equals "chunked". For requests, throw HttpException for all other Transfer-Encoding values — this propagates as a 400 + connection close, which is exactly what RFC 7230 section 3.3.3 rule 3 mandates. This is a different bug from dart-lang#63250 (TE token whitespace) and dart-lang#63133 (TE list parsing — that PR introduced the inner loop this commit tightens). All three together close the known parser-differential class on the request side. Test plan --------- Added regression tests in tests/standalone/io/http_parser_test.dart: - `Transfer-Encoding: gzip` + `Content-Length: 38` request must be rejected by `_testParseInvalidRequest` (smuggling vector blocked). - `Transfer-Encoding: chunked, gzip` request must be rejected (chunked not final). - Existing `gzip, chunked` and `identity, chunked` tests still pass — chunked is the final token. - Existing plain `chunked` test still passes.
|
Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at: https://dart-review.googlesource.com/c/sdk/+/498401 Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR). |
|
https://dart-review.googlesource.com/c/sdk/+/498401 has been updated with the latest commits from this pull request. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
Per Kevin's review: the per-line throw incorrectly rejects valid requests that split Transfer-Encoding across multiple header lines (RFC 7230 §3.2.2 lets recipients combine them). Also, for responses, a 'chunked' line followed by a 'gzip' line would leave _chunked=true incorrectly. Fix: - _chunked is now overwritten on each TE header line so it tracks the LAST seen final-token across all lines. - The request-side rule-3 validation moves to _headersEnd, after every header line has been accumulated. Tests: - New positive case: 'Transfer-Encoding: gzip' + 'Transfer-Encoding: chunked' on two lines is accepted. - New negative case: same two lines reversed (chunked then gzip + CL: 38) is rejected — chunked is no longer final. - Existing 'gzip, chunked', 'identity, chunked', plain 'chunked' tests still pass.
|
https://dart-review.googlesource.com/c/sdk/+/498401 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/498401 has been updated with the latest commits from this pull request. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
2 participants
Summary
_HttpParserset_transferEncoding = truefor any non-emptyTransfer-Encodingheader value, which causes the subsequent code athttp_parser.dart:802-803to drop any receivedContent-Lengthheader. But_chunked = truewas set only when one of the comma-separated tokens was exactlychunked; nothing checked that chunked was the FINAL transfer coding.Result for a request like:
_transferEncoding = true_chunked = falseContent-Length: 38is dropped (line 803)_transferLength = 0(set in_headersEnd, lines 414-417)_reset()runs (line 442); next bytes parsed as a new requestLive PoC against
dart:stableI ran a stock dart:stable HttpServer that just logs each received request, sent the payload above through a single TCP connection, and observed:
Two requests dispatched on the same TCP connection from the single payload. Two
HTTP/1.1 200 OKresponses came back.Any RFC-compliant upstream proxy that honours
Content-Lengthwhen the declaredTransfer-Encodingis unknown — and falls back to identity for unsupported codings, as RFC 7230 permits — therefore disagrees with dart on message boundaries: classic CL/TE request smuggling. An attacker can smuggle a second request past the front-door auth/WAF the proxy applies.Why this is a separate bug from #63250 and #63133
_tokenizeFieldValueso\"x chunked\"no longer collapses to[\"chunked\"]. That's a token-level whitespace bug.\"gzip, chunked\"was not detected because the parser did an exact-string match on the whole header value. That PR introduced the inner loop this commit tightens.\"gzip\") or is present but not the final coding (\"chunked, gzip\"). The inner loop introduced by fix: HTTP request smuggling via Transfer-Encoding list and redirect credential leak #63133 still accepts both as chunked because it doesn't enforce ordering.All three together close the known parser-differential class on the request side.
Fix
RFC 7230 section 3.3.3 rule 3:
_chunked = truewhen the last token equalschunked.HttpExceptionfor all otherTransfer-Encodingvalues — this propagates as a 400 + connection close, exactly what the RFC mandates.Test plan
Added regression tests in
tests/standalone/io/http_parser_test.dart:Transfer-Encoding: gzip+Content-Length: 38request must be rejected by_testParseInvalidRequest(smuggling vector blocked).Transfer-Encoding: chunked, gziprequest must be rejected (chunked not final).gzip, chunkedandidentity, chunkedtests still pass — chunked is the final token.chunkedtest still passes.