Fix HTTP response splitting via unvalidated reason phrase in dart:io - #63500
Fix HTTP response splitting via unvalidated reason phrase in dart:io#63500Sengtocxoen wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Please fix CLA issue and make sure to add a regression test. |
e9f9254 to
37a9fab
Compare
|
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/+/508540 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). |
|
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/+/508560 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). |
|
Please add a regression test, as I have asked above. |
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
I miss the test first time, i hope it ok now |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
|
@Sengtocxoen would you like to fix test failures? |
1fa0e1b to
22245af
Compare
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
1 similar comment
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
|
@Sengtocxoen would you like to fix test failures? (also when addressing test failures please rerun tests locally to make sure you have fixed all things!) |
HttpResponse.reasonPhrase was written verbatim into the status line without validation. A reason phrase containing CR/LF (or other control characters) allowed injecting arbitrary headers and body content into the response stream (HTTP response splitting / request smuggling). Validate the reason phrase in the setter with the same _isValidValueString check used for header values, throwing a FormatException on any disallowed control character. Closes dart-lang#63500
22245af to
0fc4e5d
Compare
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
HttpResponse.reasonPhrase was written verbatim into the status line without validation. A reason phrase containing CR/LF (or other control characters) allowed injecting arbitrary headers and body content into the response stream (HTTP response splitting / request smuggling). Validate the reason phrase in the setter with the same _isValidValueString check used for header values, throwing a FormatException on any disallowed control character. Closes dart-lang#63500
0fc4e5d to
bb9fca5
Compare
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
HttpResponse.reasonPhrase was written verbatim into the status line without validation. A reason phrase containing CR/LF (or other control characters) allowed injecting arbitrary headers and body content into the response stream (HTTP response splitting / request smuggling). Validate the reason phrase in the setter with the same _isValidValueString check used for header values, throwing a FormatException on any disallowed control character. Closes dart-lang#63500
bb9fca5 to
531448b
Compare
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
HttpResponse.reasonPhrase was written verbatim into the status line without validation. A reason phrase containing CR/LF (or other control characters) allowed injecting arbitrary headers and body content into the response stream (HTTP response splitting / request smuggling). Validate the reason phrase in the setter with the same _isValidValueString check used for header values, throwing a FormatException on any disallowed control character. Closes dart-lang#63500 Change-Id: I61a50424f7f7a55c3b4240d01f4cce8dc3187edc
HttpResponse.reasonPhrase was written verbatim into the status line without validation. A reason phrase containing CR/LF (or other control characters) allowed injecting arbitrary headers and body content into the response stream (HTTP response splitting / request smuggling). Validate the reason phrase in the setter with the same _isValidValueString check used for header values, throwing a FormatException on any disallowed control character. Closes dart-lang#63500 Change-Id: I61a50424f7f7a55c3b4240d01f4cce8dc3187edc
531448b to
636ca3f
Compare
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
|
@mraleph i think i broke some things that the CL don't run the checking for me any more, can you help me that. Thank you a lot |
|
CL has been abandoned, possibly because it was rejected. Please refer to the CL for more information. |
|
This needs to be rebased. You can just delete changes to CHANGELOG.md, they are not necessary. |
HttpResponse.reasonPhrase was written verbatim into the status line without validation. A reason phrase containing CR/LF (or other control characters) allowed injecting arbitrary headers and body content into the response stream (HTTP response splitting / request smuggling). Validate the reason phrase in the setter with the same _isValidValueString check used for header values, throwing a FormatException on any disallowed control character. Closes dart-lang#63500 Change-Id: I61a50424f7f7a55c3b4240d01f4cce8dc3187edc
b6e9ab4 to
f3b5fb7
Compare
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
|
https://dart-review.googlesource.com/c/sdk/+/508560 has been updated with the latest commits from this pull request. |
|
Gerrit CL has build or test failures, please review them in Gerrit and fix them before requesting another review. |
Problem
HttpResponse.reasonPhraseindart:ioaccepts CRLF characters without validation. Every other HTTP wire-output API in the SDK validates against CRLF via_isValueChar()/_validateValue(). ThereasonPhrasesetter is the single gap.An attacker who controls the value passed to
reasonPhrasecan inject CRLF sequences to split the HTTP response, injecting arbitrary headers and response bodies.API consistency
Fix
Adds CRLF validation to the
reasonPhrasesetter using the existing_HttpHeaders._isValidValueString()function — the same validation already applied to all header values inhttp_headers.dart.void set reasonPhrase(String reasonPhrase) { if (_outgoing.headersWritten) throw StateError("Header already sent"); + var errorAt = _HttpHeaders._isValidValueString(reasonPhrase); + if (errorAt >= 0) { + throw FormatException( + "Invalid HTTP reason phrase", + reasonPhrase, + errorAt, + ); + } _reasonPhrase = reasonPhrase; }Zero new validation infrastructure — reuses the existing
_isValidValueString()that rejects bytes <= 31 (CR, LF, NUL) except HT.Impact
Validation
headers.set('X', 'a\r\nb')→FormatException(existing, correct behavior)reasonPhrase = 'OK\r\n...'→ now also throwsFormatException(new, consistent)