Dependency Audit Retrospective: June 2019
This week, a tweet from the Chief Security Officer of Brave browser raised some public concerns of the security of our codebase.
The tweet showed an automated summary of our npm dependencies, which had some big numbers, making it a concerning image for the security-minded to rally around.
Our Response
We have taken the opportunity to clean up these dependencies for the sake of improving the results of this type of automated audit.
We developed an audit tool called npm-audit-plz that would bypass the errors that we were receiving from a normal npm audit, which we determined was due to a bug in npm. We reported the issue to npm, and were told it had been recorded, but were not offered any estimate on timeframe for a fix.
Our team members used these audit results to begin isolating the dependencies that were racking up the audit flags the most.
@whymarrh, @danjm, and @dryajov all contributed to this analysis.
This resulted in a pull request that was able to once again get the project passing an npm audit, with very few detected npm audit vulnerabilities remaining, discussed below.
We have also created a new bug bounty for anyone who can find a malicious dependency in the MetaMask codebase.
How this audit went un-noticed
In the past, we had required this automated npm audit to complete for our test suite to pass, which is required for every pull request to be merged.
This enforced requirement was highly non-deterministic, and had gradually failed more and more often, until it was blocking an urgent bug fix. At that point, we disabled the requirement for npm audit in our test suite.
In retrospect, that would have been the time to open an issue and begin implementing an alternative automated audit process that would be more fit to our needs, but this need was not done.
Analysis of those numbers
A majority of the counted vulnerabilities (3079) were a single module being repeatedly counted by npm audit: lodash prior to 4.17.5, which was vulnerable to a prototype pollution attack, which is itself only vulnerable to other malicious dependencies. (Many of the counted vulnerabilities and dependencies were also repeatedly counting the same modules)
Many of these dependencies were part of our build system or local development environment, as the npm audit command also counts “developer dependencies”, which are never shipped to production, and also often not used in our build system. (For example, our testing tools)
There also appeared to be some kind of dependency-counting loop triggered where we incorrectly listed one dependency, gaba (a module we maintain ourselves) as a devDependency.
Analysis of remaining detected vulnerabilities
After we upgraded the modules we could to eliminate these warnings, The remaining detected vulnerable modules are:
=== npm audit security report ===
# Run npm update fstream --depth 7 to resolve 2 vulnerabilities
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High │ Arbitrary File Overwrite │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ fstream │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ ganache-core [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ ganache-core > web3 > web3-bzz > swarm-js > tar.gz > fstream │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/886 │
└───────────────┴──────────────────────────────────────────────────────────────┘┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High │ Arbitrary File Overwrite │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ fstream │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ ganache-core [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ ganache-core > web3 > web3-bzz > swarm-js > tar.gz > tar > │
│ │ fstream │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/886 │
└───────────────┴──────────────────────────────────────────────────────────────┘# Run npm update tar --depth 6 to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High │ Arbitrary File Overwrite │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ tar │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ ganache-core [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ ganache-core > web3 > web3-bzz > swarm-js > tar.gz > tar │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/803 │
└───────────────┴──────────────────────────────────────────────────────────────┘┌──────────────────────────────────────────────────────────────────────────────┐
│ Manual Review │
│ Some vulnerabilities require your attention to resolve │
│ │
│ Visit https://go.npm.me/audit-guide for additional guidance │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Insecure Credential Storage │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ No patch available │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ abi-decoder │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ abi-decoder > web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/877 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Insecure Credential Storage │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ No patch available │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ gaba │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ gaba > web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/877 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Insecure Credential Storage │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ No patch available │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ web3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/877 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Regular Expression Denial of Service │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=2.3.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ gulp-stylefmt [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ gulp-stylefmt > stylefmt > stylelint > micromatch > braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/786 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Regular Expression Denial of Service │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=2.3.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ gulp-stylefmt [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ gulp-stylefmt > stylefmt > stylelint-order > stylelint > │
│ │ micromatch > braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/786 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Regular Expression Denial of Service │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=2.3.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ gulp-watch [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ gulp-watch > anymatch > micromatch > braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/786 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Regular Expression Denial of Service │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=2.3.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ qunitjs [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ qunitjs > chokidar > anymatch > micromatch > braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/786 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Regular Expression Denial of Service │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=2.3.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ qunitjs [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ qunitjs > findup-sync > micromatch > braces │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/786 │
└───────────────┴──────────────────────────────────────────────────────────────┘
found 11 vulnerabilities (8 low, 3 high) in 255418 scanned packages
run `npm audit fix` to fix 3 of them.
8 vulnerabilities require manual review. See the full report for details.
Their context in MetaMask:
ganache-core: Used in our test suite (vulnerability can only affect a machine running our tests).fstream: Used insideweb3.js, is only vulnerable when used in a node.js environment, which MetaMask never is.tar: Used insideganache-core, is only vulnerable when decompressing a file, which MetaMask never does. Also only used in our test suite.web3: Insecure when used to store credentials, which we do not do.braces: Is prone to a Denial of Service when fed untrusted input, but is only used in our build system.
A couple other notable flagged modules that are in our devDependencies but do not compromise the core product are:
- Arbitrary File Overwrite (https://nodesecurity.io/advisories/803 and https://nodesecurity.io/advisories/886) for some of our development dependencies
- GABA has Code Injection (https://nodesecurity.io/advisories/813), DoS (https://nodesecurity.io/advisories/788), Prototype Pollution (https://nodesecurity.io/advisories/755) and Regular Expression Denial of Service (https://nodesecurity.io/advisories/786) all in dev deps
The Bigger Problem
Ultimately, supply chain attacks are an constant and fundamental threat to any project’s security, and we know that as a wallet, we are a target of constantly increasing value, so we can’t take security seriously enough.
We saw a JavaScript bitcoin wallet get hacked last year by a single vulnerable dependency, and it happened again just last week. We have been thinking about these kinds of attacks seriously, because we’re absolutely a prime target for the same thing.
There were many answers thrown around in the wake of the event-stream hack, and lots of people (even in the twitter thread that inspired this incident) are quick to simply blame JavaScript, and the node ecosystem for encouraging easy or excessive use of dependencies.
While it’s true that we need to be incredibly careful with our dependencies, and be responsible for as much of our code as possible, we believe there is a better answer to this long term, and it is better dependency isolation. If we are able to tightly regulate the access that any given dependency has to application permissions, we can limit its possible damage.
Fortunately, the work at Agoric has demonstrated that JavaScript is nearly an object-capability language, which means (in part) it is very close to enabling the isolation of dependencies from each other, except for the authority needed. The Realms API proposal would make this kind of security native in JavaScript, but in the meanwhile, the Secure EcmaScript (SES) polyfill makes it already attainable, and is in production at Salesforce today for their plugin system.
To explore this possibility for MetaMask, we have been experimenting with a build tool we call SESify, which extends our browserify build process to also isolate every dependency in its own SES Realm, with permissions tightly confined with a declarative access file, which we could automatically update in response to dependency permissions changes, triggering manual review.
Effectively, this would allow us to automatically never allow a module that doesn’t need network access to spontaneously access the network, and to be alerted the moment an update to that module introduced code that would attempt to access the network.
We believe that code isolation is the best tool we have to combat dependency attacks, as even npm audit only detects known vulnerabilities. (If you're interested in helping contribute to that project, please reach out!)
Even without perfect dependency isolation, there may also be an opportunity to use tooling like Trusted on First Use (TOFU) to automatically detect dependency upgrades that introduce the use of new system APIs, which could help to automatically detect rogue dependency upgrades.
Conclusion
MetaMask suffered a process failure where we disabled an automated audit and did not open an issue to track the re-enabling or replacing of it.
We have reviewed this protocol failure with the team, and have updated the concerning dependencies and re-enabled the automated audit in our test suite that is required before each change to the project. We’re also looking at other automated audit solutions, like github and greenkeeper.
Because this means we updated many dependencies in our development branch at once, we are also announcing a fresh 3000 DAI (~$3000) bug bounty program for finding any dependency upgrade that introduces code designed to illegitimately access user keys. To aid reviewers in their effectiveness in this bug hunt, we are also introducing a new twitter account where we will post about pending release candidates.
Automated audits are not the final say in security, which is why we hired Cure53 to review our extension’s security, and have continued to pursue strategies of dependency isolation.
This improvement in our project has been thanks in part to the open source community, and we thank everyone for encouraging us to be the most secure project we can be.
