http2: mitigate reported DoS attacks #29122
Closed
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
This includes mitigations for CVE-2019-9512/CVE-2019-9515.
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).
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.
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).
This is intended to mitigate CVE-2019-9518.
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
added
security
nodejs-github-bot
added
the
lib / src
This was referenced Aug 14, 2019
addaleax
removed
the
lib / src
devsnek
reviewed
Aug 14, 2019
|
CI: https://ci.nodejs.org/job/node-test-pull-request/24979/ (:heavy_check_mark:) |
addaleax
added
the
author ready
Trott
approved these changes
Aug 14, 2019
|
+1 to fast-tracking |
|
Landed in 8ae79c9...ec60b62 |
targos
pushed a commit
to targos/node
that referenced
this pull request
Aug 15, 2019
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>
This was referenced Sep 19, 2019
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.
Moving from https://github.com/nodejs-private/node-private/pull/172 as discussed in the TSC meeting for tomorrow’s security release. See the commit messages for details. @Trott and @jasnell already gave approvals.