Consider a new v2 release addressing the npm audit issues

[xzyfer]

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]

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

[xzyfer]

Apologies I misread

…

On Tue., 23 Apr. 2019, 11:35 pm Christophe Coevoet, < ***@***.***> wrote: node-gyp is using ***@***.***, not ***@***.*** though. So what is needed would be a new v2 release. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAENSWBZC2A4OES4E5NDAC3PR4GBBANCNFSM4HHX2TTQ> .

[ChALkeR]

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:

+ npm@6.9.0
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]

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]

@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]

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

[BRKurek]

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.