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

fix: replace vulnerable merge function of predefine package with not vulnerable lodash.merge implementation #336

Merged
merged 1 commit into from Jul 5, 2021

Conversation

Kirill89
Copy link
Contributor

@Kirill89 Kirill89 commented Jul 5, 2021

What does this PR do?

According to Snyk broker has a Prototype Pollution vulnerability in predefine package introduced via primus@6.1.0 › fusing@1.0.0 › predefine@0.1.2. Together with @ohad2712 we verified that no user input flows to the vulnerable function merge directly or indirectly. To be precise, we observed some user input flows to merge but only of type String – which makes it not exploitable.

To make double sure we decided to replace vulnerable implementation of the merge function with similar function lodash.merge, which is not vulnerable.

To replace the function we require predefine package as early as possible and replace the function by directly overwriting it.

Where should the reviewer start?

lib/index.js

How should this be manually tested?

Any background context you want to provide?

https://snyk.io/vuln/SNYK-JS-PREDEFINE-1054935

predefine:

Vulnerable code: https://github.com/bigpipe/predefine/blob/0.1.2/index.js#L263-L294

PoC:

const predefine = require('predefine');
predefine.merge({}, JSON.parse('{"__proto__": {"a": "b"}}'));

console.log(({}).a === 'b' ? 'exploitable' : 'unexploitable'); // exploitable

Merge function is not used anywhere inside the package itself, but exported as part of the API.

fusing:

Vulnerable code: https://github.com/bigpipe/fusing/blob/1.0/index.js#L140

merge function is not used anywhere inside fusing package as well, but re-exported as part of prototype for each class.

primus:

  • merge function is used in createServer function as 3d argument.
  • merge function is used in transformers/engine.io/client.js but the only user input seems to be the url property.

@Kirill89 Kirill89 requested a review from a team as a code owner July 5, 2021 12:05
@CLAassistant
Copy link

CLAassistant commented Jul 5, 2021

CLA assistant check
All committers have signed the CLA.

@Kirill89 Kirill89 self-assigned this Jul 5, 2021
@Kirill89 Kirill89 merged commit d1daaee into master Jul 5, 2021
@Kirill89 Kirill89 deleted the fix/predefine-pp branch July 5, 2021 13:42
@snyksec
Copy link

snyksec commented Jul 5, 2021

🎉 This PR is included in version 4.109.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Kirill89
Copy link
Contributor Author

Kirill89 commented Jul 6, 2021

Please delete this workaround when bigpipe/predefine#12 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants