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

Replace vulnerable mkdirp with fs.mkdir #4199

Closed
4 tasks done
MrShoenel opened this issue Mar 12, 2020 · 7 comments · Fixed by #4200
Closed
4 tasks done

Replace vulnerable mkdirp with fs.mkdir #4199

MrShoenel opened this issue Mar 12, 2020 · 7 comments · Fixed by #4200
Labels
area: node.js command-line-or-Node.js-specific area: security involving vulnerabilities good first issue new contributors should look here!

Comments

@MrShoenel
Copy link

  • Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
  • Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
  • 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
  • Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

Mocha does depend on mkdirp, which depends on an old version of minimist, which has a known Prototype pollution vulnerability: https://snyk.io/test/github/mochajs/mocha

It does not seem that minimist is getting updated soon, and also, fs.mkdir(path[, options], callback) can be run recursively, effectively obsoleting mkdirp. Furthermore, packages depending on any version of mocha currently exhibit this vulnerability (cf. any package on snyk.io), and are shown to the user as vulnerable, too.

  • The output of mocha --version and node node_modules/.bin/mocha --version: 7.1.0
  • The output of node --version: v13.6.0
  • Your operating system
    • name and version: Windows 10
    • architecture (32 or 64-bit): 64-bit
  • Your shell (e.g., bash, zsh, PowerShell, cmd): PS
  • Any code transpiler (e.g., TypeScript, CoffeeScript, Babel) being used (and its version): no
@boneskull boneskull added area: security involving vulnerabilities good first issue new contributors should look here! status: accepting prs Mocha can use your help with this one! area: node.js command-line-or-Node.js-specific and removed unconfirmed-bug labels Mar 12, 2020
@boneskull
Copy link
Member

please note that fs.mkdir with recursive: true does not completely obsolete mkdirp. that said, I think we can move away from it once Node.js v8 support is dropped from Mocha. the recursive flag arrived in Node.js v10.12.0.

@outsideris
Copy link
Member

We will drop Node.js v8 support in mocha v8.0.0, see #4164

jhecking added a commit to aerospike/aerospike-client-nodejs that referenced this issue Mar 16, 2020
minimist@1.2.2 and earlier had "a prototype pollution bug that could
cause privilege escalation in some circumstances when handling untrusted
user input." [Source: https://github.com/substack/minimist#security]

Unfortunately, mocha@7.x also has a dependency on a vulnerable minimist
version through the mkdirp package; but at this point it seems likely
that this will only get addressed in mocha@8.0:
mochajs/mocha#4199.

This update partially addresses the security alert raised by GitHub in
https://github.com/aerospike/aerospike-client-nodejs/network/alert/package-lock.json/minimist/open
@Hypnosphi
Copy link

It should be enough to add caret (^) to mkdirp dependency, because 0.5.3 has minimist updated:

% npm info mkdirp@0.5.3 dependencies 
{ minimist: '^1.2.5' }

@ejke
Copy link

ejke commented Mar 18, 2020

If someone else gets here because of minimist vulnerability, do

  • force update mocha (remove from package.json and $: npm install mocha with optional
    --save-dev) Note: $: npm update might work as well. Didn't for me.
  • delete node_modules and package-lock.json
  • $: npm install to get all fresh and updated dependencies. Check with $: npm audit that all is fixed.

@brettz9
Copy link

brettz9 commented Mar 18, 2020

@ejke: The key is to use the depth flag with npm update. Even just npm update --depth 10 is typically enough (the docs recommend 9999, but besides often hanging, it doesn't seem to be typically necessary to check to such a high depth).

@JimmyBjorklund
Copy link

Getting this too in the log when installing -> npm WARN deprecated mkdirp@0.5.3: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x.

@brettz9
Copy link

brettz9 commented Mar 18, 2020

@JimmyBjorklund : That's an improvement (and separate issue) though (the message actually means you got the vulnerability fixed or avoided). While 0.5.3 is deprecated in favor of the new 1.0 series, 0.5.3 includes a fix for a security vulnerability present in 0.5.1 (which some of us still had in our node_modules or package-lock.json).

But it would indeed be good to start using the 1.0 series of mkdirp too (or remove it) as it is presumably more attractive to use internally given its use of Promises, and because it is the version currently being maintained. But as a 1.0 version, it introduces breaking changes, so there'd need to be some code refactoring to get it working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific area: security involving vulnerabilities good first issue new contributors should look here!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants