-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: prototype poisoning (CWE-915) #3192
Conversation
Added protection from prototype poisoning to mapObjIndexed()
| // Prevention of prototype poisoning | ||
| if (key !== "__proto__" && key !== "prototype" && key !== "constructor") { | ||
| acc[key] = fn(obj[key], key, obj); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me but I think we could have a test to make sure it doesn't happen again.
Basically what's in your JSFiddle could be the basis for a regression test.
What do you think?
|
I also wonder whether this would apply to other things? I'm thinking perhaps prop & path functions or lenses? I don't know I haven't checked yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only are there changes that need to be made, I'm not convinced this is actually what prototype poisoning is about.
I had to think it through a bit, and will leave a more detailed comment below.
| @@ -26,7 +26,10 @@ import keys from './keys.js'; | |||
| */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would definitely need to update the documentation too.
| @@ -26,7 +26,10 @@ import keys from './keys.js'; | |||
| */ | |||
| var mapObjIndexed = _curry2(function mapObjIndexed(fn, obj) { | |||
| return _reduce(function(acc, key) { | |||
| acc[key] = fn(obj[key], key, obj); | |||
| // Prevention of prototype poisoning | |||
| if (key !== "__proto__" && key !== "prototype" && key !== "constructor") { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand __proto__ and constructor. Is there an attack here that uses prototype directly?
|
I think "prototype poisoning" is about altering the prototype of core js elements such as The canonical example seems to look like this: const naiveMerge = (a, b) => {
// Just to illustrate the problem. But could happen with a real recursive merge
Object .entries (b) .forEach (([k, v]) =>
Object .keys (v) .forEach (inner => a[k][inner] = v[inner])
)
return a
}
// This user has no `isAdmin` field
const user = {name: 'fred'}
console.log(`Before: ${user.isAdmin}`); //=> undefined
// We get this from some perhaps-untrustworthy source
const maliciousUserInput = '{"__proto__": {"isAdmin": true}}'
// but we're smart enough to sanitize it with
const ourObject = naiveMerge ({}, JSON.parse(maliciousUserInput))
// However, naiveMerge adds `isAdmin` to `Object.prototype`, with value `true`
// And now that same user has `isAdmin` set
console.log(`After: ${user.isAdmin}`); //=> true !!!
// if we call naiveMerge again we get an error.
// But a less naive version might go undetected.Both jQuery and lodash were caught by this. I don't think it's true for Ramda because of our immutable design. It doesn't happen if we use But I don't think we need this change just yet. |
|
@CrossEye What about this though? const isAdmin = o => o.admin == true;
const check = compose(isAdmin, mapObjIndexed(identity));
check(JSON.parse('{"__proto__":{"admin":true}}'));
//=> trueI think the concern is that with |
|
But that's a very different attack surface. If you are creating your user account by JSON-parsing something from an untrustworthy source, then they could just as well pass you The problem is not with what happens with this object, but with what it allows on all other objects. It's these sorts of things that I think people are worried about:
That's a real worry, and I'm far from certain Ramda doesn't have any such issues. I think we need to go through it to check. But I'm less worried about |
|
@CrossEye To continue the Considering including "prototype" in the check - it is a common practice to account for all 3 properties, as they are known to be exploited in similar scenarios. I agree that in this case it is unnecessary, however I would suggest to keep it in case something changes in the implementation (e.g. recursive version of property assignment). |
|
Do you have examples of CVEs for this problem, or prior art of prohibiting those properties? What I'm worried about is suddenly having magical property names we can no longer support. Users might have legitimate needs for I don't want to diminish the issue here. It's worth discussing. But now that you've brought up prototype poisoning, I think it's much more important to test whether this can happen in Ramda. |
|
We are a consumer of ramda, and got a VULN ticket to fix this |
|
@yanruzhang: I know nothing about such VULN tickets. I don't have any sources for this. I would hope that whatever gave you such a ticket would be able to give you information as well. (And I hope you would share that with us!) The code here is for a specific function and IMHO the test case does not actual demonstrate that vulnerability. But that does not mean there is not one around, in this or another function. But I'd like to see evidence of the problem before worrying about solutions. |
|
@yanruzhang: This PR is not about But can you tell me the source of that report? I haven't seen any reports of Prototype Pollution in Ramda, except for this issue, which I think is invalid, and I've been looking around for references to it in various sources, but I don't know much about them. What generates that report for you? Does it have an online reference to the vulnerability?
|
|
@CrossEye |
|
@yanruzhang: I did find that one in further research, but I can't find anywhere it gives a demo of the supposed vulnerability. I'll keep looking, but that site seems pretty damned opaque. Do you know anything about it? |
|
@CrossEye |
|
If that's all the information we can get, then I'm pretty sure I can simply say no, that cannot happen.
If you want to dig through every Ramda function to see if there's some possible prototype pollution, feel free. Otherwise, I feel it's incumbent upon the reporter to demonstrate the existence of a problem. But I can find no more information from veracode. I've reached out to them but not heard back. The simplest thing I can think of to check is the sort of canonical prototype pollution; the Ramda equivalent does not share the issue. So my first thought is that no, this is not a real issue. The code supplied in this PR also does not demonstrate prototype pollution either. Without an actual demonstration, I'm not sure where to go. I don't want to close this. What I really want is to find out what veracode says in an actual problem. But I don't have any idea how to pursue it. Any ideas? |
|
I've posted a question in StackOverflow about this: https://stackoverflow.com/q/69936667. Perhaps someone there will have some information. |
|
So I've heard back from Veracode, and their source for the issue is this PR. I will need to spend some time to thoroughly write up my objection. But it's pretty clear to me that this does not do what's described as prototype pollution. That you can create an object with a prototype is not prototype pollution. Or if it is, then libraries are irrelevant, as a simple tweak to the original Fiddle shows that the same thing is true for The snyk lodash report has a good test case for prototype pollution: const mergeFn = require('lodash').defaultsDeep;
const payload = '{"constructor": {"prototype": {"a0": true}}}'
function check() {
mergeFn({}, JSON.parse(payload));
if (({})[`a0`] === true) {
console.log(`Vulnerable to Prototype Pollution via ${payload}`);
}
}
check();The closest Ramda equivalent is |
|
Below is a response I've drafted to send to Veracode. Does anyone have any suggested improvements? @ramda/core
|
|
@CrossEye So the GH issue was opened to fix an alleged vulnerability which VeraCode illustrated with my own example which wasn't even close to describing an actual prototype pollution? Bit confused right now. Thanks for looking into this! LGTM |
|
I've had no response yet, but now they've changed the description to:
(Of course I'm trying to decide if I want to get snarky and ask if they've filed the same issue against For now, though, the only response seems to be that I've been added to their marketing email list. 😦 |
|
Hey Scott, really appreciate you looking into this. Longtime fan and user of Ramda here! Through my enterprise employer's account with VeraCode itself I was able to see the Thus, their proof-of-concept is: https://jsfiddle.net/3pomzw5g/2/ The other links merely reference this PR and the mitigation commit. And yes, quite wacky that one has to be logged in to be able to see a public |
|
Thank you very much. That helps a lot. It confirms what I suspected from their email. I feel as though they've changed only their description in response to my dispute. But at least I believe it's progress. Also, "wacky" is a much more polite word than most of the ones I had in mind! 😄 |
|
Hey Scott, sure no worries. Yes I think they reacted to your email at least :D And apparently it's a been classified as a "SourceClear Premium (No CVE)" 'disclosure' (more like 'opinion piece'!) which renders the details and -worst of it all- the PoC viewable for logged-in customers only.. what a way to treat FOSS! Within my sphere of influence (read: project) we're going to go for bandaids like using more |
|
After no response for a week, I've sent the following to Veracode:
I've got to say this is extremely frustrating. |
|
I added an answer to the StackOverflow question, summing up the experience with this issue and Veracode. |
|
@CrossEye @customcommander hey folks, seems like someone assigned CVE-2021-42581 for this? assuming it should be disputed? |
|
@assaf-benjosef: Oh no, it's back!! I have several evenings free this week and I'll try to take a look. Do you know anything about the dispute resolution procedure. I'm hoping it's easier than my Veracode experience. |
|
@CrossEye I believe this form would be the best bet: https://cveform.mitre.org/ |
|
Thank you. I made this request:
We'll see what happens now. |
|
Interestingly Snyk.io has also added CVE-2021-42581 to their database, but only for Grafana packages, which I suppose depend on this library. |
|
Well this appears to be a "malicious CVE"... the description mentions 0.27.0 as affected version, even when 0.27.1 was already released. That kind of implies that 0.27.1 was released as a fix for the reported vulnerability, increasing its credibility. @CrossEye MITRE is the CNA who issues. I'm curious if they can tell you who asked them to submit this 10 days ago. @Marynk Was that you? |
ahh seems like it was automatically created as a linux vulnerability due to a few distros adding this to their advisory database. It will be automatically revoked from snyk.io once the CVE is revoked. What we had control over was the decision of publishing an advisory against the independent ramda NPM package, which we did not do. Did MITRE not reply to your request yet @CrossEye ? |
|
Nothing from MITRE. This is starting to feel like Veracode all over again. I won't have time this evening, but will try to get more out of MITRE tomorrow. After the dead silence on my previous message, I'm not really hopeful, however. |
|
Just sent another request to Mitre. The first one was never even acknowledged. Trying a different email address just in case.
Let's see if we have any better luck this time. |
|
Dug up a direct email for Mitre, meant for reporting vulnerabilities, hoping they can at least route this to the appropriate group. Something has to work, eventually. |
|
I got a response from my email to Mitre, which told me that they had already updated the CVE database. It now lists the claim as "disputed", which is progress. I really don't know the resolution process from there. But I have confirmed that this is updated at https://www.cve.org/CVERecord?id=CVE-2021-42581 and at https://nvd.nist.gov/vuln/detail/CVE-2021-42581, which I think are the key central sites. |
|
So, at long last, an update. This whole business had chased me from doing OSS for the past year. I was just too frustrated. While I'm hoping I'm back. But we'll have to find out. You see, the answer is not good. I had opened an email thread with Mitre, but they stopped responding, and didn't respond to pings, until today, fifteen months after I first started speaking with them. I had pinged again this morning, and this afternoon, I got this reply:
As far as I can tell, this means that Mitre takes no responsibility for fixing the problem they've caused. The reason I ended up pinging them was that a coworker who was facing an issue from BlackDuck for another false vulnerability knew that I had been through this before and asked for advice. (And if I become the go-to guy at my company for dealing with this, it will be a hard choice between retirement and suicide!) He was right; it was a false report. And I checked in with a third false report I knew about and found none of them had any response from Mitre. So this does not seem to be an isolated incident. Here's my response, sent at the local end of the work-week. I hope I do hear back soon.
We'll see what next week brings. One thing I'm not sure I ever mentioned above, Veracode, to their credit, did eventually write a blog post demonstrating my point. (Not that they told me about it, but someone posted a link on the StackOverflow answer.) So one security company down, one zillion left to go. I've got to say, there is a real temptation to find a way to post bounties for people reporting vulnerabilities within Mitre itself! I'd love to see how well they enjoy the process. |
|
BTW, two issues I'm tracking with similar Mitre issues: |
|
Shockingly, Mitre responded in just 67 minutes! I assume this guy has been instructed to mollify me after Mitre ignored me for well over a year.
Politely phrased, but it seems to be "Yeah, good luck, buddy!" I'm polite in my response too:
Okay, so maybe a little zing in the last line. Who can blame me? |
|
I had another exchange just before my bedtime last night:
to which I responded:
(And then a follow-up from me apologizing for typos; I really shouldn't type anything larger than a text message on my phone!) It sounds like they might actually be willing to discuss fixes to this issue. As I said, though, this is going to take some real thinking. |
|
Hey @CrossEye, I work as a security analyst at BlackDuck by Synopsys. I have been made aware of this issue and have looked into the "vulnerability". After playing about with the fiddlejs PoC we can confirm that we too believe you are correct and this is not a security vulnerability in the ramda library and is in fact the normal behaviour of JavaScript. The PoC can be recreated using just Object.assign . Additional Objects created do not inherit polluted properties as is the dangers of Prototype Pollution. Issues such as false positive vulnerability reports cause issues for all parties involved and especially OSS maintainers. The BlackDuck Security Advisory associated with the CVE will be updated to state that this is not a vulnerability. Since I have no control over the CVE listing, I can only update our advisory to state that this is not a security issue. By updating it hopefully it will deflect from the claims that this is a security vulnerability. Hopefully Mitre will follow suit and remove the CVE as it is digested by so many security tools. TLDR: ramda is not vulnerable to Prototype Pollution as claimed in CVE-2021-42581. Also, heres a modified jsfiddle of the "PoC" that shows no global prototypes are manipulated. |
|
@RustyButtons: Thank you. That is fantastic news! I'm booked pretty solid for the next two days, but I will write a note detailed response as soon as I can. |
|
Again, thank you. If you've actually read this whole thread, you will know that dealing with these issues has been enough of a headache that it's mostly chased me from doing open source for more than a year. After fighting it out with Veracode for weeks, a few months later, we got the same problems with Mitre, who were even less responsive. Basically crickets for over a year, until ten weeks ago, when I got a "sorry, but that's just the way it is" answer. I'm really glad to hear that Black Duck will be addressing this. Doubly so as BD is the tool used at my day job. If you have a little time, I would love advice on how on how Ramda might proceed. When Veracode did recognize that this wasn't a real issue and pulled their report, they were kind enough to write a blog post debunking the CVE. While I can point people to that post, I don't know who to point to it. Any insights you can offer would be much appreciated. (And if you don't want to do it in a public forum, you can reach me at scott.sauyet@gmail.com.) Again, thank you very much. This is so refreshing! |


NOTE from the project maintainer: Security Professionals: Please read the whole thread before accepting this at face value. This is not a real vulnerability! We're just trying to get the CVE folks to understand that (or even to respond to us.☹️ )
Added protection from prototype poisoning to mapObjIndexed().
For Proof of Concept exploit of the vulnerability visit https://jsfiddle.net/3pomzw5g/2/