http2: treat non-EOF empty frames like other invalid frames #37875

addaleax · 2021-03-23T14:52:50Z

http2: fix setting options before handle exists

Currently, when a JS Http2Session object is created, we have
to handle the situation in which the native object corresponding
to it does not yet exist. As part of that, we create a typed array
for storing options that are passed through the AliasedStruct
mechanism, and up until now, we copied that typed array over
the native one once the native one was available.

This was not good, because it was overwriting the defaults that
were set during construction of the native typed array with zeroes.

In order to fix this, create a wrapper for the JS-created typed array
that keeps track of which fields were changed, which enables us to
only overwrite fields that were intentionally changed on the JS side.

It is surprising that this behavior was not tested (which is,
guessing from the commit history around these features, my fault).
The subseqeuent commit introduces a test that would fail without
this change.

http2: treat non-EOF empty frames like other invalid frames

Use the existing mechanism that we have to keep track of invalid frames
for treating this specific kind of invalid frame.

The commit that originally introduced this check was 695e38b,
which was supposed to proected against CVE-2019-9518, which in turn
was specifically about a flood of empty data frames. While these are
still invalid frames either way, it makes sense to be forgiving here
and just treat them like other invalid frames, i.e. to allow a small
(configurable) number of them.

Fixes: #37849

@nodejs/http2

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change.

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: nodejs#37849

mcollina

lgtm

nodejs-github-bot · 2021-03-23T16:09:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36930/ (💚)

lib/internal/http2/core.js

blakebyrnes · 2021-03-23T16:47:17Z

src/node_http2.cc

@@ -1335,7 +1335,11 @@ int Http2Session::HandleDataFrame(const nghttp2_frame* frame) {
      frame->hd.flags & NGHTTP2_FLAG_END_STREAM) {
    stream->EmitRead(UV_EOF);
  } else if (frame->hd.length == 0) {
-    return 1;  // Consider 0-length frame without END_STREAM an error.
+    if (invalid_frame_count_++ > js_fields_->max_invalid_frames) {


Should invalid_frame_count be reset if valid frames come through? Pertaining to #37849, if an end server is sending EOF frames after every valid frame, would this solution only allow for single requests in a session?

So... I don't think we'd want to reset after every valid frame, or at least I think that would be a bit separate from this PR.

What would makes sense to me is to make this a ratio of invalid frames/total frames that is being limited, rather than the total # of invalid frames ... but that feels a bit out of scope here :/ In any case, users who do wish to allow for a large number of invalid frames can set the maxSessionInvalidFrames option to a large value, if they are connecting to problematic servers.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2021-03-25T11:26:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36961/

nodejs-github-bot · 2021-03-25T14:48:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36972/ (💚)

addaleax · 2021-03-26T19:52:27Z

Landed in a9cdeed...87aa3f1

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: #37875 Fixes: #37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: #37849 PR-URL: #37875 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: #37875 Fixes: #37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: #37849 PR-URL: #37875 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: #37875 Fixes: #37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: #37849 PR-URL: #37875 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danielleadams · 2021-04-28T22:00:35Z

@addaleax Do you mind opening up a backport PR for 14.x-staging? Thank you!

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: nodejs#37875 Fixes: nodejs#37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: nodejs#37849 PR-URL: nodejs#37875 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: nodejs#37875 Fixes: nodejs#37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: nodejs#37849 PR-URL: nodejs#37875 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: nodejs#37875 Fixes: nodejs#37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: nodejs#37849 PR-URL: nodejs#37875 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: #37875 Backport-PR-URL: #38673 Fixes: #37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use the existing mechanism that we have to keep track of invalid frames for treating this specific kind of invalid frame. The commit that originally introduced this check was 695e38b, which was supposed to proected against CVE-2019-9518, which in turn was specifically about a *flood* of empty data frames. While these are still invalid frames either way, it makes sense to be forgiving here and just treat them like other invalid frames, i.e. to allow a small (configurable) number of them. Fixes: #37849 PR-URL: #37875 Backport-PR-URL: #38673 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>