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: Address prototype pollution vulnerability in merge function #12

Conversation

ohad2712
Copy link
Contributor

@ohad2712 ohad2712 commented Jul 5, 2021

I have verified that this lib's merge function has a Prototype Pollution vulnerability in predefine.

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

PoC (Before):

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

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

This PR edits the vulnerable place, and adds a test to make sure it is unexploiable.

@davedoesdev
Copy link

Seems sensible

@davedoesdev
Copy link

FYI: Published as @davedoesdev/predefine@0.1.3

index.js Outdated Show resolved Hide resolved
@lpinca
Copy link
Contributor

lpinca commented Jan 23, 2022

@Swaagie can you merge this and cut a new release if @3rd-Eden is ok with it?

@lpinca
Copy link
Contributor

lpinca commented Jan 23, 2022

Also CCing: @indexzero

@lpinca
Copy link
Contributor

lpinca commented Jan 30, 2022

@jcrugzz ?

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@Swaagie Swaagie merged commit 1a86a01 into bigpipe:master Jan 30, 2022
@MateuszKikmunter
Copy link

Is this really solved @Swaagie? Still getting warnings in Synk reports. Could it be false positive vulnerability report?
Screenshot 2022-02-21 at 17 50 22

@lpinca
Copy link
Contributor

lpinca commented Feb 21, 2022

Is this really solved @Swaagie? Still getting warnings in Synk reports. Could it be false positive vulnerability report?

Yes, see 29851b6.

@MateuszKikmunter
Copy link

Is this really solved @Swaagie? Still getting warnings in Synk reports. Could it be false positive vulnerability report?

Yes, see 29851b6.

Thanks @lpinca !

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

Successfully merging this pull request may close these issues.

None yet

5 participants