remy / nodemon

Monitor for any changes in your node.js application and automatically restart the server - perfect for development
http://nodemon.io/
MIT License
26.31k stars 1.73k forks source link

[ CVE-2022-37600 ] / Prototype Pollution found in clone.js #2068

Closed secdevlpr26 closed 2 years ago

secdevlpr26 commented 2 years ago

Prototype pollution vulnerability in function clone in clone.js in remy nodemon 2.0.8 via the attr variable in clone.js.

The prototype pollution vulnerability can be mitigated with several best practices described here: https://learn.snyk.io/lessons/prototype-pollution/javascript/

remy commented 2 years ago

And how are you exploiting this code exactly?

remy commented 2 years ago

More importantly, can I ask where you copied this text from?

rezoled commented 2 years ago

It seems like a critical CVE was opened for this issue: https://ossindex.sonatype.org/vulnerability/CVE-2022-37600?component-type=npm&component-name=nodemon&utm_source=dependency-check&utm_medium=integration&utm_content=7.2.1 When checking for dependency issues it fails the most recent version of nodemon (2.0.20) because of this ticket.

remy commented 2 years ago

Yeah, because of this ticket not because there's actually an exposed vuln... ffs.

remy commented 2 years ago

Yeah, so this is a completely false report that can't be replicated. Here's the code in question: https://jsbin.com/rimurur/2/edit?console

What's extra rubbish is that sonartype has accepted the cve without any proof of concept.

What a waste of everyone's time.

Garrestocles commented 2 years ago

Hi @remy,

Sorry about the consternation this is causing. The Security Research team at Sonatype looked into the vulnerability and it does appear that @Supraja9726 reported a real prototype pollution issue. As you've pointed out, because the report was a little vague, we wrote out a PoC for it to make sure. Assuming you want it, what would be the best way for us to send it to you?

remy commented 2 years ago

@Garrestocles you can send it to remy@remysharp.com

remy commented 2 years ago

Sent a reply, the PoC doesn't actually use nodemon to pollute the chain.

huineng commented 2 years ago

It's cumbersome to follow up on all these CVE's, on top of that this also reported as critical. I would appreciate it to know if this is going to be solved somehow, or if i can suppress it.

Thanks

remy commented 2 years ago

@huineng I'm fairly sure this is a false and wrongly been report. The PoC was this:

    const nodemon = require("nodemon");
    const target = {};

    nodemon({

        a: target, 

        b: __proto__.constructor.prototype.toString = function() {

            console.log("toString is polluted") 

    });
    const newTarget = {};
    newTarget.toString();

Which, for the keen eyed of you, can see that the pollution happens outside and regardless of nodemon. i.e. this is the same:

    const target = {};

    {

        a: target, 

        b: __proto__.constructor.prototype.toString = function() {

            console.log("toString is polluted") 

    }
    const newTarget = {};
    newTarget.toString();

I'm just waiting on a new PoC (I'm guessing @Garrestocles is on US time and/or is busy with real work :) )

huineng commented 2 years ago

thanks for your quick reply, i agree , that example can apply to any function.

Garrestocles commented 2 years ago

We looked at it again and you're right, @remy. There's that getOwnPropertyNames call in your merge method that's preventing the pollution of the prototype chain. We'll go ahead and remove it from our data.

So, I know other organizations also look at Github issues for vulnerability advisories, so you might see this showing up from other vendors in the future. Were I you, I'd remove the CVE from the title of this ticket, and that might save you from future headaches if they haven't picked it up already. I see that this CVE ID in mitre/NVD is still listed as reserved, so I assume it hasn't actually been reported yet. Unless you see something we're missing here, @Supraja9726, would you mark this CVE as rejected in whatever CNA you were using, please?

secdevlpr26 commented 1 year ago

Hello,

All the reports are based on the research work of my colleague (you can find her paper's link below) and I am reporting them here as per her analysis and records.

https://dl.acm.org/doi/pdf/10.1145/3488932.3497769 - This is the published paper with the Github link to her static analysis tool. The mentioned code has been flagged potentially vulnerable by the static analysis tool Thanks

remy commented 1 year ago

@Supraja9726 right, but the problem is that you created a full CVE based on "potentially" when in fact the reality was that it was not possible.

This means there's a fault in the static analysis (likely that it's not thorough enough) and there's a fault in your process of follow up.

Currently this process, in my opinion, is irresponsible and needs to be rectified as I've seen the same thing happen to other maintainers having the burnden of having to prove there's no vulnerability.

secdevlpr26 commented 1 year ago

Please refer to the below link for the PoC: https://jsbin.com/qecehotoqo/edit?js,console

This does pollute the target object (although the global level object is not affected in this case) which can still be a potential danger to the places in the code where this object has been used.

For what it's worth, you can place controls on this target object such that the proto properties are not copied. Thanks

remy commented 1 year ago

@Supraja9726 that's cute, except it's not my code. This is the clone function, last touched in 2015: https://github.com/remy/nodemon/blob/main/lib/utils/clone.js

So this begs the question, what on earth was your source for you to go off an (wrongly) create a cve?