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.

Sign up for GitHub

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

Jump to bottom

Update npm on all supported release lines to address CVE scored 9.8 in minimist package #32296

Closed
mleneveut opened this issue Mar 16, 2020 · 28 comments
Closed

Update npm on all supported release lines to address CVE scored 9.8 in minimist package #32296

mleneveut opened this issue Mar 16, 2020 · 28 comments
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.

Comments

@mleneveut
Copy link

Is your feature request related to a problem? Please describe.
The package mkdir 0.5.1 contains a dependency to minimist 0.0.8, which has the CVE-2020-7598, scored 9.8

Describe the solution you'd like
Remove the package mkdirp or find a maintained alternative.

Others

node -v
v12.16.1

npm -v
6.13.4

list mkdirp
[email protected] /usr/lib/node_modules/npm
+-- [email protected]
| `-- [email protected]  deduped
+-- [email protected]
| `-- [email protected]  deduped
+-- [email protected]
| `-- [email protected]  deduped
+-- [email protected]
| `-- [email protected]  deduped
+-- [email protected]
+-- [email protected]
| +-- [email protected]
| | `-- [email protected]  deduped
| `-- [email protected]  deduped
+-- [email protected]
| `-- [email protected]  deduped
+-- [email protected]
| `-- [email protected]  deduped
`-- [email protected]
  `-- [email protected]  deduped
@mleneveut mleneveut changed the title Remove package mkdirp (obsolete since 2015) which uses minimalist 0.0.8 having CVE scored 9.8 Upgrade dependency mkdirp to 1.0.3 to fix CVE scored 9.8 in minimalist package Mar 16, 2020
@mleneveut
Copy link
Author

seems to have been forked and released in v1.0.3 without the minimalist deps : https://github.com/isaacs/node-mkdirp

@mscdex
Copy link
Contributor

mscdex commented Mar 16, 2020

This should be posted to the npm issue tracker instead.

@mleneveut
Copy link
Author

I did, thx

@mcollina
Copy link
Member

I think we should keep this open because we'll need to issue new releases on all LTS line.

@mcollina mcollina reopened this Mar 16, 2020
@sam-github
Copy link
Contributor

Can the subject be changed to something more specific, is this the plan?

Update npm on all supported release lines to address CVE

Right now, subjects suggests we'll be updating a package deep inside npm's deps, which I assume/hope is not the intention.

@mcollina
Copy link
Member

The intention should be for npm to do releases for all those lines and we'll just backport those to all lts lines

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 16, 2020
@mcollina
Copy link
Member

cc @nodejs/tsc this is important.

@targos
Copy link
Member

targos commented Mar 16, 2020

I think this is not important because mkdirp doesn't use minimist in its API (only in the CLI, which is never used by npm or any of its dependencies).

@mcollina
Copy link
Member

It is because most vulnerability scanners are going to detect this automatically.

@targos
Copy link
Member

targos commented Mar 16, 2020

If we want to quickly fix this on our side, we can probably just rm -rf deps/npm/node_modules/minimist

@sam-github
Copy link
Contributor

I think hacking deps/npm sets a bad precedent, but given a 10.x is going out tomorrow, maybe it can update npm to the latest (assuming latest fixes this).

@sam-github sam-github mentioned this issue Mar 16, 2020
Merged
@richardlau
Copy link
Member

I think hacking deps/npm sets a bad precedent, but given a 10.x is going out tomorrow, maybe it can update npm to the latest (assuming latest fixes this).

AFAIK there isn't a fixed version of npm yet. v6.14.2 (the latest npm release) still has [email protected]: https://github.com/npm/cli/blob/v6.14.2/node_modules/minimist/package.json

Tracking bug: npm/cli#1027

@mcollina mcollina mentioned this issue Mar 17, 2020
Closed
@mleneveut mleneveut changed the title Upgrade dependency mkdirp to 1.0.3 to fix CVE scored 9.8 in minimalist package Update npm on all supported release lines to address CVE scored 9.8 in minimalist package Mar 17, 2020
@MylesBorins
Copy link
Contributor

We've floated patches to npm in the past, fwiw. I would be a bit more comfortable with patching the tree to squelch any dependency warnings than shipping with a version of npm that hasn't gone out in any other release lines.

@lirantal
Copy link
Member

I asked the npm folks in the openjs slack and Darcy confirmed they will be shipping an npm release today so if this can wait out a bit for that maybe that can work

@richardlau
Copy link
Member

There's an OpenSSL update due out today (#32210). That could potentially go out in the same release as the npm update.

@BridgeAR
Copy link
Member

I will wait a tiny bit with the next v13 release to get a fix into that release.

@BridgeAR BridgeAR mentioned this issue Mar 17, 2020
Closed
@rvagg
Copy link
Member

rvagg commented Mar 17, 2020

I don't have an active line to any npm folks these days but if they want to co-ordinate on a node-gyp release then I'd like to hear about it. We have a flagged minimist in our dep tree via mkdirp too but we try and keep our dep ranges roughly in line with npm's too. So for them to ship a "safe" npm will require a "safe" node-gyp.
Can someone pass this on to them: nodejs/node-gyp#2074
Or just contact me directly, my email address is all over the place, or IRC.

(Also, this whole minimist issue is beyond bogus, I hate this binary security culture we have that incentivises certain companies to make package maintainers lives hard).

@rvagg rvagg changed the title Update npm on all supported release lines to address CVE scored 9.8 in minimalist package Update npm on all supported release lines to address CVE scored 9.8 in minimist package Mar 18, 2020
@rvagg
Copy link
Member

rvagg commented Mar 18, 2020

Actually, we (node-gyp) probably don't need to do anything to synchronize, we ship with "mkdirp": "^0.5.1" and Isaac has released an 0.5.3 to update the dependency. We don't ship a package-lock.json, so new installs should take care of this, npm just needs to update their deps and it should be taken care of.

@rvagg rvagg mentioned this issue Mar 18, 2020
Closed
@mleneveut
Copy link
Author

FYI, as Isaac released a 0.5.3 of mkdirp, a simple npm update (actually two) fixes the CVE in a node 12.x :

cd /usr/lib/node_modules/npm/node_modules/rc && npm update
cd /usr/lib/node_modules/npm && npm update

@richardlau richardlau added the npm Issues and PRs related to the npm client dependency or the npm registry. label Mar 18, 2020
@ljharb
Copy link
Member

ljharb commented Mar 18, 2020

What actual vulnerability is being addressed here? The CVE itself seems to indicate that the attack vector is "you can craft a malicious command line argument to attack yourself", which doesn't seem like something particularly urgent.

Additionally, it seems like mkdirp can be updated to v0.5.3 trivially, even as a floating patch. Can that be pulled in and shipped in v13 ASAP, especially if that has to start a two-week clock?

@mcollina
Copy link
Member

What actual vulnerability is being addressed here? The CVE itself seems to indicate that the attack vector is "you can craft a malicious command line argument to attack yourself", which doesn't seem like something particularly urgent.

My opinion is that there is no vulnerability. I strongly disagree on considering this a vulnerability on minimist in all form or fashion. If somebody can manipulate the commands passed to a CLI, they are definitely outside the Node.js threat model (and therefore the one of every app for Node.js). My opinion is that the security process failed the community in this specific case. However, the topic is not if minimist is vulnerable or not, it is to avoid Node.js including a vulnerable piece of code.

The problem is that every vulnerability scanner is going to pinpoint Node.js as vulnerable because those files are on disk. This is causing disruption to all enterprise deployments: most companies have a very strict rule of no known vulnerabilities. As a result, we have to ship releases asap to all lines, without waiting for the 2 weeks period.

cc @nodejs/releasers

@jasnell
Copy link
Member

jasnell commented Mar 18, 2020

I agree 100% with @mcollina. This is unfortunately forcing us to make a release when it should not have. Let's just do it and move on.

@ljharb
Copy link
Member

ljharb commented Mar 18, 2020

Thanks for clarifying, that position makes sense to me.

@BethGriggs
Copy link
Member

v12.x (#32313) and v10.x (#31984) have releases due Tuesday 24th March that the necessary patch/update could be pulled into. Is that timeframe sufficient?

It's possible v13.x could be sooner (@BridgeAR nodejs/Release#487 (comment))

Are we still waiting on a new version of npm or do we just need to float a patch?

@mcollina
Copy link
Member

v12.x (#32313) and v10.x (#31984) have releases due Tuesday 24th March that the necessary patch/update could be pulled into. Is that timeframe sufficient?

I think that's sufficient.

Are we still waiting on a new version of npm or do we just need to float a patch?

I don't think npm has fixed it yet unfortunately.

@MylesBorins
Copy link
Contributor

npm update has landed

npm/cli@cc3122a

going to make a PR rn

@MylesBorins MylesBorins mentioned this issue Mar 19, 2020
Closed
@MylesBorins MylesBorins linked a pull request Mar 19, 2020 that will close this issue
deps: update npm to 6.14.3 #32368
Closed
MylesBorins added a commit that referenced this issue Mar 19, 2020
@MylesBorins
deps: update npm to 6.14.3
4a3ccd8 
PR-URL: #32368
Refs: #32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins added a commit that referenced this issue Mar 19, 2020
@MylesBorins
deps: update npm to 6.14.3
5366682 
PR-URL: #32368
Refs: #32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@mleneveut mleneveut mentioned this issue Mar 20, 2020
Closed
BethGriggs pushed a commit that referenced this issue Mar 23, 2020
@MylesBorins @BethGriggs
deps: update npm to 6.14.3
c2b3cf6 
PR-URL: #32368
Refs: #32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@mhdawson mhdawson mentioned this issue Mar 23, 2020
Closed
MylesBorins added a commit that referenced this issue Mar 24, 2020
@MylesBorins
deps: update npm to 6.14.3
26e9d5f 
PR-URL: #32368
Refs: #32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins added a commit that referenced this issue Mar 24, 2020
@MylesBorins
deps: update npm to 6.14.3
eef4721 
PR-URL: #32368
Refs: #32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@mleneveut
Copy link
Author

Hi, thanks for your quick and efficient work on this.

Could we release a node 12.x with npm 6.14.4 which seems to fix deeper the issue ? npm/cli#1059

@mhdawson mhdawson mentioned this issue Mar 30, 2020
Closed
MylesBorins added a commit to MylesBorins/node that referenced this issue Apr 1, 2020
@MylesBorins
deps: update npm to 6.14.3
949a83c 
PR-URL: nodejs#32368
Refs: nodejs#32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 1, 2020
MylesBorins added a commit that referenced this issue Apr 1, 2020
@MylesBorins
deps: update npm to 6.14.3
c419cd2 
Backport-PR-URL: #32527
PR-URL: #32368
Refs: #32296
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@richardlau
Copy link
Member

Node.js 10.20.0, 12.16.2 and 13.12.0 were all updated to use npm 6.14.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

deps: update npm to 6.14.3 MylesBorins/node