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

Wrongly assigned CVE critical vulnerabilities to 16.16.0 #43946

Closed
CrlsMrls opened this issue Jul 22, 2022 · 26 comments
Closed

Wrongly assigned CVE critical vulnerabilities to 16.16.0 #43946

CrlsMrls opened this issue Jul 22, 2022 · 26 comments
Labels
feature request Issues that request new features to be added to Node.js. security Issues and PRs related to security.

Comments

@CrlsMrls
Copy link

CrlsMrls commented Jul 22, 2022

After reading the contributing guidelines, in my opinion this is the best place I found to raise this issue. I understand this may not be correct though, sorry in advance for the inconvenience.

What is the problem this feature will solve?

There are multiple resources that (in my opinion) are wrongly assigning CVE critical vulnerabilities to node.js version 16.16.0.

The goal of this GitHub issue is to raise awareness in the Node.js community, so this situation is fixed.

Our security CICD pipelines are raising these critical vulnerabilities for the latest LTS version of node.js (version 16.16.0)

CVE SEVERITY CVSS PACKAGE VERSION STATUS
CVE-2022-32215 critical 9.10 node 16.16.0 fixed in 18.5.0, 16.20.0, 14.20.0
CVE-2022-32214 critical 9.10 node 16.16.0 fixed in 18.5.0, 16.20.0, 14.20.0
CVE-2022-32213 critical 9.10 node 16.16.0 fixed in 18.5.0, 16.20.0, 14.20.0

This seems wrong to me, because:

On the other hand, there are very well respected vulnerability databases stating this is not fixed yet:

I am not sure how to resolve these discrepancies. Until this is fixed, our security practices are blocking this node.js version, which means we cannot use version 16 at all.

What is the feature you are proposing to solve the problem?

Somebody from the Node.js organization contacts NATIONAL VULNERABILITY DATABASE to fix the issue.

@CrlsMrls CrlsMrls added the feature request Issues that request new features to be added to Node.js. label Jul 22, 2022
@mhdawson
Copy link
Member

Looking at our entries in Hacker 1 which is how we request/publish CVEs it does seems like there was a cut/paste error so that we reported it was fixed in 16.20.0 intead of 16.16.0.

I could not find an option in H1 to update the CVE data so I've send a message asking how can can do this.

Thanks for raising the issue.

@mhdawson
Copy link
Member

@RafaelGSS FYI.

@thejackal
Copy link

@mcollina FYI

@mcollina
Copy link
Member

@mhdawson thanks for taking care of this. I made another typo too apparently in another one.

We really need more eyes on them before hitting publish.

@mcollina
Copy link
Member

One note: I'm not aware why those vulnerabilities were classified as critical by others. THEY ARE NOT.

Update at your own pace, you are almost certainly not at risk as they apply only in very specific conditions with limited impact.

@mhdawson
Copy link
Member

No answer to my question on how to update yet and I'm out until next Tuesday. WIll check when I get back.

@texiontech
Copy link

texiontech commented Jul 25, 2022

We have got this report issue from Aqua-scan too.

image

@mcollina
Copy link
Member

@MylesBorins what would be the best way to notify the Github Security Advisories team about this change?

@nil4
Copy link

nil4 commented Jul 25, 2022

https://github.com/github/advisory-database#contributions suggests a couple of approaches that might help.

@MoLow MoLow added the security Issues and PRs related to security. label Jul 25, 2022
@mhdawson
Copy link
Member

Trying to follow up with a contact we have at H1 as I'm still waiting on a response through the help system.

@mcollina
Copy link
Member

I checked and it's not possible to fix it via GitHub as the information is not copied in that registry.

@mhdawson
Copy link
Member

I reached out to our contact at H1 and they indicated they would help point me in the right direction. Will update here when I have more info.

@aberezovski
Copy link

Hey guys,

I was thinking to open similar issue, but found this one.
Regarding NVD confusion, I was convinced that it was a copy/paste issue regarding the Node.js versioning in NVD vulnerabilities like CVE-2022-32214.
The original NVD statement says vulnerability applicable:

  • from (including) 16.0.0 up to (excluding) 16.20.0

but the update from July 27th says vulnerability applicable:

  • from (including) 16.0.0 up to (including) 16.12.0
  • from (including) 16.13.0 up to (excluding) 16.16.0

Apart of that I would like to understand how did the above mentioned commit 1da22eb fix the issue?
The July 7th Security Release mentions that all three CVEs related to llhttp library issue were fixed by using llhttp v6.0.7 where tag v6.0.7 was added on July 6th.

I found the line project(llhttp VERSION 6.0.5) in the Node.js 16.16.0 source code, but according to security release the fix is in llhttp v6.0.7.

Could you clarify what does the sticked llhttp version 6.0.5 in the file deps/llhttp/CMakeLists.txt mean?
And if there is nothing missed to provide a proper fix for the discussed CVEs?

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2022

I've gotten info on how to request an update to the CVEs. Adding here so that we have a record (although I'll think about whether we have a good place to PR into our docs/guidance as well)

Go to the “CVE IDs” section in your program sections (https://hackerone.com/:handle/cve_requests)
Click the “Request a CVE ID” button
Enter the CVE ID that needs to be updated 
    Include all the details that need updating within the form
Submit the request

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2022

The link I think should be - https://hackerone.com/nodejs/cve_requests

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2022

Ok updates are now submitted. They still need to be processed on the H1 side before we'll see the updates externally.

@aberezovski
Copy link

Hey guys,

Could you confirm that commit 1da22eb really fixed the issue?
And there is not any typo in the file deps/llhttp/CMakeLists.txt, and no adjustment (like mentioned below) is required in that sense?

  • From: project(llhttp VERSION 6.0.5)
  • To: project(llhttp VERSION 6.0.7)

Thank you in advance and be well.

@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2022

@aberezovski I think it is an artifact on how llhttp is updated for vulnerabilities. Since updating llhttp in advance would dislose the vulnerability, the commits for the security release were directly landed in the node repo. There was then a later llhttp release which incorporated the changes.

I had to fix a similar issue in the past - https://github.com/nodejs/node/pull/43029/files. It looks to me like CMakeList.txt is an additional file not in the llnode repo that is maintained separately and needs to be updated independently as well.

Since https://github.com/nodejs/node/blob/main/deps/llhttp/include/llhttp.h says 6.0.7 I think updating CmakeList.txt was just missed. @ShogunPanda can you confirm that is correct?

We still need to better document the update process for a security release to avoid the confusion.

mhdawson added a commit to mhdawson/io.js that referenced this issue Aug 4, 2022
Refs: nodejs#43946

- Add instructions on updating llhttp version in
  file maintained only in Node.js repository as it
  needs to be kept in sync with what is copied from
  the llhttp repository when an update is made.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2022

PR to add missing step to update the CMakeList.txt to llhttp update instructions. #44136

It may not have fixed this case where we do things a bit differently for security releases but still needed and will help avoid us missing it once we document the security release specific flow.

@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2022

@aberezovski if you believe my analysis is incorrect and we have actually missed something, please report through H1 so that we can handle as a additional/new vulnerability - see https://github.com/nodejs/node/security/policy

@ShogunPanda
Copy link
Contributor

@mhdawson Yes, I do confirm it.

llhttp is updated first in private, then incorporated in node and then released publicly.
Another related issue is that llhttp release procedure was not streamlined yet and this resulted in some inconsistencies over the past.

I'm trying to address this in nodejs/llhttp#173

@aberezovski
Copy link

@mhdawson, I am OK with your arguments.
I was not aware about another place where the llhttp version was tracked.
Anyway, either fixing the inconsistency caused by two sources for llhttp versions, or keeping only one source of versioning is exactly way to go.
Thanks for clarification.

mhdawson added a commit to mhdawson/io.js that referenced this issue Aug 24, 2022
Refs: nodejs#43946

- Add instructions on updating llhttp version in
  file maintained only in Node.js repository as it
  needs to be kept in sync with what is copied from
  the llhttp repository when an update is made.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 2, 2023
@RafaelGSS
Copy link
Member

Is it solved @ShogunPanda @mhdawson? In case not, how can I help? I think it should be updated in https://github.com/nodejs/security-wg/blob/main/vuln/core/94.json as well.

@mhdawson mhdawson removed the stale label Feb 2, 2023
@mhdawson
Copy link
Member

mhdawson commented Feb 2, 2023

I had asked Hacker one how to best do this but never got an answer. We'll have to follow up again.

@CrlsMrls
Copy link
Author

I believe it is safe to close this ticket, as the version 16.16.0 is deprecated.

Thank you all for your feedback and work. 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. security Issues and PRs related to security.
Projects
None yet
Development

No branches or pull requests

10 participants