[utils] Fix GitHub-reported prototype pollution vulnerability in deepmerge
#41652
Conversation
Netlify deploy previewhttps://deploy-preview-41652--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
danilo-leal
changed the title
deepmergedeepmerge
danilo-leal
added
package: utils
|
Unfortunately (in some sense haha), it looks like GitHub makes code scanning results private even on open-source projects. Oops. Sorry about that! I'll post screenshots of the report here.
Here are the CWE links on the side of the page: Here is the line of code flagged by this code scan. It's a Vite-bundled dist file containing this Hopefully that will help you at least to get some idea of what I'm seeing and why I submitted this PR. Please let me know if there's any additional information I can provide from my end. Thanks! (Sorry for the delays. I was on vacation for two weeks.) |
|
I don't believe disallowing certain properties is the right solution here. i.e. the following code should pass, but it doesn't const result = deepmerge(
{},
JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'),
);
expect(result.__proto__).to.have.property('isAdmin');
expect({}).not.to.have.property('isAdmin');I believe the better solution is to avoid traversing into the prototype instead with index a8035a0c47..bb31f5b7cc 100644
--- a/packages/mui-utils/src/deepmerge/deepmerge.ts
+++ b/packages/mui-utils/src/deepmerge/deepmerge.ts
@@ -41,12 +41,11 @@ export default function deepmerge<T>(
if (isPlainObject(target) && isPlainObject(source)) {
Object.keys(source).forEach((key) => {
- // Avoid prototype pollution
- if (key === '__proto__') {
- return;
- }
-
- if (isPlainObject(source[key]) && key in target && isPlainObject(target[key])) {
+ if (
+ isPlainObject(source[key]) &&
+ Object.prototype.hasOwnProperty.call(target, key) &&
+ isPlainObject(target[key])
+ ) {
// Since `output` is a clone of `target` and we have narrowed `target` in this block we can cast to the same type.
(output as Record<keyof any, unknown>)[key] = deepmerge(target[key], source[key], options);
} else if (options.clone) {Which is also what the code scanning tool suggests as a solution. We could even consider disallowing the edit: This reminds me of this |
|
Seems reasonable to me! I was just trying to make as small a change as possible to avoid bothering anyone ;) I noticed the package: utils (private) label is now on this PR. Does that mean this PR doesn't actually do anything and the change will be made internally somewhere? If so, should I still update this PR with your suggestion, or is it irrelevant at this point? Thanks! |
|
The label just denotes that the exports of these packages are not meant to be exposed as public API. Rest assured, it is used by |
|
Thanks for letting me know! Looks like that change breaks a couple of tests:
|
|
Both tests seem to work on my end, can you push your changes to see if it fails in CI as well? |
|
I think I was running the tests wrong. Sorry about that! Pushed :) |
oliviertassinari
added
security
|
If I read this correctly, the framing of this pull request is wrong. There is no prototype pollution vulnerability. All the tests related to it pass with or without the changes to the code. It can be tested with: describe('deepmerge', () => {
// https://snyk.io/blog/after-three-years-of-silence-a-new-jquery-prototype-pollution-vulnerability-emerges-once-again/
it('should not be subject to prototype pollution via __proto__', () => {
const result = deepmerge(
{},
JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'),
{
clone: false,
},
);
// @ts-expect-error __proto__ is not on this object type
// eslint-disable-next-line no-proto
// expect(result.__proto__).to.have.property('isAdmin');
expect({}).not.to.have.property('isAdmin');
});
// https://cwe.mitre.org/data/definitions/915.html
it('should not be subject to prototype pollution via constructor', () => {
const result = deepmerge(
{},
JSON.parse('{ "myProperty": "a", "constructor" : { "prototype": { "isAdmin" : true } } }'),
{
clone: true,
},
);
// expect(result.constructor.prototype).to.have.property('isAdmin');
expect({}).not.to.have.property('isAdmin');
});
// https://cwe.mitre.org/data/definitions/915.html
it('should not be subject to prototype pollution via prototype', () => {
const result = deepmerge(
{},
JSON.parse('{ "myProperty": "a", "prototype": { "isAdmin" : true } }'),
{
clone: false,
},
);
// @ts-expect-error prototype is not on this object type
// expect(result.prototype).to.have.property('isAdmin');
expect({}).not.to.have.property('isAdmin');
});
it('should appropriately copy the fields without prototype pollution', () => {
const result = deepmerge(
{},
JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'),
);
// @ts-expect-error __proto__ is not on this object type
// eslint-disable-next-line no-proto
// expect(result.__proto__).to.have.property('isAdmin');
expect({}).not.to.have.property('isAdmin');
});
});What we fixed here looks like to be a small bug, where in the result of the deep merge, the key If there is truly a flaw, then we are missing a regression test case. |
|
I'm sorry for all the stir and for my lack of thoroughness and forthrightness; I didn't intend for all this to get out of hand as it did. I should have been more considerate, thoughtful, and clear in my approach to this situation. I recognize that I have caused a preemptive and likely unnecessary public stir over this, and I'm sorry for having done so. I hope you can forgive me, and I intend to do what I can to resolve this problem. Now, on to the work: I also noticed when working on my change that the tests succeeded whether or not I changed any code. However, I also noticed the test named Now (again, I should have explained this before. I'm sorry I didn't; I'm just hoping I can correct my error as best I can moving forward), I have dedicated further effort to figuring out what prototype pollution I am supposedly fixing with my change (according to the GitHub report) to make sure this change is actually necessary. However, I still have come up short. I have read everything I can about prototype pollution via (Note snyk mentions prototype pollution via It seems that original reference equality must be preserved for I see a couple of possibilities as to what is happening here:
With all the work I have done on this so far, I am leaning toward thinking 2 is more likely. As such, I'm going to request that they close https://security.snyk.io/vuln/SNYK-JS-MUIUTILS-7231125 for now. We can always ask them to re-open it later if we find a problem. Here are some other test cases I ran against Prototype pollution tests with "constructor"
it('should not be subject to prototype pollution via constructor using proxies', () => {
const proxy = new Proxy(
{},
{
has(target, p) {
if (p === 'constructor') {
return true;
}
return p in target;
},
},
);
deepmerge(proxy, JSON.parse('{ "constructor" : { "prototype": { "isAdmin" : true } } }'), {
clone: false,
});
expect({}).not.to.have.property('isAdmin');
});
it('should not be subject to prototype pollution via constructor and Object', () => {
deepmerge(Object, JSON.parse('{ "constructor" : { "prototype": { "isAdmin" : true } } }'), {
clone: false,
});
expect({}).not.to.have.property('isAdmin');
});
it('should not be subject to prototype pollution via prototype and Object', () => {
deepmerge(Object, JSON.parse('{ "prototype": { "isAdmin" : true } }'), {
clone: false,
});
expect({}).not.to.have.property('isAdmin');
});
// https://cwe.mitre.org/data/definitions/915.html
it('should not be subject to prototype pollution via constructor 2', () => {
deepmerge({}, JSON.parse('{ "myProperty": "a", "constructor" : { "isAdmin" : true } }'), {
clone: false,
});
expect(Object).not.to.have.property('isAdmin');
});
// https://cwe.mitre.org/data/definitions/915.html
it('should not be subject to prototype pollution via constructor 3', () => {
deepmerge(
{},
{ myProperty: 'a', constructor: () => ({ isAdmin: true }) },
{
clone: false,
},
);
expect(Object).not.to.have.property('isAdmin');
});
// https://cwe.mitre.org/data/definitions/915.html
it('should not be subject to prototype pollution via constructor and class', () => {
const NewClass = class { constructor() {} }
deepmerge(
new NewClass(),
{ myProperty: 'a', constructor: () => ({ isAdmin: true }) },
{
clone: false,
},
);
expect(Object).not.to.have.property('isAdmin');
});Please let me know if you have ideas regarding how to successfully cause prototype pollution using |
Added more checks for prototype pollution in
deepmergeto abide by GitHub's vulnerability report in another repo using MUI.It was hard to figure out which library was contributing the code the reported vulnerability is in, but this line helped me to find
isPlainObjectexported from the same module which then led me to finddeepmerge.Thanks for all the hard work!