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

CVE-2022-3517 (npm) found on v14.x (Manual) #88

Closed
mhdawson opened this issue Nov 8, 2022 · 19 comments
Closed

CVE-2022-3517 (npm) found on v14.x (Manual) #88

mhdawson opened this issue Nov 8, 2022 · 19 comments

Comments

@mhdawson
Copy link
Member

mhdawson commented Nov 8, 2022

This public CVE is reported against minimatch 3.0.4 which is a dependency of the version of npm in Node.js 14.x.

@nodejs/npm could you help us with an assessment/statement on the severity applicability of this CVE in the context of npm's usage of minimatch?

@mhdawson
Copy link
Member Author

mhdawson commented Nov 8, 2022

@facutuesca wondering why this was not automatically found as a dependency of npm. We may want to make sure that the current query against the GitHub data should find vulns in all depedencies. Or possibly somebody at GitHub/npm had looked at it and the suppresed the report. Not sure if they do any of that in the databasel.

@mhdawson
Copy link
Member Author

mhdawson commented Nov 8, 2022

@facutuesca interestingly the CVE includes that node.js is affeted but not npm, that might be one of the reasons, I wonder if we should expand our query against the database to include nodejs in addition to dependencies? I don't that that addresses the question of not finding CVEs reported againts the full dep tree, but probably would have caught this one earlier.

@facutuesca
Copy link
Contributor

@mhdawson Yes, the problem is that the CVE is reported against the following CPE:

cpe:2.3:a:minimatch_project:minimatch:*:*:*:*:*:node.js:*:*

which means the minimatch package, running under the node.js software environment. Since we only look for vulnerabilities of direct Node dependencies that match:

cpe:2.3:a:$DEPENDENCY_VENDOR:$DEPENDENCY_NAME:*:*:*:*:*:*:*:*

(that is, any CVE for a direct dependency, no matter which environment is running on), we didn't get a warning because minimatch is not a direct dependency.

My question for this report would be, how is Node affected by the vuln in minimatch? Since minimatch is a dependency of npm, isn't npm itself vulnerable? Or is npm somehow safe, and only node is vulnerable?

@mhdawson
Copy link
Member Author

mhdawson commented Nov 8, 2022

@facutuesca I suspect that there may be a number of vulnerabilities which are only reported against an npm module versus all of the modules which use it. Is there no recursive option for npm modules in the GitHub APIs? npm audit will report vulns all the way down the dependency tree and I think we want to do the same in the checks in this repo.

In terms of:

My question for this report would be, how is Node affected by the vuln in minimatch? Since minimatch is a dependency of npm, isn't npm itself vulnerable? Or is npm somehow safe, and only node is vulnerable?

I think that is likely just a result of the complexity of the problem, an incorrect choice in terms of flagging what is vulnerable.

@RafaelGSS
Copy link
Member

I think we need to look at it recursively. How complex would it be @facutuesca?

@facutuesca
Copy link
Contributor

@mhdawson @RafaelGSS

Is there no recursive option for npm modules in the GitHub APIs?

As far as I can tell, no. The Github Advisory Database does not contain information about dependencies of a given package.

I think that is likely just a result of the complexity of the problem, an incorrect choice in terms of flagging what is vulnerable.

I would agree, the report should have said that npm is vulnerable (unless there's a way for node to be vulnerable without npm being vulnerable).

I think we need to look at it recursively. How complex would it be @facutuesca?

I would assume that for npm packages like acorn, we could simply call npm audit on it and parse the results. If we can do the same thing for npm itself, that should cover it.

The tradeoff here is that we would be creating issues for any vulnerability present in the dependency trees of all of our dependencies. So if we have that:

Node depends-on A depends-on B depends-on C depends-on D

and a vulnerability for D is reported, we'll need to determine if C is vulnerable, and then if B is vulnerable because of that, and then if A is, and finally if Node is

@RafaelGSS
Copy link
Member

Yes, we've discussed it in the last Security WG meeting. We agreed on testing the recursive approach, and in case it adds a lot of noise, we revert it.

@mhdawson
Copy link
Member Author

Node depends-on A depends-on B depends-on C depends-on D

Is what tools report as problems so it will be good for us to have issues/be aware of all of those in this repo.

We of course will only want them for the specific versions that we have in Node.js under deps versus anything else.

I'm hoping it should be the exception where any vulns have not already been reported, but we should assess from some initial runs before turning on.

@mhdawson
Copy link
Member Author

mhdawson commented Nov 14, 2022

@nodejs/npm is this something you can help with. If you can comment on the issue with a "yes we'll take a look" or the contrary that would help use figure out the next step for the project.

@mhdawson
Copy link
Member Author

@nodejs/npm

@darcyclarke
Copy link
Member

Quick update on this; I've queued up a v6.x npm release that I think might be able to address any kind of outstanding production transitive dependency CVE warnings folks are seeing for [email protected] (ref. npm/cli#5855) - we should be able to cut this in the next week or so & circle back to ensure this resolves the problem you're seeing here once it's out (we'll need to land that in node@14)

@mhdawson
Copy link
Member Author

@darcyclarke many thanks for the update!

@mhdawson
Copy link
Member Author

@nlf, @lukekarrys any update on the v6.x release that was queued up?

@mhdawson
Copy link
Member Author

@nodejs/npm @nlf @lukekarrys is there a better way to reach out to the team at npm? @darcyclarke mentioned you two would be the best contacts now that he's left npm.

@ruyadorno
Copy link
Member

Following up with the release here: npm/cli#5988

@lukekarrys
Copy link
Member

v6.14.18 has been released! thank you to @ruyadorno for doing the heavy lifting to get it over the finish line 🎉

@mhdawson
Copy link
Member Author

@lukekarrys, @ruyadorno many thanks.

@nodejs/releasers we should try to schedule a 14.x release to pick up the new npm v6.14.18 which should cover a number of issues reported in this repo. I think the next regularly planned one is in Feb, but I think it would be good to see if we can do one early Jan to pull in the new npm if possible.

@ruyadorno
Copy link
Member

Here's the backport PR to v14.x: nodejs/node#45936

@mhdawson
Copy link
Member Author

Believe this was addressed by recent security release, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants