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

[v8.x] http2: mitigate reported DoS attacks #29124

Closed
wants to merge 19 commits into from

Conversation

addaleax
Copy link
Member

See #29122 for the PR against master.

jasnell and others added 18 commits August 13, 2019 22:01
Multiple general improvements to http2 internals for
readability and efficiency

[This backport applied to v10.x cleanly but had several
merge conflicts on v8.x.]

PR-URL: nodejs#23984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Key new feature: RFC 8441 `:protocol` support

PR-URL: nodejs#23284
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: nodejs#26990
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#27295
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
PR-URL: nodejs#28448
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
DRY up the `debug()` calls, and in particular, avoid building template
strings before we know whether we need to.
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.
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).

[This backport also applies changes from 83e1b97 and required
some manual work due to the lack of `AllocatedBuffer` on v10.x.
More work was necessary for v8.x, including copying utilities
for `util.h` from more recent Node.js versions.]

Refs: nodejs#26201
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.
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.

[This commit differs from the v12.x one due to the lack of
libuv/libuv@ee24ce900e5714c950b248da2b.
See the comment in the test for more details.]
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.
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).
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.
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.
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.
@addaleax addaleax added security Issues and PRs related to security. v8.x http2 Issues or PRs related to the http2 subsystem. fast-track PRs that do not need to wait for 48 hours to land. labels Aug 14, 2019
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 14, 2019
@addaleax addaleax removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 14, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

The ci stability fixes for AIX never got backported as far as 8.x. I've resumed the build.

@addaleax
Copy link
Member Author

Yeah, I think we had issues with AIX failing during the memory-intensive addon tests on more recent Node.js versions before… I’m okay with ignoring a failure on AIX if this is all that’s standing in the way of a release here.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 14, 2019
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
This includes mitigations for CVE-2019-9512/CVE-2019-9515.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
DRY up the `debug()` calls, and in particular, avoid building template
strings before we know whether we need to.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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).

[This backport also applies changes from 83e1b97 and required
some manual work due to the lack of `AllocatedBuffer` on v10.x.
More work was necessary for v8.x, including copying utilities
for `util.h` from more recent Node.js versions.]

Refs: #26201

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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.

[This commit differs from the v12.x one due to the lack of
libuv/libuv@ee24ce900e5714c950b248da2b.
See the comment in the test for more details.]

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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).

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
This is intended to mitigate CVE-2019-9518.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
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.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019

Unverified

The committer email address is not verified.
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.

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019

Unverified

The committer email address is not verified.
Refs: #27914

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs
Copy link
Member

Landed on v8.x-staging

@BethGriggs BethGriggs closed this Aug 15, 2019
@BethGriggs BethGriggs mentioned this pull request Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants