[v14.x] http2: treat non-EOF empty frames like other invalid frames #38673
Conversation
This file contains hidden or 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
github-actions bot added c++ 
Collaborator
Collaborator
Member
| Sorry I missed this PR. Can you please rebase? |
targos force-pushed the backport-37875-v14.x branch from 225a893 to 25a4df3 Compare Collaborator
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>
targos force-pushed the backport-37875-v14.x branch from 25a4df3 to 990ef98 Compare Collaborator
Collaborator
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
Member
| Landed on v14.x-staging, thanks! |
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 6, 2021
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>
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
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.
Backport of #37875