http: guard against response splitting in trailers #2945
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note to self: typo in first commit log, s/storeHeader/setHeader/. |
|
this is |
|
I would prefer if this was just throwing on any invalid input instead of trying to hide the issue in the user code. Btw, by «invalid input» I mean not just |
That didn't escape my notice but that's a tangential issue. This PR just fixes the inconsistency between normal and trailing headers. Can I get one or two LGTMs? |
|
Not a measurable one, no.
Maybe in a follow-up PR, I'd like to keep this one free of policy changes. |
|
lgtm, as a first pass at attacking this, we can do more in later PRs where debate can be had, this is simpler to 👍 |
|
LGTM. |
The test verified the output of http.OutgoingMessage#writeHead() but not http.OutgoingMessage#setHeader(). Also check the response body. PR-URL: nodejs#2945 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Rod Vagg <r@va.gg>
Commit 3c293ba ("http: protect against response splitting attacks") filters out newline characters from HTTP headers but forgot to apply the same logic to trailing HTTP headers, i.e., headers that come after the response body. This commit rectifies that. The expected security impact is low because approximately no one uses trailing headers. Some HTTP clients can't even parse them. PR-URL: nodejs#2945 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Rod Vagg <r@va.gg>
bnoordhuis
force-pushed
the
http-trailers-response-splitting
branch
from
8035797
to
e68a119
Compare
bnoordhuis
added a commit
that referenced
this pull request
Sep 20, 2015
The test verified the output of http.OutgoingMessage#writeHead() but not http.OutgoingMessage#setHeader(). Also check the response body. PR-URL: #2945 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Rod Vagg <r@va.gg>
bnoordhuis
added a commit
that referenced
this pull request
Sep 20, 2015
Commit 3c293ba ("http: protect against response splitting attacks") filters out newline characters from HTTP headers but forgot to apply the same logic to trailing HTTP headers, i.e., headers that come after the response body. This commit rectifies that. The expected security impact is low because approximately no one uses trailing headers. Some HTTP clients can't even parse them. PR-URL: #2945 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Rod Vagg <r@va.gg>
rvagg
added a commit
that referenced
this pull request
Sep 22, 2015
Notable changes * buffer: Fixed a bug introduced in v4.1.0 where allocating a new zero-length buffer can result in the next allocation of a TypedArray in JavaScript not being zero-filled. In certain circumstances this could result in data leakage via reuse of memory space in TypedArrays, breaking the normally safe assumption that TypedArrays should be always zero-filled. (Trevor Norris) #2931. * http: Guard against response-splitting of HTTP trailing headers added via response.addTrailers() by removing new-line ([\r\n]) characters from values. Note that standard header values are already stripped of new-line characters. The expected security impact is low because trailing headers are rarely used. (Ben Noordhuis) #2945. * npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full details (Kat Marchán) #2958 - Upgrades graceful-fs on multiple dependencies to no longer rely on monkey-patching fs - Fix npm link for pre-release / RC builds of Node * v8: Update post-mortem metadata to allow post-mortem debugging tools to find and inspect: - JavaScript objects that use dictionary properties (Julien Gilli) #2959 - ScopeInfo and thus closures (Julien Gilli) #2974
rvagg
added a commit
that referenced
this pull request
Sep 23, 2015
Notable changes * buffer: Fixed a bug introduced in v4.1.0 where allocating a new zero-length buffer can result in the next allocation of a TypedArray in JavaScript not being zero-filled. In certain circumstances this could result in data leakage via reuse of memory space in TypedArrays, breaking the normally safe assumption that TypedArrays should be always zero-filled. (Trevor Norris) #2931. * http: Guard against response-splitting of HTTP trailing headers added via response.addTrailers() by removing new-line ([\r\n]) characters from values. Note that standard header values are already stripped of new-line characters. The expected security impact is low because trailing headers are rarely used. (Ben Noordhuis) #2945. * npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full details (Kat Marchán) #2958 - Upgrades graceful-fs on multiple dependencies to no longer rely on monkey-patching fs - Fix npm link for pre-release / RC builds of Node * v8: Update post-mortem metadata to allow post-mortem debugging tools to find and inspect: - JavaScript objects that use dictionary properties (Julien Gilli) #2959 - ScopeInfo and thus closures (Julien Gilli) #2974 PR-URL: #2995
This was referenced Jun 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit 3c293ba ("http: protect against response splitting attacks")
filters out newline characters from HTTP headers but forgot to apply
the same logic to trailing HTTP headers, i.e., headers that come after
the response body. This commit rectifies that.
The expected security impact is low because approximately no one uses
trailing headers. Some HTTP clients can't even parse them.
R=@ChALkeR
CI: https://ci.nodejs.org/job/node-test-pull-request/338/