fix: HTTP request smuggling via Transfer-Encoding list and redirect credential leak#63133
fix: HTTP request smuggling via Transfer-Encoding list and redirect credential leak#63133mohammadmseet-hue wants to merge 2 commits intodart-lang:mainfrom
Conversation
…eader leak Two security fixes in dart:io HTTP implementation: 1. HTTP Request Smuggling via Transfer-Encoding list (http_parser.dart) RFC 7230 section 3.3.1 allows Transfer-Encoding to be a comma-separated list (e.g. "gzip, chunked"). The parser only checked for exact match on "chunked", missing the chunked encoding when other encodings preceded it. This creates a CL/TE desync when a Dart HTTP server sits behind a compliant reverse proxy that correctly parses the list. Fix: Parse Transfer-Encoding as a comma-separated list and check each token individually for "chunked". 2. shouldCopyHeaderOnRedirect parameter swap (http_impl.dart) The call at line 3107 passed `resolved` (redirect target) as `originalUrl` and `previous.uri` (original URL) as `redirectUri`, swapping the parameters. This caused the _isSubdomain check to verify the wrong direction, leaking Authorization/Cookie headers to parent domains on redirect and stripping them when redirecting to subdomains. Fix: Swap parameters to match the function signature.
|
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/+/493600 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). |
|
A TEST that demonstrates the problem would be amazing. |
Adds two test cases to http_parser_test.dart that exercise the CL/TE request smuggling fix: 1. POST with `Content-Length: 10` and `Transfer-Encoding: gzip, chunked`. Before the fix the parser did an exact match on the header value, so "gzip, chunked" was not recognized as chunked encoding and the request was read using Content-Length instead. 2. POST with `Transfer-Encoding: identity, chunked` to cover the more general comma-separated list case. Both tests assert chunked=true and that the chunked body bytes are correctly received, which fails on the unpatched parser and passes after the fix. Addresses review feedback on PR dart-lang#63133.
|
@kevmoo good call — I added two regression tests in
Both assert |
|
https://dart-review.googlesource.com/c/sdk/+/493600 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/493600 has been updated with the latest commits from this pull request. |
|
Hi @mohammadmseet-hue thanks very much for the PR! I'm just waiting for the tests to finish running and then I'll merge it. |
|
Gerrit CL has been approved, please wait for a reviewer to merge it. |
Summary
Two security fixes in
dart:ioHTTP implementation:1. HTTP Request Smuggling via Transfer-Encoding list parsing
File:
sdk/lib/_http/http_parser.dart:751RFC 7230 section 3.3.1 allows
Transfer-Encodingto be a comma-separated list (e.g.gzip, chunked). The parser only checked for an exact match on"chunked", so it missed chunked encoding when other encodings preceded it in the list.This creates a CL/TE desync (HTTP request smuggling) when a Dart HTTP server runs behind a compliant reverse proxy. The proxy correctly parses the list and reads a chunked body, while Dart falls back to
Content-Length, causing the two to disagree on message boundaries.Fix: Parse
Transfer-Encodingas a comma-separated list and check each token individually for"chunked".2.
shouldCopyHeaderOnRedirectparameter swap — credential leak on redirectFile:
sdk/lib/_http/http_impl.dart:3107The call site passed
resolved(redirect target URL) asoriginalUrlandprevious.uri(original request URL) asredirectUri, swapping the intended parameter order. This caused the_isSubdomaincheck to verify the wrong direction:Fix: Swap parameters to match the function signature:
shouldCopyHeaderOnRedirect(header, previous.uri, resolved).Test plan
Transfer-Encoding: gzip, chunkedis now parsed as chunked encodingTransfer-Encoding: chunkedstill works (exact match path preserved)_httptests