Security Fix for Prototype Pollution in cytoscape.js

[zpbrent]

📊 Metadata *

The setMap in cytoscape is subject to prototype pollution due to the recursely copy obj[key] to obj in the code. This vulnerble API is exported to be called by the end users throgh cytoscape constructor. This vulnerability allows modification of the Object prototype. If an attacker can control part of the structure passed to this function, they could add or modify an existing property. Possibly leading to many kinds of attacks such as the denial-of-service, checking bypass, or potentially code execution.

Bounty URL: https://www.huntr.dev/bounties/1-npm-cytoscape/

⚙️ Description *

Sanitize the key before the code obj = obj[ key ];.

💻 Technical Description *

if (key !== '__proto__' && key !== 'constructor' && key !== 'prototype') {
        obj = obj[ key ];
      }

🐛 Proof of Concept (PoC) *

// PoC.js
var cytoscape=require('cytoscape');
console.log('Before: ' + {}.polluted);//Before: undefined
cytoscape('__proto__','polluted','HACKED');
console.log('After: ' + {}.polluted); //After: HACKED

🔥 Proof of Fix (PoF) *

// PoF.js
var cytoscape=require('cytoscape');
console.log('Before: ' + {}.polluted);//Before: undefined
cytoscape('__proto__','polluted','HACKED');
console.log('After: ' + {}.polluted); //After: undefined

👍 User Acceptance Testing (UAT)

[huntr-helper]

👋 Hello, @maxkfranz. @zpbrent has opened a PR to us with a fix for a potential vulnerability in your repository. To view the vulnerability, please refer to the bounty URL in the first comment, above. If you want this fix in your repository, a PR will automatically open once you comment:

@huntr-helper - LGTM

---

☎️ Need further support?

Come and join us on our community Discord!

---

❓@maxkfranz - want more fixes like this?

Copy this snippet into your README.md for more vulnerability fixes in the future:

[![huntr](https://cdn.huntr.dev/huntr_security_badge_mono.svg)](https://huntr.dev)

[maxkfranz]

This is a good catch. Here are some points to consider:

(1) Only extensions would use the registration function, cytoscape('extensionName' ...). The risk of this being exploited is very low, as user-entered data would never go in this function. There could be a malicious extension, but a malicious extension could run arbitrary code anyway.

(2) Whereas the setMap() function is a low-level, general utility function, your vulnerability is specific to the higher-level extension API. It would be more robust and less invasive to address the issue at that level.

[zpbrent]

This is a good catch. Here are some points to consider:

(1) Only extensions would use the registration function, cytoscape('extensionName' ...). The risk of this being exploited is very low, as user-entered data would never go in this function. There could be a malicious extension, but a malicious extension could run arbitrary code anyway.

(2) Whereas the setMap() function is a low-level, general utility function, your vulnerability is specific to the higher-level extension API. It would be more robust and less invasive to address the issue at that level.

Hey @maxkfranz thanks for your advise.

For your first point, yes, I agree with you that it is not quite common the user-entered data can reach the setExtension function since it is widely used to register a new extension for use. But is it possible to have a scenario that some third parties use cytoscape to enable the end-users to register pre-defined extensions dynamically by calling cytoscape('extensionName' ...) directly, and if the third parties do not understasnd the potential prototype pollution, they may let the users to choose extensionName through a pre-defined list on the web page. In this case, the vulnerability becomes exploitable.

For your second point, I have opened another PR at #2 to avoid this vulnerability at the high-level setExtension function to make the fix more robust and less invasive. Can you help to take a review? If you are fine with that fix, please comment @huntr-helper - LGTM at #2, or if you have more comments, please also reply on that PR. Thanks.

Note that, the high-level fix at setExtension cannot completely avoid the prototype pollution in setMap, in case the cytoscape is upgraded and unfortunately exports setMap to the end users in future.