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

http2: mitigate reported DoS attacks #29122

Closed
wants to merge 13 commits into from

Conversation

@addaleax
Copy link
Member

commented Aug 14, 2019

Moving from nodejs-private/node-private#172 as discussed in the TSC meeting for tomorrow’s security release. See the commit messages for details. @Trott and @jasnell already gave approvals.

addaleax added 12 commits Aug 5, 2019
http2: improve JS-side debug logging
DRY up the `debug()` calls, and in particular, avoid building template
strings before we know whether we need to.
http2: only call into JS when necessary for session events
For some JS events, it only makes sense to call into JS when there
are listeners for the event in question.

The overhead is noticeable if a lot of these events are emitted during
the lifetime of a session. To reduce this overhead, keep track of
whether any/how many JS listeners are present, and if there are none,
skip calls into JS altogether.

This is part of performance improvements to mitigate CVE-2019-9513.
http2: do not create ArrayBuffers when no DATA received
Lazily allocate `ArrayBuffer`s for the contents of DATA frames.
Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8.

This is part of performance improvements to mitigate CVE-2019-9513.

Together with the previous commit, these changes improve throughput
in the adversarial case by about 100 %, and there is little more
that we can do besides artificially limiting the rate of incoming
metadata frames (i.e. after this patch, CPU usage is virtually
exclusively in libnghttp2).
http2: limit number of rejected stream openings
Limit the number of streams that are rejected upon creation. Since
each such rejection is associated with an `NGHTTP2_ENHANCE_YOUR_CALM`
error that should tell the peer to not open any more streams,
continuing to open streams should be read as a sign of a misbehaving
peer. The limit is currently set to 100 but could be changed or made
configurable.

This is intended to mitigate CVE-2019-9514.
deps: update nghttp2 to 1.39.2
This includes mitigations for CVE-2019-9512/CVE-2019-9515.
http2: handle 0-length headers better
Ignore headers with 0-length names and track memory for headers
the way we track it for other HTTP/2 session memory too.

This is intended to mitigate CVE-2019-9516.
http2: shrink default `vector::reserve()` allocations
Allocating memory upfront comes with overhead, and in particular,
`std::vector` implementations do not necessarily return memory
to the system when one might expect that (e.g. after shrinking the
vector).
http2: consider 0-length non-end DATA frames an error
This is intended to mitigate CVE-2019-9518.
http2: stop reading from socket if writes are in progress
If a write to the underlying socket finishes asynchronously, that
means that we cannot write any more data at that point without waiting
for it to finish. If this happens, we should also not be producing any
more input.

This is part of mitigating CVE-2019-9511/CVE-2019-9517.
http2: pause input processing if sending output
If we are waiting for the ability to send more output, we should not
process more input. This commit a) makes us send output earlier,
during processing of input, if we accumulate a lot and b) allows
interrupting the call into nghttp2 that processes input data
and resuming it at a later time, if we do find ourselves in a position
where we are waiting to be able to send more output.

This is part of mitigating CVE-2019-9511/CVE-2019-9517.
http2: limit number of invalid incoming frames
Limit the number of invalid input frames, as they may be pointing towards a
misbehaving peer. The limit is currently set to 1000 but could be changed or
made configurable.

This is intended to mitigate CVE-2019-9514.
http2: allow security revert for Ping/Settings Flood
nghttp2 has updated its limit for outstanding Ping/Settings ACKs
to 1000. This commit allows reverting to the old default of 10000.

The associated CVEs are CVE-2019-9512/CVE-2019-9515.
@nodejs-github-bot

This comment has been minimized.

Copy link

commented Aug 14, 2019

@nodejs-github-bot

This comment has been minimized.

@addaleax addaleax removed the lib / src label Aug 14, 2019

@nodejs-github-bot

This comment has been minimized.

Copy link

commented Aug 14, 2019

@Trott
Trott approved these changes Aug 14, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

+1 to fast-tracking

@targos

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Landed in 8ae79c9...ec60b62

@targos targos closed this Aug 15, 2019

targos added a commit to targos/node that referenced this pull request Aug 15, 2019
deps: update nghttp2 to 1.39.2
This includes mitigations for CVE-2019-9512/CVE-2019-9515.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit that referenced this pull request Aug 18, 2019
http2: remove security revert flags
As the comment in `node_revert.h` indicates, the master branch should
not provide security revert flags.

Refs: #29122

PR-URL: #29141
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cTn-dev

This comment has been minimized.

Copy link

commented Aug 19, 2019

@addaleax Hi Anna, i don't know if this the right place to ask about this (quite a few commits for http2 went into the 10.16.3 LTS).

Was there any "behavior" change when it comes to ClientHttp2Stream errors?
http/2 client code that i have been in running in production for about a year managed to break down on me several times (after few hours of uptime) after upgrading to 10.16.3.

I traced it down to ClientHttp2Stream firing the error event handler with either of these 2 errors
Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM
Stream closed with error code NGHTTP2_INTERNAL_ERROR

I handle the error gracefully but what happens is that the application enters a weird lock/loop when attempting to send another request.

No errors (that i can see have been risen on the active http2session), so in the future when application calls the http2session.request(options) again it results in the error handler firing with one of the 2 errors just as before.
Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM
Stream closed with error code NGHTTP2_INTERNAL_ERROR

This effectively blocks the app from sending out any requests from that point onward.

  1. I was under the impression that the "broken" stream will be destroyed after the error fires and subsequent call to http2session.request will open a new one, is this not the case?
  2. Should i close/destroy the underlying http2session when i get any of the errors above ?

Is there any additional documentation/article where i could read up upon the http/2 error flow?

BTW:

  1. I am fully aware that the issue might be on my end, i am just trying to figure out why the implementation is suddenly breaking.
  2. I haven't provided an "code example" in this case as it would take a significant amount of time to strip down of all the redundant code, i will do that if its the only option.

Thank you for your time

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Hi @cTn-dev, thanks for commenting here!

Was there any "behavior" change when it comes to ClientHttp2Stream errors?

The changes here affect both client and server side in the same way.

I traced it down to ClientHttp2Stream firing the error event handler with either of these 2 errors
Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM
Stream closed with error code NGHTTP2_INTERNAL_ERROR

That means you’re likely exceeding some of the limits which have been introduced or changed here. Does running node as node --security-reverts=CVE-2019-9514 help (possibly on the other side of the connection)? Some changes, in this case for the CVEs ending in -9512, -9514, -9516 and -9518, can be disabled for Node.js, in cases of unexpected or undesirable breakage.

No errors (that i can see have been risen on the active http2session), so in the future when application calls the http2session.request(options) again it results in the error handler firing with one of the 2 errors just as before.

I’m guessing that when a limit is exceeded, attempting to open more streams is also going to fail – it’s hard to tell without knowing more about what happens.

  1. I was under the impression that the "broken" stream will be destroyed after the error fires and subsequent call to http2session.request will open a new one, is this not the case?

I am under that same impression, yes.

  1. Should i close/destroy the underlying http2session when i get any of the errors above ?

If these errors are for specific streams, the HTTP/2 session should generally remain usable, but I guess that depends on what the other side does exactly.

Is there any additional documentation/article where i could read up upon the http/2 error flow?

Maybe somebody in @nodejs/http2 knows more about this?

  1. I haven't provided an "code example" in this case as it would take a significant amount of time to strip down of all the redundant code, i will do that if its the only option.

Having a reproduction, or at least a better understanding of the usage pattern, would probably indeed help figure out what the right behavior is here?

Security fixes are always going to be a bit unfortunate in that they land in LTS releases without a baking-in phase and are mostly developed without community input, so there’s always a higher chance of unintended breakage.

JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
deps: update nghttp2 to 1.39.2
This includes mitigations for CVE-2019-9512/CVE-2019-9515.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: improve JS-side debug logging
DRY up the `debug()` calls, and in particular, avoid building template
strings before we know whether we need to.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: only call into JS when necessary for session events
For some JS events, it only makes sense to call into JS when there
are listeners for the event in question.

The overhead is noticeable if a lot of these events are emitted during
the lifetime of a session. To reduce this overhead, keep track of
whether any/how many JS listeners are present, and if there are none,
skip calls into JS altogether.

This is part of performance improvements to mitigate CVE-2019-9513.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: do not create ArrayBuffers when no DATA received
Lazily allocate `ArrayBuffer`s for the contents of DATA frames.
Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8.

This is part of performance improvements to mitigate CVE-2019-9513.

Together with the previous commit, these changes improve throughput
in the adversarial case by about 100 %, and there is little more
that we can do besides artificially limiting the rate of incoming
metadata frames (i.e. after this patch, CPU usage is virtually
exclusively in libnghttp2).

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: limit number of rejected stream openings
Limit the number of streams that are rejected upon creation. Since
each such rejection is associated with an `NGHTTP2_ENHANCE_YOUR_CALM`
error that should tell the peer to not open any more streams,
continuing to open streams should be read as a sign of a misbehaving
peer. The limit is currently set to 100 but could be changed or made
configurable.

This is intended to mitigate CVE-2019-9514.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: limit number of invalid incoming frames
Limit the number of invalid input frames, as they may be pointing
towards a misbehaving peer. The limit is currently set to 1000 but
could be changed or made configurable.

This is intended to mitigate CVE-2019-9514.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: handle 0-length headers better
Ignore headers with 0-length names and track memory for headers
the way we track it for other HTTP/2 session memory too.

This is intended to mitigate CVE-2019-9516.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: shrink default `vector::reserve()` allocations
Allocating memory upfront comes with overhead, and in particular,
`std::vector` implementations do not necessarily return memory
to the system when one might expect that (e.g. after shrinking the
vector).

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: consider 0-length non-end DATA frames an error
This is intended to mitigate CVE-2019-9518.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: stop reading from socket if writes are in progress
If a write to the underlying socket finishes asynchronously, that
means that we cannot write any more data at that point without waiting
for it to finish. If this happens, we should also not be producing any
more input.

This is part of mitigating CVE-2019-9511/CVE-2019-9517.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: pause input processing if sending output
If we are waiting for the ability to send more output, we should not
process more input. This commit a) makes us send output earlier,
during processing of input, if we accumulate a lot and b) allows
interrupting the call into nghttp2 that processes input data
and resuming it at a later time, if we do find ourselves in a position
where we are waiting to be able to send more output.

This is part of mitigating CVE-2019-9511/CVE-2019-9517.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: allow security revert for Ping/Settings Flood
nghttp2 has updated its limit for outstanding Ping/Settings ACKs
to 1000. This commit allows reverting to the old default of 10000.

The associated CVEs are CVE-2019-9512/CVE-2019-9515.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: remove security revert flags
As the comment in `node_revert.h` indicates, the master branch should
not provide security revert flags.

Refs: nodejs#29122

PR-URL: nodejs#29141
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
deps: update nghttp2 to 1.39.2
This includes mitigations for CVE-2019-9512/CVE-2019-9515.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: improve JS-side debug logging
DRY up the `debug()` calls, and in particular, avoid building template
strings before we know whether we need to.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: only call into JS when necessary for session events
For some JS events, it only makes sense to call into JS when there
are listeners for the event in question.

The overhead is noticeable if a lot of these events are emitted during
the lifetime of a session. To reduce this overhead, keep track of
whether any/how many JS listeners are present, and if there are none,
skip calls into JS altogether.

This is part of performance improvements to mitigate CVE-2019-9513.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: do not create ArrayBuffers when no DATA received
Lazily allocate `ArrayBuffer`s for the contents of DATA frames.
Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8.

This is part of performance improvements to mitigate CVE-2019-9513.

Together with the previous commit, these changes improve throughput
in the adversarial case by about 100 %, and there is little more
that we can do besides artificially limiting the rate of incoming
metadata frames (i.e. after this patch, CPU usage is virtually
exclusively in libnghttp2).

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: limit number of rejected stream openings
Limit the number of streams that are rejected upon creation. Since
each such rejection is associated with an `NGHTTP2_ENHANCE_YOUR_CALM`
error that should tell the peer to not open any more streams,
continuing to open streams should be read as a sign of a misbehaving
peer. The limit is currently set to 100 but could be changed or made
configurable.

This is intended to mitigate CVE-2019-9514.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: limit number of invalid incoming frames
Limit the number of invalid input frames, as they may be pointing
towards a misbehaving peer. The limit is currently set to 1000 but
could be changed or made configurable.

This is intended to mitigate CVE-2019-9514.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: handle 0-length headers better
Ignore headers with 0-length names and track memory for headers
the way we track it for other HTTP/2 session memory too.

This is intended to mitigate CVE-2019-9516.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: shrink default `vector::reserve()` allocations
Allocating memory upfront comes with overhead, and in particular,
`std::vector` implementations do not necessarily return memory
to the system when one might expect that (e.g. after shrinking the
vector).

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: consider 0-length non-end DATA frames an error
This is intended to mitigate CVE-2019-9518.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: stop reading from socket if writes are in progress
If a write to the underlying socket finishes asynchronously, that
means that we cannot write any more data at that point without waiting
for it to finish. If this happens, we should also not be producing any
more input.

This is part of mitigating CVE-2019-9511/CVE-2019-9517.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: pause input processing if sending output
If we are waiting for the ability to send more output, we should not
process more input. This commit a) makes us send output earlier,
during processing of input, if we accumulate a lot and b) allows
interrupting the call into nghttp2 that processes input data
and resuming it at a later time, if we do find ourselves in a position
where we are waiting to be able to send more output.

This is part of mitigating CVE-2019-9511/CVE-2019-9517.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: allow security revert for Ping/Settings Flood
nghttp2 has updated its limit for outstanding Ping/Settings ACKs
to 1000. This commit allows reverting to the old default of 10000.

The associated CVEs are CVE-2019-9512/CVE-2019-9515.

PR-URL: nodejs#29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: remove security revert flags
As the comment in `node_revert.h` indicates, the master branch should
not provide security revert flags.

Refs: nodejs#29122

PR-URL: nodejs#29141
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.