CVE-2021-42581: fix: prototype poisoning (CWE-915) by Marynk · Pull Request #3192 · ramda/ramda

Below is a response I’ve drafted to send to Veracode. Does anyone have any suggested improvements?

@ramda/core

Overview

There seems to be an erroneous vulnerability report in Veracode, saying that Ramda is subject to prototype pollution.

While we certainly have not definitively demonstrated that nothing in Ramda has this vulnerability, the examples so far are not examples of prototype pollution, only of the ability to create objects that the user didn’t know would contain custom prototypes.

It seems like this report should be withdrawn.

Prototype Pollution

Prototype pollution is the ability for crafted input supplied to a generic function to the alter the prototype of built-in JavaScript constructs, such as Object.

One example, from a WhiteSource blog, is:

const getObject = require(‘getobject’); var obj = {}; var obj2 = 1; console.log("Before Polluting : " + obj2.Check); //~> ‘Before Polluting : undefined’ getObject.set(obj, "__proto__.Check", “polluted”); console.log("After Polluting: " + obj2.Check); //~> ‘After Polluting: polluted’

Another one, closer to the case in question, from a snyk vulnerability report, looks like this:

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(); //~> ‘Vulnerable to Prototype Pollution via {"constructor": {"prototype": {"a0": true}}}’

What these have in common is that calling getObject.set or lodash.defaultsDeep modifies the prototype of the built-in Object, and thus they affect the behavior not just of the resulting object they return but also of any existing JavaScript objects and any newly created ones. This would allow a malicious user to perform denial-of-service attacks and even perform actions without proper permissions, such as modifying or deleting data in a Node.js application.

History

A Veracode vulnerability report said:

ramda is vulnerable to prototype pollution. An attacker can inject properties into existing construct prototypes via the _curry2 function and modify attributes such as proto, constructor, and prototype.

It has recently been updated to say this instead:

ramda is vulnerable to prototype pollution. An attacker is able to inject and modify attributes of an object through the mapObjIndexed function via the proto property.

(It seems that this update was only in the last day or so, but that’s not entirely clear.)

This was brought to our attention by a comment on an open issue in Ramda. That issue, as discussed below, does not demonstrate prototype pollution, although it’s an understandable mistake. The commenter was able to point us to that report, but there was no other information available.

I am one of the founder of Ramda and currently its chief maintainer. I reached out through Veracode’s contact form online asking for more details. When I didn’t hear back in a few days, I dug a bit deeper and found contact emails online as well. I emailed them, and a few days later got a response from a salesperson who thought I was in the market to buy their product. I explained the problem and was told that this would be forwarded to someone who might know more. Today, I heard back from someone else, a sales manager who had spoken to the technical team, who explained

We have based this artifact from the information available in #3192. In the Pull Request, there is a POC (https://jsfiddle.net/3pomzw5g/2/) clearly demonstrating the prototype pollution vulnerability in the mapObjIndexed function. In the demo, the user object is modified via the proto property and is considered a violation to the Integrity of the CIA triad. This has been reflected in our CVSS scoring for this vulnerability in our vuln db.

There is also an unmerged fix for the vulnerability which has also been included in our artifact ( 774f767 )

In other words, the issue that was used to comment on Veracode’s vulnerability report was earlier the issue which triggered the report. Oh my aching head! 😄

That response continued:

Please let me know if there is a dispute against the POC, and we can look further into this.

This document is my attempt to explain the issue and thoroughly explore the reported vulnerability.

Reported Vulnerability

The reported issue was demonstrated with the code on a Fiddle. It looks like this:

const hasOwn = JSON.parse(‘{"__proto__": {"isAdmin": true}}’); console.log(hasOwn.isAdmin); //~> undefined const mapped = R.mapObjIndexed((val) => val, hasOwn); console.log(mapped.isAdmin); //~> true

Note that hasOwn does not have isAdmin in its prototype, but mapped does have it.

The suggestion is that because of this Ramda’s mapObjIndexed can cause prototype pollution.

Dispute

All the top answers on a search for “prototype pollution” have variants of the description on codeburst:

Prototype Pollution, as the name suggests, is about polluting the prototype of a base object which can sometimes lead to arbitrary code execution.

The above code does not do so. It does not pollute the base objects. It simply offers a surprising way to adjust the prototype of one specific object. And this behavior has little to do with Ramda; it’s part of the base language as well. If we replaced the call to Ramda’s mapObjIndexed with one to JavaScript’s Object.assign, we get the exact same behavior. As far as I can tell, there is no report out against Object.assign for prototype pollution.

Another way to look at it is to view some slight variants of the snyk demonstration above for mergeDeepRight and mapObjIndexed. In each of these cases, the vulnerability is not found.

There’s a reason these exploits are usually mentioned in combination of path-setting or deep merging and that they usually involve JSON.parse: The usage of JSON.parse allows the code to create the proto node without actually updating the prototype directly. And then many deep-merge and path-setting functions work by mutating their input parameter with the values supplied. In combination, that means that these tools will eventually do a deep set of someObject .proto .isAdmin, and since a plain object supplied, someObject .proto will be Object .prototype, which is then mutated to include isAdmin.

Ramda’s design is different. One of its guarantees is that it never mutates input data. Ramda’s equivalent of a deep set, assocPath, does not add properties to your object. Instead it returns a new object, with all properties copied over from the old one and with your new property added or altered on this clone. The various merge functions operate similarly.

That means there is a wall between the data you’re trying to insert and the underlying Object prototype. It’s not clear if this wall is unbreachable, but it’s more difficult than in the jQuery or lodash cases, where immutability is not the default. While avoiding such an exploit was not a reason for Ramda’s immutable design, it is a good demonstration of the advantages it offers.

So this vulnerability report was based on an invalid GitHub issue. The Ramda team will close that issue soon. In fact, it was only because of this report that it’s been left open.

Can the Veracode team remove this entry from its database?

Follow-up

The history above was a dry recital of the order of events, but it has been a very frustrating process. Would it be possible for Veracode’s online reports to either directly include or offer a link to supporting evidence? Here if the report linked back to the GitHub issue or supplied a proof-of-concept of the vulnerability, the Ramda team could have responded to this much more effectively. Or, if that’s not appropriate because they are reported before the team has a chance to work on the issue, could the report offer a “Challenge this finding” link with a way for tool maintainers to do what I’ve done?