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

Vulnerability in node-forge dependency (CVE-2020-7720) #5145

Closed
stanlemon opened this issue Sep 15, 2020 · 13 comments · Fixed by #5149
Closed

Vulnerability in node-forge dependency (CVE-2020-7720) #5145

stanlemon opened this issue Sep 15, 2020 · 13 comments · Fixed by #5149

Comments

@stanlemon
Copy link

🐛 bug report

CVE-2020-7720

The package node-forge before 0.10.0 is vulnerable to Prototype Pollution via the util.setPath function. Note: Version 0.10.0 is a breaking change removing the vulnerable functions.

🎛 Configuration (.babelrc, package.json, cli command)

Include parcel, node-forge is a dependency and is vulnerable.

🤔 Expected Behavior

Install parcel with no vulnerabilities.

😯 Current Behavior

Install parcel with vulnerability.

💁 Possible Solution

Upgrade node-forge.

🔦 Context

Insecure application.

💻 Code Sample

Use the amazing parcel!

🌍 Your Environment

Software Version(s)
Parcel 1.12.4
Node *
npm/Yarn *
Operating System *
@DeMoorJasper
Copy link
Member

DeMoorJasper commented Sep 16, 2020

This isn't really a security vulnerability that applies to Parcel, you can look at the changelog of node-forge where they mention this vulnerability applies to utils that weren't even being used in node-forge and neither did we in Parcel: https://github.com/digitalbazaar/forge/blob/master/CHANGELOG.md#0100---2020-09-01

To get rid of the useless warning I created a PR to fix this in Parcel 2 #5149 but unfortunately we don't publish new v1 releases

@zawys
Copy link

zawys commented Sep 16, 2020

Dependabot notifies:
grafik
Cannot be solved by yarn upgrade.

andrewdotn added a commit to andrewdotn/2md that referenced this issue Sep 18, 2020
Only used in parcel, claims not to be vulnerable, but says “we don't
publish new v1 releases” even though v2 is not released.
parcel-bundler/parcel#5145
@Flupster
Copy link

Flupster commented Oct 2, 2020

Are we going to leave node-forge 0.7.6 in the 1.x version?

npm i parcel-bundler; npm ls node-forge

/home/flupster/test
└─┬ parcel-bundler@1.12.4
  └── node-forge@0.7.6 

@MrBrownser
Copy link

I know the exposed method is not applied in Parcel, but being this a security topic I think it should be fixed in actual 1.X version.
In my case this is stopping my build pipeline (shared with other Node applications) where we do have an npm audit step checking all "high and above" vulnerabilities, and I bet I won't be the only one.

I could ship a PR if this could help, just let me know.

@stanlemon
Copy link
Author

@MrBrownser a PR was previously submitted, see #5146 but @DeMoorJasper closed it.

Part of the issue here is the vulnerable code is available to a project using parcel, it's not just a matter of parcel not using the vulnerable code in question. Any enterprise software company has scanning software (GitHub, Snyk, etc.) and this vulnerability does create risk that has to be explained or mitigated. It is often much more challenging to explain why a certain amount of risk should be accepted when the alternative is switching to something else. The alternative of just using 2.0 isn't really viable either as many organizations limit using beta software because it's generally considered to not be production-ready.

I'm not looking to create issues with the maintainers of parcel, the software has been good to me and helped me move fast on a number of projects. At the same time the unwillingness to look at this is disheartening and discouraging.

@DeMoorJasper
Copy link
Member

This is not an actual security issue it isn't even a real security issue in node-forge, these util functions weren't used by node-forge nor Parcel so it's a false positive by the npm security team, if you want to blame anyone blame their poor security audit tools (this issue should've been flagged low severity at most). This doesn't create any risk for any organisation, although warnings don't look good and many people won't bother to actually look up a security issue and just trust npm completely.

I've started a discussion about this internally, if this gets through we'll make a patch release just to get rid of the warning...

@danielrbradley
Copy link

Just in case anyone's looking for a workaround - I just added a manual resolution for the sub-package using the 'resolutions' feature in the package.json:

{
 ...
  "resolutions": {
    "node-forge": "0.10.0"
  }
}

@woutervanbakel
Copy link

@DeMoorJasper what is the reason of not publishing a patch on v1 anymore? Vulnerability to Parcel or not this blocks the CI of many happy Parcel users on the audit step. As there is also still no "overrides" function in npm there is not really a neat workaround.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Oct 29, 2020

@woutervanbakel I think the main reason is that it is not easy to publish a new version for Parcel 1 as testing and CI is kind of messed up for master branch.

Besides that Devon is also the only person who can publish the packages... Like I said I brought this up internally without any response so can't do much more than that

@woutervanbakel
Copy link

@DeMoorJasper The publishing of packages part I get. It is of-course a pity that only one person can publish ;-)

But in terms of messing up the master branch, there are tags used in this repository? Or at least the commit hash of the published version is known right?

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Oct 29, 2020

@woutervanbakel yeah it's definitely possible just kind of complicated. Would fix this if I could

@phocks
Copy link

phocks commented Oct 30, 2020

Just in case anyone's looking for a workaround - I just added a manual resolution for the sub-package using the 'resolutions' feature in the package.json:

{
 ...
  "resolutions": {
    "node-forge": "0.10.0"
  }
}

Thanks. Also needed this in my "scripts":

"scripts": {
  "preinstall": "npx npm-force-resolutions"
}

(from https://www.npmjs.com/package/npm-force-resolutions)

@wis
Copy link

wis commented Nov 12, 2020

@phocks I use yarn, I didn't need to do this.
Jasper I agree with you, but GitHub repo vulnerable dependencies warnings are annoying so you need to fix them even if you're unaffected.

iBelieve added a commit to iBelieve/gty-daily-audio-bible that referenced this issue Dec 6, 2020
Parcel Bundler depends on a vulnerable version of node-forge but is not directly affected by the vulnerability. They're not publishing updates to v1.x anymore, so we manually force resolution to a non-vulnerable version of node-forge.

See parcel-bundler/parcel#5145
pwmarcz added a commit to pwmarcz/autotable that referenced this issue Jan 23, 2021
foobarbecue added a commit to nasa-jpl/sstmp that referenced this issue Feb 7, 2021
Makes the vulnerability notice for node-forge go away
ingara added a commit to navikt/su-se-framover that referenced this issue Feb 12, 2021
It's a dependency of parcel, and as they are unwilling/unable update it
on their end we have to force it.
See parcel-bundler/parcel#5145.

Having to add the fix both in package.json and server/package.json
also highlights that our setup is not that great. We have parcel-bundler
as a dependency in package.json for building the frontend and in
server/package.json for running the dev version. Ideally we should only
have it as a dependency one of these places.
ingara added a commit to navikt/su-se-framover that referenced this issue Feb 12, 2021
It's a dependency of parcel, and as they are unwilling/unable update it
on their end we have to force it.
See parcel-bundler/parcel#5145.

Having to add the fix both in package.json and server/package.json
also highlights that our setup is not that great. We have parcel-bundler
as a dependency in package.json for building the frontend and in
server/package.json for running the dev version. Ideally we should only
have it as a dependency one of these places.
washingtonsteven added a commit to washingtonsteven/genmo-v2 that referenced this issue Feb 22, 2021
washingtonsteven added a commit to washingtonsteven/genmo-v2 that referenced this issue Feb 22, 2021
mikehenrty added a commit to mikehenrty/cubic-world that referenced this issue Mar 7, 2021
ctrueden added a commit to imagej/imagej.github.io that referenced this issue May 7, 2021
It can't be updated normally due to parcel-bundler.
But we can use the workaround described at:

  parcel-bundler/parcel#5145 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants