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

prototype pollution vulnearability due to hoek dependency #225

Closed
nishch opened this issue Mar 16, 2018 · 8 comments
Closed

prototype pollution vulnearability due to hoek dependency #225

nishch opened this issue Mar 16, 2018 · 8 comments

Comments

@nishch
Copy link

nishch commented Mar 16, 2018

Observed following security vulnerability in the grpc node package (even on latest version 1.10.0)

The merge function, and the applyToDefaults and applyToDefaultsWithShallow functions which leverage merge behind the scenes, are vulnerable to a prototype pollution attack when provided an unvalidated payload created from a JSON string containing the __proto__ property. This can be demonstrated like so: javascript var Hoek = require('hoek'); var malicious_payload = '{"__proto__":{"oops":"It works !"}}'; var a = {}; console.log("Before : " + a.oops); Hoek.merge({}, JSON.parse(malicious_payload)); console.log("After : " + a.oops);

This type of attack can be used to overwrite existing properties causing a potential denial of service.
This can be fixed by updating the dependency version to hoek@4.2.1

@murgatroid99
Copy link
Member

That is actually a transitive dependency through node-pre-gyp, which will be updated in the next patch release. Until then, I don't think there is any API surface for a malicious user to trigger this through gRPC.

@marr
Copy link

marr commented Mar 20, 2018

This is fixed in 1.10.0, correct?

@murgatroid99
Copy link
Member

It will be in 1.10.1, actually

@murgatroid99
Copy link
Member

We have now published grpc 1.10.1, which depends on node-pre-gyp 0.7.0.

@Arun-KumarH
Copy link

This issue still persists : https://nvd.nist.gov/vuln/detail/CVE-2018-3728 for hoek version < 5.0.3.

@murgatroid99
Copy link
Member

You're right. It looks like node-pre-gyp didn't actually remove that dependency until 0.9.0. So this will actually be fixed when we upgrade to that. Doing so is currently blocked on mapbox/node-pre-gyp#356

@yoiang
Copy link

yoiang commented May 4, 2018

It looks like mapbox/node-pre-gyp#356 / mapbox/node-pre-gyp#371 has been resolved in v0.10.0 of node-pre-gyp: https://github.com/mapbox/node-pre-gyp/commits/master

Does that mean we're good to go?

@nicolasnoble
Copy link
Member

1.11.1 has been published with the newest version of node-pre-gyp, and we're upmerging this to master, yes.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants