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

NPM Audit issue - Arbitrary File Overwrite #1717

Closed
ma-jahn opened this issue Apr 12, 2019 · 38 comments
Closed

NPM Audit issue - Arbitrary File Overwrite #1717

ma-jahn opened this issue Apr 12, 2019 · 38 comments

Comments

@ma-jahn
Copy link

ma-jahn commented Apr 12, 2019

NPM Audit issue:
Version 3.8.0 of node-gyp relies on tar < 4.4.2 which comes with a high Vulnerability (https://www.npmjs.com/advisories/803)

I can see that you have updated this dependency within your master branch, but version 4.0.0 is not yet released.

Can you let me know if and when this fix will be available?

Thanks

@antoniopaisfernandes
Copy link

#1713

@cberg-zalando
Copy link

Following the comment stream in #1713 does not clearly say whether the fix will be backported into the v3.x branch. @richardlau just mentioned that the update of tar from version 2.* to 3.* was considered a breaking change at that time so it was only applied to master.

@romanstingler
Copy link

It's a shame that this is known since april 4th and there is still no release.
Furthermore, it should be fixed a year ago when the issue was found in node-tar April 30th 2018.

You guys with your infinite number of dependencies should try a rolling release, as soon as one dependency is updatable you should upgrade and don't stick with node v4 which hasn't been updated since march 2018 so security updates or whatsoever.

In my opinion merge #1670 and #1718 and release this piece of ..
and don't waste time in discussions if there should be a major or minor version bump, as it breaks compatibility with node 4 there must be a major release bump.

see semver.org

@flyke
Copy link

flyke commented Apr 16, 2019

This is how I fixed this in my project, please let me know if there is a better way:
npm install -D node-gyp
npm install -D tar@">4.4.7"

Edit package-lock.json and replace:
"tar": { "version": "2.2.1", "resolved": "https://registry.npmjs.org/tar/-/tar-2.2.1.tgz", "integrity": "sha1-jk0qJWwOIYXGsYrWlK7JaLg8sdE=", "dev": true, "requires": { "block-stream": "*", "fstream": "^1.0.2", "inherits": "2" } }

with:

"tar": { "version": "4.4.8", "resolved": "https://registry.npmjs.org/tar/-/tar-4.4.8.tgz", "integrity": "sha512-LzHF64s5chPQQS0IYBn9IN5h3i98c12bo4NCO7e0sGM2llXQ3p2FGC5sdENN4cTW48O915Sh+x+EXx7XW96xYQ==", "dev": true, "requires": { "chownr": "^1.1.1", "fs-minipass": "^1.2.5", "minipass": "^2.3.4", "minizlib": "^1.1.1", "mkdirp": "^0.5.0", "safe-buffer": "^5.1.2", "yallist": "^3.0.2" } }

@ma-jahn
Copy link
Author

ma-jahn commented Apr 16, 2019

@flyke Have a look at: #1713
Updating tar comes with a breaking change, therefor manually overwriting the tar version in your package-lock.json will most likely break node-gyp and should therefore imho not be recommended solution.

@flyke
Copy link

flyke commented Apr 16, 2019

@ma-jahn I am very open to any suggestion that fixes major security flaws like this. So if you have any 'good way' to fix this, please share, I would be very grateful.

@ma-jahn
Copy link
Author

ma-jahn commented Apr 16, 2019

A fix is currently in progress: 1456ef2

@I-keep-trying
Copy link

I-keep-trying commented Apr 17, 2019

A fix is currently in progress: 1456ef2

Is there any reason a person couldn't, in the meantime, when installing node-sass, change package.json to include node-gyp 4.0.0, which already includes the updated tar version?

Edit: To clarify, I'm not suggesting editing the package-lock directly. Just do

npm install -D node-sass

Then, edit the node-sass package.json to correct the node-gyp version, and then just do

npm install

?

@JoshRobertson
Copy link

1456ef2

That fix is for 4.0.0 which hasn't been released to NPM yet.

This is the PR to release a 3.x update :
#1718

This is the ticket regarding 4.0.0 not published to npm:
#1721

@I-keep-trying
Copy link

So, nobody knows what we are supposed to do in the meantime? Just don't use node-sass? I'm not trying to be hyperbolic, I honestly don't know how serious this is.

@sidhujaspreet
Copy link

Just switched to node-sass for current project and ran into the same issue. Seems like it's been there since few days now, please pick this one up on priority.
Would be good to have rolling release as suggested by @romanstingler .

@jhnferraris

This comment has been minimized.

@corbin88
Copy link

That fix is for 4.0.0 which hasn't been released to NPM yet.

Anyone know when this will be?

@kaiyoma
Copy link

kaiyoma commented Apr 22, 2019

Looks like a fix was merged 10 days ago, but there's still no release. What's the holdup?

@uniibu
Copy link

uniibu commented Apr 22, 2019

Temporary Solution for users using Yarn: (use at your own risk)

This is just a temporary solution for users using Yarn until the fix gets published to npmjs.

  • Verify that you are affected by the vulnerability yarn audit
  • If you are, edit package.json and add (do not install tar yet)
  "resolutions": {
    "tar": "^4.4.8"
  }
  • After adding resolutions to your package.json, Install the latest tar package yarn add tar@^4.4.8
  • Test it yarn audit should no longer show that node-gyp -> tar vulnerability.

Once the fix has been published, simply remove both the resolutions and the package yarn remove tar in the same order to update the yarn.lock

For npm users, it require manually editing you package-lock.json and replacing the tar dependency with the latest. I don't recommend doing this if you don't know what you are doing. There are packages that could help you with this such as https://www.npmjs.com/package/npm-force-resolutions. Again use at your own risk.

@shazilrehman
Copy link

npm install npm@latest -g

that worked for me

@jhnferraris
Copy link

@shazilrehman What should be the correct npm version to be use? I am at 6.9.0 and still experiencing the problem. :D

@subhashkonda
Copy link

Do we have the fix ready for this, "npm audit fix" or explicit "npm install tar" is not helping in this case.

@juliusl
Copy link

juliusl commented Apr 22, 2019

@uniibu To clarify the risk, this will break older versions of node. Besides that this is basically what the current fix we're waiting on does.

I don't think you need the yarn add tar@^4.4.8 though. Yarn seems to understand the resolutions on it's own and seems to update the yarn.lock appropriately.

@shazilrehman
Copy link

@jhnferraris may god be with you then.

@benjamingr
Copy link
Member

A kind reminder that Node.js has interaction standards and maintainer abuse will not be tolerated. Comments that are not constructive will be deleted (criticism of technical issues and discussion is welcome).

If you are unsure how to interact we have a code of conduct here. If you have any questions about our policy or would like to report anything feel free to reach out at [email protected] or [email protected]

englishextra added a commit to englishextra/englishextra-app that referenced this issue May 7, 2019
englishextra added a commit to yuzhtushino/yuzhtushino.github.io that referenced this issue May 7, 2019
englishextra added a commit to englishextra/serguei-uwp-dev that referenced this issue May 8, 2019
englishextra added a commit to yuzhtushino/yuzhtushino.github.io that referenced this issue May 8, 2019
englishextra added a commit to englishextra/englishextra-app that referenced this issue May 8, 2019
englishextra added a commit to englishextra/serguei-muicss-dev that referenced this issue May 8, 2019
englishextra added a commit to englishextra/englishextra-app that referenced this issue May 8, 2019
englishextra added a commit to noushevr/noushevr.github.io that referenced this issue May 8, 2019
englishextra added a commit to englishextra/mytushino-muicss-dev that referenced this issue May 10, 2019
englishextra added a commit to mytushino/mytushino.github.io that referenced this issue May 10, 2019
englishextra added a commit to englishextra/lovespy-muicss-dev that referenced this issue May 10, 2019
englishextra added a commit to lovespy/lovespy.github.io that referenced this issue May 10, 2019
englishextra added a commit to englishextra/16pixels that referenced this issue May 10, 2019
englishextra added a commit to englishextra/cdn that referenced this issue May 10, 2019
englishextra added a commit to englishextra/iframe-lightbox that referenced this issue May 10, 2019
englishextra added a commit to englishextra/img-lightbox that referenced this issue May 10, 2019
englishextra added a commit to englishextra/qrjs2 that referenced this issue May 10, 2019
englishextra added a commit to englishextra/typeboost.css that referenced this issue May 10, 2019
englishextra added a commit to englishextra/shimansky.biz that referenced this issue May 11, 2019
englishextra added a commit to englishextra/englishextra.github.io that referenced this issue May 11, 2019
@jcorkhill
Copy link

@benjamingr @ChALkeR @ma-jahn

Updating node-gyp does not fix the issue for me either. I've been having this problem ever since updating the node-gyp configure.js file as follows due to an issue with MSBuild.exe after updating to Visual Studio 2019 (lines 152-162):

if (vsSetup) {
      // GYP doesn't (yet) have support for VS2017, so we force it to VS2015
      // to avoid pulling a floating patch that has not landed upstream.
      // Ref: https://chromium-review.googlesource.com/#/c/433540/
      gyp.opts.msvs_version = '2015'
      process.env['GYP_MSVS_VERSION'] = 2015
      process.env['GYP_MSVS_OVERRIDE_PATH'] = vsSetup.path
      defaults['msbuild_toolset'] = 'v142'
      defaults['msvs_windows_target_platform_version'] = vsSetup.sdk
      variables['msbuild_path'] = path.join(vsSetup.path, 'MSBuild', 'Current', 'Bin', 'MSBuild.exe')
    }

This may just be coincidental because I would think those changes would be irrelevant to tar.

Here is my NPM Audit Report:

https://gist.github.com/JamieCorkhill/90f92c4f50d7e59e42c03a90d11ad93c

Any help would be much appreciated.

Thanks.

@subhashkonda
Copy link

Any update on this fix, I saw some where the ETA was 10th May, so thought check again on new ETA ???

@ChALkeR
Copy link
Member

ChALkeR commented May 13, 2019

@JamieCorkhill

As your report shows, npm in your setup (actually, in their latest published version) chain-depends on old node-gyp version, which depends on old branch of tar. For this to be fixed, either of those two should happen:

That is not a problem of node-gyp and isn't something that is trivially fixable on node-gyp side.

@subhashkonda No, there is nothing else to be done here. node-gyp release containing the fixed version of tar has already been released about 20 days ago.

node-gyp is just one item in the dependency chain that needs to be updated by everyone unless tar ships a fix in their 2.x branch, and node-gyp was already bumped. So, either your other deps that chain-depend on node-gyp have to be updated in order for you to receive the fix, or tar should ship a patch in their 2.x branch.

@jcorkhill
Copy link

@ChALkeR

Thank you. So, to confirm, npm itself in my dependency chain relies on an old version of node-gyp which relies on an old version of tar? I updated Node and npm yesterday to their newest versions, and have had the issue since then.

@ChALkeR
Copy link
Member

ChALkeR commented May 13, 2019

@JamieCorkhill Yes, you can confirm that it does not happen with npm install node-gyp in a clean project directory with no other deps but happens with npm install npm in a clean project directory.

@jcorkhill
Copy link

jcorkhill commented May 13, 2019

@ChALkeR Thanks. So are npm aware of this issue do you know of, and have they taken any steps to mitigate it other than the issue you linked above, which doesn't appear to have gone anywhere for the last 10 days?

@ChALkeR
Copy link
Member

ChALkeR commented May 14, 2019

@JamieCorkhill There are four separate dep chains in npm leading to the outdated tar dependency in the audit you linked above. It appears that those are being worked on, e.g. npm-lifecycle had a release recently (5 days ago) that bumped its deps: https://github.com/npm/npm-lifecycle/commits/latest, and now at least npm-lifecycle inside the deps chain does not cause audit warnings anymore.

It is understandable why unrolling those deps chains is not fast, if they want to be on the safe side and test everything.

That said, I personally would have appreciated slightly more feedback in the tar@2 backport issue/pr :-).

@ChALkeR
Copy link
Member

ChALkeR commented May 15, 2019

I think this is finally fixed with everyone with [email protected] release, except for packages where exact nested dependency versions are locked.

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