FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: fix a error in the check for frame->hd.type (related to CVE-2025-23085) by hgqxjj · Pull Request #57644 · nodejs/node · GitHub

/ node Public

http2: fix a error in the check for frame->hd.type (related to CVE-2025-23085) - #57644

Merged
aduh95 merged 3 commits into
nodejs:mainfrom
hgqxjj:patch-1
Apr 9, 2025
ConversationCommits (3)ChecksFiles changed
Merged

http2: fix a error in the check for frame->hd.type (related to CVE-2025-23085)#57644
aduh95 merged 3 commits into
nodejs:mainfrom
hgqxjj:patch-1

Conversation

hgqxjj commented Mar 27, 2025 •
edited by aduh95
Loading

Copy link
Copy Markdown
Contributor

http2: fix a typo in the check for frame->hd.type in 1b693fa

According to the description above, this should be checking
whether frame->hd.type is NGHTTP2_GOAWAY,
and the value of NGHTTP2_GOAWAY is 0x07. However,
it is written as 0x03 here, which I think it is a typo.

Refs: 1b693fa

nodejs-github-bot commented Mar 27, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Mar 27, 2025
RafaelGSS reviewed Mar 27, 2025
View reviewed changes

RafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's correct. Could you please add an additional test assertion to this? See the tests introduced by this change in 1b693fa

codecov Bot commented Mar 27, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.22%. Comparing base (1123585) to head (2279ad5).
Report is 103 commits behind head on main.

Files with missing lines Patch % Lines
src/node_http2.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57644      +/-   ##
==========================================
- Coverage   90.22%   90.22%   -0.01%     
==========================================
  Files         630      630              
  Lines      185055   185055              
  Branches    36216    36212       -4     
==========================================
- Hits       166975   166965      -10     
- Misses      11042    11050       +8     
- Partials     7038     7040       +2
Files with missing lines Coverage Δ
src/node_http2.cc 84.63% <0.00%> (-0.25%) ⬇️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hgqxjj commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

@RafaelGSS hi RafaelGSS, I have reviewed all your test files in 1b693fa, and it seems that none of them cover the Http2Session::OnFrameNotSent function test. I attempted to write a test for it, but it is quite difficult to trigger Http2Session::OnFrameNotSent with frame->hd.type equal to 0x07. Do you have any good ideas on how to achieve this? Alternatively, is it possible to merge this patch first if you think this modification is urgent.

RafaelGSS commented Mar 28, 2025

Copy link
Copy Markdown
Member

@hgqxjj, it seems your change is breaking test-http2-premature-close. This test is historically known for being flaky, so I'll re-run the suite just to make sure.

hgqxjj commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

@RafaelGSS I think my change is unlikely to touch test-http2-premature-close. Maybe it still be flaky, haha

hgqxjj commented Mar 29, 2025

Copy link
Copy Markdown
Contributor Author

@RafaelGSS Unfortunately, the test failed again due to a timeout. I conducted the same test using different versions of Node.js in my side. When running test-http2-premature-close.js with Node.js 14.19, the program terminates as expected. However, when testing with Node.js 20.12, the program gets stuck.

Based on my analysis of the test-http2-premature-close.js script combined with debug logs, there are two errors in the client's header frame content.

In Node.js 14.19, these errors cause the server to send an RST_STREAM upon receiving the header frame. For some reason, this also triggers the onFrameError function, leading to the session being closed, which subsequently triggers server.close(), allowing the program to terminate.

However, in Node.js 20.12, server seems to have ignored these errors and did not send RST_STREAM , and onFrameError is not triggered, and server.close() is not called, causing the program to hang. Therefore, the test failure does not seem to be related to my modifications but rather an issue with test-http2-premature-close.js itself.

hgqxjj changed the title fix a typo in the check for frame->hd.type http2: fix a error in the check for frame->hd.type (relate to CVE-2025-23085) Mar 29, 2025
hgqxjj changed the title http2: fix a error in the check for frame->hd.type (relate to CVE-2025-23085) http2: fix a error in the check for frame->hd.type (related to CVE-2025-23085) Mar 29, 2025

hgqxjj commented Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

@RafaelGSS hi RafaelGSS，i already correct header frame content according to http2 related protocol so that make it exited successfully，please re-running the test suite to make sure at your convenience .

hgqxjj added 2 commits April 1, 2025 15:57
http2: fix an error in an if condition in 1b693fa
2d3a1fd 
According to the description above, this should be
checking whether frame->hd.type is NGHTTP2_GOAWAY,
and the value of NGHTTP2_GOAWAY is 0x07. However,
it is written as 0x03 here, which I think
it is an error.
http2:correct http2 header frame content
cf86853 
previously, due to some error in the content of
the header frame, the script could not exit properly,
which eventually led to a timeout error，this commit
correct header frame content according to http2
related protocol so that make it exited successfully.

hgqxjj commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

hi @RafaelGSS ，currently, all tests have passed. Could you please take some time to review this patch ? Thanks in advance

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025
github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 3, 2025

github-actions Bot commented Apr 3, 2025

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14248886630

mcollina approved these changes Apr 4, 2025
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm, but this should wait @RafaelGSS approval too.

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2025

nodejs-github-bot commented Apr 4, 2025

Copy link
Copy Markdown
Collaborator

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

RafaelGSS commented Apr 7, 2025

Copy link
Copy Markdown
Member

I will review this PR carefully tomorrow. I'll double check the memory leak.

hgqxjj commented Apr 8, 2025

Copy link
Copy Markdown
Contributor Author

hi @RafaelGSS ， I'll paste some screenshots below to help explain the changes I made.
RFC7541 link is https://datatracker.ietf.org/doc/html/rfc7541#appendix-A

RafaelGSS added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 8, 2025
aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 9, 2025
aduh95 merged commit ada4abf into nodejs:main Apr 9, 2025

aduh95 commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

Landed in ada4abf

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
http2: fix check for frame->hd.type
733b6b1 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
http2: fix check for frame->hd.type
406b06b 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
http2: fix check for frame->hd.type
a4fa2cd 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
http2: fix check for frame->hd.type
9f8d08f 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
http2: fix check for frame->hd.type
d90a650 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
http2: fix check for frame->hd.type
dc5322d 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
github-actions Bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
http2: fix check for frame->hd.type
9baf580 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
ghost mentioned this pull request Jun 8, 2025
Closed
marco-ippolito pushed a commit that referenced this pull request Aug 18, 2025
http2: fix check for frame->hd.type
b02517b 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito mentioned this pull request Aug 20, 2025
Merged
marco-ippolito pushed a commit that referenced this pull request Aug 20, 2025
http2: fix check for frame->hd.type
c44fcf3 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 23, 2025
http2: fix check for frame->hd.type
42d2f27 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
http2: fix check for frame->hd.type
1553919 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
http2: fix check for frame->hd.type
28d8f85 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
http2: fix check for frame->hd.type
500b0f7 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 27, 2025
http2: fix check for frame->hd.type
ff5cf8a 
According to the comment, this should be checking whether
`frame->hd.type` is `NGHTTP2_GOAWAY`, i.e. `0x07` and not `0x03`.

PR-URL: #57644
Refs: 1b693fa
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
RafaelGSS RafaelGSS approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL