Fix vulnerability in tar < 4.2.2

[kemenaran]

npm audit reports a bad vulnerability in tar < 4.2.2. It has been reported since April 5th.

Etherpad-lite isn't using tar directly though. The dependency chain is npm > npm-lifecycle > node-gyp > tar.

So we need to wait for the chain to publish new versions with the dependencies fixed:

• Wait for a fixed version of node-gyp 3.x to be released ( v3.8.1 proposal (became v4.0.0) nodejs/node-gyp#1718)
• Wait for a fixed version of npm to be released
• Bump the npm dependency.

(This issue is just for tracking the progress of updates in the dependencies chain.)

[muxator]

node-gyp updated its tar dependency in v4.0.0 (see nodejs/node-gyp#1718 (comment)). The major version bump is due to tar dropping support for node < 4.0. This is not a problem for Etherpad (in next 1.8 version we will already require node >= 8.9.0).

We are standing by for now, waiting for a new npm release. The current latest version (6.9.0, we are at 6.4.1 now) still did not apply the vulnerability fix.

[muxator]

I could use some help here.

Vulnerabilities keep getting reported for transitive dependencies of npm due to tar, even when updating to the latest npm version (which by now I would hope had sorted its dependencies).

Test case:

1. Clean installation:

# DO NOT DO THIS ON A PRODUCTION INSTANCE!
cd <basedir>
rm -rf src/node_modules
rm -rf node_modules
rm -f package-lock.json
rm -f src/package-lock.json
rm -f src/.ep_initialized

update to latest devel:

git checkout 04a45fbe46a7

Edit package.json, bumping npm to 6.9.0 (latest as of today):

diff --git a/src/package.json b/src/package.json
--- a/src/package.json
+++ b/src/package.json
@@ -49,7 +49,7 @@
     "log4js": "0.6.35",
     "measured-core": "1.11.2",
     "nodeify": "^1.0.1",
-    "npm": "6.4.1",
+    "npm": "6.9.0",
     "object.values": "^1.0.4",
     "request": "2.88.0",
     "resolve": "1.1.7",

Install dependencies:

bin/installDeps.sh

Verify the output of npm audit (showing only the parts reletive to npm):

$ cd <basedir>/src
$ npm audit
                                                                                
                       === npm audit security report ===                        
                                                                                
[...]

┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > libcipm > npm-lifecycle > node-gyp > fstream           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > libcipm > npm-lifecycle > node-gyp > tar > fstream     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > libnpm > npm-lifecycle > node-gyp > fstream            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > libnpm > npm-lifecycle > node-gyp > tar > fstream      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > node-gyp > fstream                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > node-gyp > tar > fstream                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > npm-lifecycle > node-gyp > fstream                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ fstream                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.0.12                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > npm-lifecycle > node-gyp > tar > fstream               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/886                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ tar                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.2.2 <3.0.0 || >=4.4.2                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > libcipm > npm-lifecycle > node-gyp > tar               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/803                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ tar                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.2.2 <3.0.0 || >=4.4.2                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > libnpm > npm-lifecycle > node-gyp > tar                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/803                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ tar                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.2.2 <3.0.0 || >=4.4.2                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > node-gyp > tar                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/803                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Arbitrary File Overwrite                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ tar                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.2.2 <3.0.0 || >=4.4.2                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ npm                                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ npm > npm-lifecycle > node-gyp > tar                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/803                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

[gobengo]

Looks like there is this PR on npm that will bump node-gyp to >= 4.x (which deps on safer tar): npm/cli#198

Can etherpad-lite just force resolution of tar to a safe version until npm cli is more better? That's what this repo did: https://github.com/vaadin/magi-cli/pull/95/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R36

UPDATE: err.. package.json 'resolutions' is a yarn thing, but there is an npm lib you can use to use it. https://stackoverflow.com/questions/52416312/npm-equivalent-of-yarn-resolutions

npm hasn't had a PR merged for 3 months so might be worth something like that to remove the hard dependency on that team. https://github.com/npm/cli/tree/release-next

[muxator]

Wow. Thanks @gobengo, nice sum up.

I did a quick check (install only) using npm-force-resolutions, and npm audit actually stopped listing the vulnerabilities on tar.

But this is so ugly I am still doubtful:

• we cannot know if this is functionally correct

• npm-force-resolutions basically performs a tool-assisted surgery on package-lock.json, and Etherpad does not come with one. The reason is not laziness, but the "peculiar" directory structure, startup & installation mode of Etherpad, which makes things hairy

• going this way has potentially no end. There is already another high profile vulnerability remaining on npm:

  ┌───────────────┬──────────────────────────────────────────────────────────────┐
  │ High          │ Arbitrary File Overwrite                                     │
  ├───────────────┼──────────────────────────────────────────────────────────────┤
  │ Package       │ fstream                                                      │
  ├───────────────┼──────────────────────────────────────────────────────────────┤
  │ Patched in    │ >=1.0.12                                                     │
  ├───────────────┼──────────────────────────────────────────────────────────────┤
  │ Dependency of │ npm                                                          │
  ├───────────────┼──────────────────────────────────────────────────────────────┤
  │ Path          │ npm > libcipm > npm-lifecycle > node-gyp > fstream           │
  ├───────────────┼──────────────────────────────────────────────────────────────┤
  │ More info     │ https://nodesecurity.io/advisories/886                       │
  └───────────────┴──────────────────────────────────────────────────────────────┘
  

  This one was reported on 2019-05-19: https://www.npmjs.com/advisories/886.

  What to do here? We could continue forcing resolutions, but at that point we would basically end up doing npm's work, and I am not really sure this would be healty for the project :/

At least, thanks to your analysis, we now know that we are at the mercy of the npm team, and this is insane.

[gobengo]

I agree it's not ideal. Ideally everyone upstream would fix all the things right away. But by that logic I wouldn't be typing this. :)

we cannot know if this is functionally correct

True. But we also can't know it's functionally correct even if npm team fixes something. Just because the npm repo gets code in it, that doesn't mean it's functionally correct. The only way to know if something is (our opinion of) 'functionally correct' is to test the functionality. Is there a smoke test in etherpad-lite already we can use to verify our dep on npm? If not, what could that test consist of?

npm-force-resolutions basically performs a tool-assisted surgery on package-lock.json, and Etherpad does not come with one. The reason is not laziness, but the "peculiar" directory structure, startup & installation mode of Etherpad, which makes things hairy

I noticed this when trying to dig in. I actually couldn't npm audit until I'd generated a package-lock.json, which installDeps.sh didn't do. I had to cd src && npm install first. I noticed the directory structure was peculiar. What I don't understand is: What harm would come from adding a package-lock.json?

going this way has potentially no end. There is already another high profile vulnerability remaining on npm:

Sure. There can always be new vulnerabilities in dependencies. But that's not a unique critique to this particular way of resolving some of them. If we never run npm audit we can pretend they don't exist. :) But that's not responsible.

More constructively, it seems like you have an implicit policy that all npm audit-discovered vulnerabilities MUST be resolved before releasing a minor version. Why? I'm all for doing the best we can, but now it has led to an "endless" conflict of releasing incrementally vs only once perfection is reached. That's a hard one!

What I would do: Either

• use npm-force-resolutions to forcefully require only safe transitive deps. If desired, add tests in etherpad-lite to ensure this doesn't lead to functional regression
• Make explicit the policy on npm audit results, and consider including something like "We're going to do the best we can, but if our dependency (npm) hasn't done something after 3 months, we'll do minor version releases that don't fix the vulnerability"

This is indeed tricky. Thanks for doing what you can!

[muxator]

An updated version of npm (6.10.3) and npm-lifecycle fixes the vulnerabilities.
We can close this.