Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: guard against response splitting in trailers #2945

Merged
merged 2 commits into from
Sep 18, 2015

Conversation

bnoordhuis
Copy link
Member

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/

@bnoordhuis
Copy link
Member Author

Note to self: typo in first commit log, s/storeHeader/setHeader/.

@rvagg rvagg mentioned this pull request Sep 18, 2015
@rvagg
Copy link
Member

rvagg commented Sep 18, 2015

this is land-on-v0.12 and maybe land-on-v0.10 isn't it?

@ChALkeR
Copy link
Member

ChALkeR commented Sep 18, 2015

I would prefer if this was just throwing on any invalid input instead of trying to hide the issue in the user code.
But if throwing is not acceptable, this one is probably fine here.
Another note in the mailing list.

Btw, by «invalid input» I mean not just \n/\r, but anything that is not allowed by the spec as header keys.

@bnoordhuis
Copy link
Member Author

Btw, by «invalid input» I mean not just \n/\r, but anything that is not allowed by the spec as header keys.

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?

@ChALkeR
Copy link
Member

ChALkeR commented Sep 18, 2015

  1. Old logic: if (…) value = …, new logic: value = (…) ? … : value. Does that have a performance impact?
  2. Could you also apply the same function on field?

@bnoordhuis
Copy link
Member Author

Old logic: if (…) value = …, new logic: value = (…) ? … : value. Does that have a performance impact?

Not a measurable one, no.

Could you also apply the same function on field?

Maybe in a follow-up PR, I'd like to keep this one free of policy changes.

@rvagg
Copy link
Member

rvagg commented Sep 18, 2015

lgtm, as a first pass at attacking this, we can do more in later PRs where debate can be had, this is simpler to 👍

@ChALkeR
Copy link
Member

ChALkeR commented Sep 18, 2015

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 bnoordhuis force-pushed the http-trailers-response-splitting branch from 8035797 to e68a119 Compare September 18, 2015 19:37
@bnoordhuis bnoordhuis closed this Sep 18, 2015
@bnoordhuis bnoordhuis deleted the http-trailers-response-splitting branch September 18, 2015 19:37
@bnoordhuis bnoordhuis merged commit e68a119 into nodejs:master Sep 18, 2015
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 rvagg mentioned this pull request Sep 22, 2015
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
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