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 reports the package with high vulnerability (Denial of Service) #1446

Closed
yarpeytz opened this issue May 14, 2020 · 51 comments
Closed

Comments

@yarpeytz
Copy link

More info https://www.npmjs.com/advisories/1486

@ecbrodie
Copy link

And there goes CI pipelines across the globe...

@krautface
Copy link

krautface commented May 14, 2020

Seems like a key bit here that limits the scope:

This is only possible when the proxy server sets headers in the proxy request using the proxyReq.setHeader function.

Wouldn't this be a short term work around - proxyReq.headers['arr'] = "I'm a pirate" ?

@tonix-tuft
Copy link

I get the security warning from NPM about this package too. Using react-scripts (https://github.com/facebook/create-react-app)

@neilbryson
Copy link

Yeah, projects using webpack-dev-server will have this warning.

@jay-finn
Copy link

Is there a fix for this in the works?

@andrewsteadcc
Copy link

andrewsteadcc commented May 15, 2020

Our pipeline returns this audit failure
High
Denial of Service
Package http-proxy
Patched in No patch available
Dependency of react-scripts
Path react-scripts > webpack-dev-server > http-proxy-middleware > http-proxy
More info https://nodesecurity.io/advisories/1486

@pamit
Copy link

pamit commented May 15, 2020

npm audit warns in projects with webpack-dev-server
webpack-dev-server > http-proxy-middleware > http-proxy

@ady642
Copy link

ady642 commented May 15, 2020

sam withyarn audit

@maxmckenzie
Copy link

maxmckenzie commented May 15, 2020

So you can provisionally fix this by moving webpack-dev-server or react-scripts (whichever package is causing the issue) to devDependencies.

Once the package is in your devDependencies you can run npm audit --prod and it will not audit the package as it only audits the node packages in your dependencies with the --prod arg passed to the CLI.

@schemburkar
Copy link

npm audit warns in projects with webpack-dev-server
webpack-dev-server > http-proxy-middleware > http-proxy

Same for react-scripts as it depends on webpack-dev-server
react-scripts > webpack-dev-server > http-proxy-middleware > http-proxy

@panuhorsmalahti
Copy link

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

@nacholibre
Copy link

nacholibre commented May 15, 2020

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

Yes, if you use https://github.com/IBM/audit-ci, you can whitelist http-proxy like that:

$ npx audit-ci --high --whitelist=http-proxy

Caution

It's not recommended to use packages with vulnerabilities. In my case I've had to whitelist this in order for Travis CI to continue working.

@MacKoslowski
Copy link

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

Yes, if you use https://github.com/IBM/audit-ci, you can whitelist http-proxy like that:

$ npx audit-ci --high --whitelist=http-proxy

Sorry for noob question, but does this mean it isn't risky to use locally?

@Jackques
Copy link

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

Yes, if you use https://github.com/IBM/audit-ci, you can whitelist http-proxy like that:

$ npx audit-ci --high --whitelist=http-proxy

Sorry for noob question, but does this mean it isn't risky to use locally?

In theory it still is but by whitelisting you're intentionally ignoring the vulnerability. However i think this particulair vulnerability is more of a threat when published on a server.

@jamesAppello
Copy link

@Jackques, I tried running the whitelist command and it had no luck for me. Would it work if you were to CRA and then run --> npm uninstall http-proxy ?

@Jackques
Copy link

@Jackques, I tried running the whitelist command and it had no luck for me. Would it work if you were to CRA and then run --> npm uninstall http-proxy ?

If you use https://github.com/IBM/audit-ci then you should be able to set the error id (which relates to the security vul. issue id) in a .json. Check the audit-cli package for details. No need to reinstall the package in order to whitelist it. Once you have got the security id in your witelist the package should no longer appear in the security report.

@jamesAppello
Copy link

@Jackques, Thank you! What I meant to say was that what if you dont whitelist it?
In my specfic situation:
I am working in a group repo with branches and I am tying to install the dependencies for the client from create-react-app (where as I go to the master branch, git pull'd --> ran npm i | npm install). At that point i was saying what if you uninstall http-proxy at ^that point(after installing dependencies ad seeing the warning) instead of whitelisting? If that's a clearer way to ask the question?

Else:
Again I thank you for your help!

@jamesAppello
Copy link

I'm actually more concerned as to what specifically is wrong within the http-proxy package that is causing the error in the first place.

@dezfowler
Copy link

Disappointed that people are recommending whitelisting http-proxy or worse still completely disabling audit of devDependencies in response to this. I would guess that 90% of the people that read this thread and take that advice probably don't understand the full implications of it and will not reinstate the audit after http-proxy is fixed.

It's not just vulnerabilities that audit guards against but intentional exploits also. Just think about what things the code in those devDependencies has access to when it runs on your local machine or build agent:

  • Probably some private keys, database passwords, etc.
  • If you're in the cloud your build agent may well have privileged access to your cloud resources so it can deploy/modify things.
  • That code may not be directly included in your production JS files but it's able to access and manipulate those files so it could rewrite them any way it wanted.
  • It could duplicate itself to any npm packages you publish from your build agent

In summary: please don't do that.

@skottklebe
Copy link

According to the advisory, this was reported back in February.
Count me disappointed as well.

@jonny-harte
Copy link

What's the eta on this getting fixed?

@jamesAppello
Copy link

Maybe this happened due to a potential protoypePollutionAttack; possibly?

@jamesAppello
Copy link

https://blog.0daylabs.com/2019/02/15/prototype-pollution-javascript/
I noticed that besides the DoS vulnerability(http-proxy<< "High"), there is a "Low" vulnerability via yargs-parser package for ProtoType Pollution....so maybe this could have resulted from a prototype pollution.

@jsmylnycky
Copy link
Contributor

Simplest fix is to just not send the proxyReq event when the expect header is present. See #1447 for a hotfix, and feel free to leave comments if you do not agree and have a better suggestion. :)

@pamit
Copy link

pamit commented May 16, 2020

Disappointed that people are recommending whitelisting http-proxy or worse still completely disabling audit of devDependencies in response to this. I would guess that 90% of the people that read this thread and take that advice probably don't understand the full implications of it and will not reinstate the audit after http-proxy is fixed.

It's not just vulnerabilities that audit guards against but intentional exploits also. Just think about what things the code in those devDependencies has access to when it runs on your local machine or build agent:

* Probably some private keys, database passwords, etc.

* If you're in the cloud your build agent may well have privileged access to your cloud resources so it can deploy/modify things.

* That code may not be directly included in your production JS files but it's able to access and manipulate those files so it could rewrite them any way it wanted.

* It could duplicate itself to any npm packages you publish from your build agent

In summary: please don't do that.

I'm definitely sure 99.99% of the people fully understand the risks and issues with those SUGGESTIONS. I wouldn't do it if I was going to release a production version which would access my cloud resources, keys, etc! I suggest focusing on fixing the issue rather than expressing useless opinions. We are not 2-year-old developers!

@zackchu
Copy link

zackchu commented May 16, 2020

Using http-proxy with autoRewrite: true , and got breakdown. :(

@jimmyandrade
Copy link

I get the security warning from NPM about this package too. Using gatsby and netlify-cli:

gatsby > webpack-dev-server > http-proxy-middleware > http-proxy
netlify-cli > http-proxy
netlify-cli > http-proxy-middleware > http-proxy

@AviVahl
Copy link

AviVahl commented May 16, 2020

will be resolved once/if #1447 is merged and released.

@Jackques
Copy link

@Jackques, Thank you! What I meant to say was that what if you dont whitelist it?
In my specfic situation:
I am working in a group repo with branches and I am tying to install the dependencies for the client from create-react-app (where as I go to the master branch, git pull'd --> ran npm i | npm install). At that point i was saying what if you uninstall http-proxy at ^that point(after installing dependencies ad seeing the warning) instead of whitelisting? If that's a clearer way to ask the question?

Else:
Again I thank you for your help!

Well if you were to have http-proxy as a direct dependancy (which i doubt since it's also a dependancy of eventually angular, react etc.) then it would resolve the error. But like i said in between the paranthesis, you probably have an app built on on of those previously mentioned frameworks or at the very least something which relies on the http-proxy dependancy, thus you cannot resolve this issue by uninstalling it unless you install the very package which ultimately has http-proxy as a dependancy.

@medhax
Copy link

medhax commented May 16, 2020

If #1447 is merged and released this exploit after audit will be resolved? It's a good fact to run npm audit fix on all our React apps live on the web, isn't it?

@pl-jay
Copy link

pl-jay commented May 17, 2020

when i go here for more info, it says,
"No fix is currently available. Consider using an alternative package until a fix is made available."
So is there any alternative package available ?

@lauriejones
Copy link

The npm advisory lists the newly published 1.18.1 as still being affected: https://www.npmjs.com/advisories/1486/versions

Not sure if that is normal and it still needs to be reviewed by someone at npm, or #1447 unfortunately didn't do the trick.

@G-Rath
Copy link

G-Rath commented May 18, 2020

That is normal: they work on a whitelist :)

I've sent an email to security@npm with links to this issue & the PR, commenting that 1.18.1 patches the advisory & should be whitelisted.

@lauriejones
Copy link

Wonderful! Thank you @G-Rath!

@Jackques
Copy link

Jackques commented May 18, 2020

when i go here for more info, it says,
"No fix is currently available. Consider using an alternative package until a fix is made available."
So is there any alternative package available ?

Well yes, but since most of us use packages (e.g. http-proxy-middleware) which in turn use http-proxy as a dependancy those packages will need to use the alternatives themselves first.
And yes i am aware of a dirty hack we could exploit by changing the hash in the package-lock file but the packages which are dependant on http-proxy also need to update the api's when switching to another package other than http-proxy.

@andrewsteadcc
Copy link

andrewsteadcc commented May 18, 2020

Thanks @G-Rath can you post on here when it has been whitelisted?
Preparing for the update in my package-lock.json file

"http-proxy": {
      "version": "1.18.1",
      "resolved": "https://registry.npmjs.org/http-proxy/-/http-proxy-1.18.1.tgz",
      "integrity": "sha512-7mz/721AbnJwIVbnaSv1Cz3Am0ZLT/UBwkC92VlxhXv/k/BBQfM2fXElQNC27BVGr0uwUpplYPQM9LnaBMR5NQ==",
      "requires": {}
    }

Got the info from https://registry.npmjs.org/http-proxy

@dezfowler
Copy link

dezfowler commented May 18, 2020

The advisory has been updated: https://www.npmjs.com/advisories/1486

Remediation
Upgrade to version 1.18.1 or later

@jsmylnycky
Copy link
Contributor

Since released and advisory is updated, I'm closing this.

@dosstx
Copy link

dosstx commented May 18, 2020

Anyone mind telling me how to make the update with npm? DO i just change the version number in the package-lock.json file or ....?

@AviVahl
Copy link

AviVahl commented May 18, 2020

@dosstx

  • npm audit fix should fix it for you (now that the audit is resolved with a patch version).
    or
  • If you are able to (no other breakages), deleting and regenerating the lock file from scratch (rm package-lock.json; npm i) should also pick up the new version.
    or
  • manually deleting the current resolution for http-proxy (in the pacakge-lock.json file) and re-running npm i.

@zwbetz-gh
Copy link

zwbetz-gh commented May 18, 2020

Edit: see @Hypnosphi's comment for how to narrow down the focus #1446 (comment)

Add this to your package.json, so that packages that depend on http-proxy will resolve it to the patched version.

"resolutions": {
  "http-proxy": "^1.18.1"
}

Yarn supports the resolutions syntax out-of-the-box.

If using NPM, see https://www.npmjs.com/package/npm-force-resolutions

@AviVahl
Copy link

AviVahl commented May 18, 2020

Forcing resolutions (even if range) is a bad practice and should be avoided if possible, as it may cause other breakages. Not to say that it wouldn't work in this specific instance (it probably would).

@G-Rath
Copy link

G-Rath commented May 18, 2020

Sorry y'all I was asleep when the email came in:

Hi Gareth,

Thank you for contacting the npm security team. We have updated the advisory and versions >=1.18.1 are marked as unnaffected.

@zwbetz-gh
Copy link

zwbetz-gh commented May 18, 2020

Forcing resolutions (even if range) is a bad practice and should be avoided if possible, as it may cause other breakages. Not to say that it wouldn't work in this specific instance (it probably would).

That is a blanket statement. Forcing resolutions works fine in our project. But I would agree that it needs to be evaluated on a case-by-case basis.

Valid reasons for using: https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-why-would-you-want-to-do-this

@AviVahl
Copy link

AviVahl commented May 18, 2020

Forcing resolution forces all instances of that package in the tree to use the same major. This can obviously break (it's one of the things that semver solves).

Of course, if your tree already contains the same major, it can work, and even improve deduping. Or, if usages of those packages is limited, it might not blow up. Or, if the majors are compatible, it might also work.

In addition, using it for production dependencies does not affect your consumers. That's an important aspect usually forgotten.

It's one of those functionalities that you better understand the implications before using, and I wouldn't personally suggest it in yet-another-issue as a go-to solution.

EDIT: I remember people also suggesting it for the mkdirp vulnerability, and in that instance it was completely incorrect, as the API of the library completely changed between majors.

@sonikamah
Copy link

sonikamah commented May 19, 2020

Thanks, I am using netlify and I was also getting the same error yesterday. It got resolved through npm audit fix and just delete http-proxy from node-modules then do npm i.

                   === npm audit security report ===


                             Manual Review
         Some vulnerabilities require your attention to resolve

      Visit https://go.npm.me/audit-guide for additional guidance

High Denial of Service

Package http-proxy

Patched in No patch available

Dependency of react-scripts

Path react-scripts > webpack-dev-server > http-proxy-middleware >
http-proxy

More info https://npmjs.com/advisories/1486

Low Prototype Pollution

Package yargs-parser

Patched in >=13.1.2 <14.0.0 || >=15.0.1 <16.0.0 || >=18.1.2

Dependency of react-scripts

Path react-scripts > webpack-dev-server > yargs > yargs-parser

More info https://npmjs.com/advisories/1500

found 2 vulnerabilities (1 low, 1 high) in 1690 scanned packages
2 vulnerabilities require manual review. See the full report for details.

@Hypnosphi
Copy link

Hypnosphi commented May 19, 2020

@AviVahl

Forcing resolution forces all instances of that package in the tree to use the same major.

That's why I always specify the parent, e.g:

"resolutions": {
  "**/http-proxy-middleware/http-proxy": "^1.18.1"
}

Or even the full path:

"resolutions": {
  "react-scripts/webpack-dev-server/http-proxy-middleware/http-proxy": "^1.18.1"
}

@AviVahl
Copy link

AviVahl commented May 19, 2020

I can't say I think this is much better.

http-proxy-middleware uses semver caret for http-proxy so there's absolutely no reason to use a forced resolution. Just let it resolve to the latest version. The original range covers the new patch version.

Referring to the above example, forced resolutions might actually cause future breakage. Consider a case where http-proxy releases a breaking v2.0.0, and http-proxy-middleware upgrades, and the forced range is left in place (such configurations have a tendency to stick around)... Stuff can then break.. Yeah, yarn would warn about incompatible ranges. Developers love to ignore these warnings.

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