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

Consider a new v2 release addressing the npm audit issues #212

Closed
xzyfer opened this issue Apr 23, 2019 · 24 comments
Closed

Consider a new v2 release addressing the npm audit issues #212

xzyfer opened this issue Apr 23, 2019 · 24 comments

Comments

@xzyfer
Copy link

xzyfer commented Apr 23, 2019

I can't do the issue justice. See nodejs/node-gyp#1718

tl:dr; npm audit is unhappy with node-tar@^2 because of https://hackerone.com/reports/344595. Since node-tar@^2 is used in node-gyp the npm audit alert is rippling throughout the Node community. Bumping to node-tar@^4 breaks Node 0.10 & 0.12 support which starting up the typical semver debates, and may force a lot of projects to subsequently bump their majors, and so on an do fourth.

Edit: node-tar@^2 not @^3

@stof
Copy link

stof commented Apr 23, 2019

node-gyp is using node-tar@2, not node-tar@3 though. So what is needed would be a new v2 release.

@xzyfer
Copy link
Author

xzyfer commented Apr 23, 2019 via email

@xzyfer xzyfer changed the title Consider a new v3 release addressing the npm audit issues Consider a new v2 release addressing the npm audit issues Apr 23, 2019
@ChALkeR
Copy link
Contributor

ChALkeR commented Apr 23, 2019

Query: "tar@2
11791102        node-gyp
11435204        node-sass
5687179 npm
3411992 tar-pack
1222492 gulp-sass
1200697 npm-lifecycle
1193345 lerna
1005624 cordova
701893  cordova-lib
585175  renovate

There are a lot of libs chain-depending on tar@2, and they get reports from npm audit now.
Even npm itself does, 4 times in different chains:

+ [email protected]
added 426 packages from 800 contributors and audited 12096 packages in 5.679s
found 4 high severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details

That's essentially a false positive, because anyone in position to exploit node-tar in that dependency chain is most likely also in position to just ship malicious code.
But that's a signal that is deminishing value from future valid npm audit reports, which is not a very good thing to do. /cc @isaacs

node-gyp can't update node-tar to v4.x without doing a semver-major version bump because of node-tar changing the supported platforms version.

So, unless a fixed node-tar version is released in 2.x branch, downstream (including npm, npm-lifecycle etc) would have to do a major version bum for node-gyp.

Another possible way forward would be to migrate node-gyp@3 off node-tar usage completely, but that is not the ideal solution.

@isaacs
Copy link
Owner

isaacs commented Apr 23, 2019

I would gladly accept a PR against the v2 branch to address this. But I'm not really interested in doing work to support node versions long past their LTS expirations. The library has been completely rewritten since then, so it's not a simple backport. If anyone wants to take this on, I'll be happy to answer any questions and publish it once it lands, but I haven't even looked at that version of the codebase for 2 years now.

@ChALkeR
Copy link
Contributor

ChALkeR commented Apr 23, 2019

@isaacs Thanks for the fast reply! (Also, for the willingness to accept a patch and cut a release).

But I'm not really interested in doing work to support node versions long past their LTS expirations.

I believe that that's not the point, the point is that npm audit is going to emit false positive high severity warnings on everything, including npm itself, until either of these happens (example is for npm itself):

  • a fixed v2.x version is released,
  • a semver-major version of node-gyp is released (done), and new versions of npm-lifecycle and npm (and pretty much everything else for packages other than npm) are released to address that semver-major version bump.

@ChALkeR
Copy link
Contributor

ChALkeR commented Apr 24, 2019

See e.g. npm/npm-lifecycle#28

@BRKurek
Copy link

BRKurek commented Apr 24, 2019

I've been digging into the v2 code this morning, and I'm not sure v2 is actually vulnerable to this attack. In v2, when an extract encounters a link, it runs this code (lines 46-49 in lib/extract.js):

if (entry.type === "Link") {
  entry.linkpath = entry.props.linkpath =
    path.join(opts.path, path.join("/", entry.props.linkpath))
}

And a comment above this code explains the reasoning behind this:

// Hardlinks in tarballs are relative to the root
// of the tarball.  So, they need to be resolved against
// the target directory in order to be created properly.

However, I believe this results in a buggy implementation of tar extraction. Here's an example:

I have a sample tar, archive.tar, and running tar tvf archive.tar yields the following output:

drwxr-xr-x  0 briankurek staff       0 Apr 24 12:28 ./
-rw-r--r--  0 briankurek staff       4 Apr 24 12:28 ./two.txt
hrw-r--r--  0 briankurek staff       0 Apr 24 12:28 ./one.txt link to ./two.txt

So the tar simply consists of two files: two.txt and one.txt where one.txt is a hard link to two.txt.

Using the v2 node-tar branch locally, I ran the following script:

var tar = require('./tar');
var fs = require('fs');

var extract = tar.Extract('extracted');

fs.createReadStream('archive.tar').pipe(extract);

And got the following error:

Error: ENOENT: no such file or directory, link '/some-path/node-tar/extracted/extracted/two.txt' -> '/some-path/node-tar/extracted/one.txt'

I believe the root cause of this issue is that the linkpath contained in the entry for one.txt is changed from ./two.txt to extracted/two.txt by the previously mentioned if statement (and this can be confirmed with a few logs). Removing the above if statement, this extraction works as expected.

So, from what I can tell, tar extraction with hard links in v2 is buggy, but it isn't actually vulnerable to this attack.

Side note: if you comment out the if statement included above, the attack does work.

EDIT: I did some digging through the v2.x.x tags, and all of them have the above if statement in lib/extract.js. So, if this piece of code truly does keep the attack from working (even though it does create other bugs), no 2.x.x versions would be vulnerable.

@ChALkeR
Copy link
Contributor

ChALkeR commented Apr 24, 2019

/cc @vdeturckheim, perhaps, re: 2.x being affected or not

Upd: It looks like 2.x still overwrites files inside the dest dir. That's not what patched 4.x does.
Also, I am not completely sure if that's not exploitable to overwrite files outside of the dest dir, I didn't take a close enough look yet.

Thoughts?

@BRKurek
Copy link

BRKurek commented Apr 24, 2019

After more experimenting I can also confirm that 2.x.x can still overwrite files inside the destination directory. However, I don't think it's possible to overwrite files outside of the destination directory because of the inner path.join on line 48:

path.join("/", entry.props.linkpath)

The result of the above path.join will always be something like the following:

/some/path/here

And this path can't have any ..s in it because the path.normalize run internally by path.join will resolve them. So, when the outside path.join runs on line 48, the arguments will be something like this:

path.join('target', '/some/path/here')

And this will always result in a path inside the target directory because the second argument can't have any ..s in it.

@BRKurek
Copy link

BRKurek commented Apr 27, 2019

Opened #213 with a potential solution to this vulnerability.

@e-yoshi
Copy link

e-yoshi commented May 1, 2019

@isaacs, @xzyfer and other contributors, would you be able to review @BRKurek 's PR?
Thanks!

@AzRu
Copy link

AzRu commented May 3, 2019

Can we get this looked at sooner than later please?

@isaacs
Copy link
Owner

isaacs commented May 15, 2019

Ok, there's a v2.2.2 which no longer has this vulnerability, and includes a test to verify.

Fixed on this commit in fstream: npm/fstream@6a77d2f which is published on [email protected]

Let's get this tested in node-gyp, update the advisory, and put this to bed.

@isaacs
Copy link
Owner

isaacs commented May 15, 2019

npm advisory will be updated tomorrow morning, and then npm audit fix will Just Work for anyone hitting this due to node-gyp, since it's a patch version. This is a patch in a legacy version that's been broken for a pretty long time, so it seems hard to make the case that it should be an urgent emergency.

(I swear to god if someone comes out of the woodwork demanding this be backported to tar@v1, I'm gonna have to be rushed to the hospital, this code is so old and so gnarly. The tests don't even run properly on any version of node newer than v4.)

@xzyfer
Copy link
Author

xzyfer commented May 15, 2019

Thank you

pablothedude added a commit to OpenConext/OpenConext-engineblock that referenced this issue May 15, 2019
Revert the tar resolution because the vulnerability is
patched in node-tar.

isaacs/node-tar#212
@vinayakkulkarni
Copy link

Fixes sass/node-sass#2625

@isaacs
Copy link
Owner

isaacs commented May 15, 2019

The advisory has been updated as of this morning. npm audit fix should address this without any issue now, as long as you're on either the 2.x or 4.x tar versions.

@TorstenStueber
Copy link

Will this also be updated automatically? https://nvd.nist.gov/vuln/detail/CVE-2018-20834
Seems that this is what github bases its vulnerability reports on.

@trupti11
Copy link

Hi @isaacs , I just tried this, and it still doesn't fix my problems doing npm audit fix. I had tar version 4.4.8, but i also tried downgrading it to 4.4.2; Please can you help?
──────────────┬──────────────────────────────────────────────────────────────┐
│ 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

@trupti11
Copy link

Got it resolved! sorry guys... i had a node-gyp dependency for npm-lifecycle, once I resolved, the manual review on fstream dependencies could be fixed quickly! It works now! :) 👍

@vinayakkulkarni
Copy link

vinayakkulkarni commented May 16, 2019 via email

@erikeckhardt
Copy link

I contacted NIST directly about 2.2.2 not having the vulnerability, and got the following response:

After review of the CVEs, the information provided, and the configurations we have made the appropriate modifications. Please allow up to 24 hours for these changes to populate on the website and in the data feeds.

So stay tuned, because soon 2.2.2 will be recognized as an acceptable, non-vulnerable version, and appropriate versions of dependent packages like node-gyp will no longer be flagged as vulnerable.

dmethvin-gov added a commit to dmethvin-gov/us-forms-system that referenced this issue May 24, 2019
@jwalton
Copy link

jwalton commented May 28, 2019

There's a new version, but it seems to be uninstallable.

If you:

mkdir t
cd t
npm init
npm install semantic-release

You'll get errors about tar and fstream being out of date. npm audit fix will say "12 out of 12 problems fixed", but then npm install will tell you there are still 12 problems. The dependency that keeps installing [email protected] is [email protected], and it wants:

"tar": "^2.0.0",

So it should be getting 2.2.2. The complete dependency tree for tar is:

└─┬ [email protected]
  └─┬ @semantic-release/[email protected]
    └─┬ [email protected]
      ├─┬ [email protected]
      │ └── [email protected]
      ├─┬ [email protected]
      │ └── [email protected]  deduped
      └── [email protected]

So, with great irony, npm appears to indirectly be part of the problem why I'm getting an old version of tar. -_- The only way to get 2.2.2 seems to be to manually edit my package-lock.json file.

@dominykas
Copy link

dominykas commented May 28, 2019

There's a new version, but it seems to be uninstallable.

It is installable, except npm is using bundledDependencies, so in effect it is the same as having it locked. Hopefully there's a new release of npm soon.

bullettrang added a commit to bullettrang/v9-bears-team-14 that referenced this issue Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests