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

Prevent prototype injection #216

Closed
pixelspark opened this issue Dec 3, 2018 · 7 comments · Fixed by #219
Closed

Prevent prototype injection #216

pixelspark opened this issue Dec 3, 2018 · 7 comments · Fixed by #219
Assignees

Comments

@pixelspark
Copy link

It appears fast-json-patch may be vulnerable to prototype injection depending on the way it is used. See e.g. the following code:

let fp = require("fast-json-patch");

function SomeClass() {
        this.foo = "bar";
}

let doc = new SomeClass();
let otherDoc = new SomeClass();

const patch = [
        {op: "replace", path: "/__proto__/x", value: "Hacked!"}
];

fp.applyPatch(doc, patch);
console.log(doc.x, otherDoc.x);

This will output 'hacked' twice, while there is no property x visible on object doc nor otherDoc when it is printed.

This becomes an issue in scenarios where patches cannot be trusted (and are not checked for this). Perhaps it is a good idea to disable patching of __proto__ (and maybe prototype as well? I haven't tested that yet) unless enabled by a flag (although I do not really see a use case where you would use JSON patches to fix up a prototype...).

@pixelspark
Copy link
Author

Using prototype instead of __proto__ throws an error, so at least this PoC doesn't work for prototype:

TypeError: Cannot read property 'x' of undefined
    at Object.replace (/x/node_modules/fast-json-patch/lib/core.js:27:26)
    at applyOperation (/x/node_modules/fast-json-patch/lib/core.js:231:60)
    at Object.applyPatch (/x/node_modules/fast-json-patch/lib/core.js:268:22)
    at Object.<anonymous> (/x/main.js:14:4)
    at Module._compile (internal/modules/cjs/loader.js:722:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:775:12)

@warpech
Copy link
Collaborator

warpech commented Jan 4, 2019

I guess, if we check for hasOwnProperty before applying the patch, that would be solved?

@alshakero
Copy link
Collaborator

hasOwnProperty method will only cover for replace. It won't work for add. I'll just ban __proto__.

@alshakero
Copy link
Collaborator

PR in #219

@pixelspark
Copy link
Author

Nice work 👍 perhaps it is also a good idea to look into why the version with prototype instead of __proto__ does not appear to work, just to be sure all cases are covered...

@warpech
Copy link
Collaborator

warpech commented Jan 7, 2019

After checking https://hackernoon.com/understand-nodejs-javascript-object-inheritance-proto-prototype-class-9bd951700b29, I guess that if obj is a object constructor function, then manipulating obj.prototype would affect instances of that function.

@nicdumz
Copy link

nicdumz commented May 31, 2021

PR #262 prevents another instance of this problem, as /constructor/prototype/polluted is still a way to achieve prototype pollution.

It would be great if we could merge it and/or address the underlying issue :-)

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 a pull request may close this issue.

4 participants