Patch the prototype pollution vulnerability in jQuery < 3.4.0 (CVE-2019-11358)

[DanielRuf]

This patches the prototype pollution vulnerability in jQuery < 3.4.0.
See https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/

Minor vulnerability fix: Object.prototype pollution
jQuery 3.4.0 includes a fix for some unintended behavior when using jQuery.extend(true, {}, ...). If an unsanitized source object contained an enumerable proto property, it could extend the native Object.prototype. This fix is included in jQuery 3.4.0, but patch diffs exist to patch previous jQuery versions.

Example
jQuery.extend(true, {},
JSON.parse('{"proto": {"test": true}}')
);
console.log( "test" in {} ); // true
Note that while jQuery does its best to protect users from security vulnerabilities, jQuery is a DOM manipulation library that will generally do what you tell it to do. In this case, the behavior was likely unexpected, so jQuery.extend will no longer write any properties named proto. But guards such as this one are not replacements for good security practices such as user input sanitization.

Patches: https://github.com/DanielRuf/snyk-js-jquery-174006?files=1

Original report: https://app.snyk.io/vuln/SNYK-JS-JQUERY-174006

Description (*)

Fixed Issues (if relevant)

1. magento/magento2#<issue_number>: Issue title
2. ...

Manual testing scenarios (*)

1. ...
2. ...

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[m2-assistant]

Hi @DanielRuf. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[orlangur]

@DanielRuf it is not so easy to review a minified file, you know :)

Please provide exact steps so that resulting file can be verified.

[DanielRuf]

Hi,

I have only changed the part directly before ,g!==c&&(j&&c&&(n.isPlainObject(c).

The blog article by Snyk contains some examples: https://snyk.io/blog/after-three-years-of-silence-a-new-jquery-prototype-pollution-vulnerability-emerges-once-again/

[DanielRuf]

By using jsbeautify or any other beautifier it should be possible to see the single change.

The patch files are also linked by the jQuery team in their blog post about the 3.4.0 release.

[orlangur]

@DanielRuf thanks, this should work as well. I was thinking of applying UglifyJS or whatever tool you used the same way you did it. I didn't think you edited minified file directly :)

Should we specify additional patch in comment maybe?

[DanielRuf]

Should we specify additional patch in comment maybe?

Can you specify what you mean?
Changing the first line in the minified file / add a another header comment?

[orlangur]

Another header comment which would be strippable in production mode (copyright-containing comments are probably not strippable).

[DanielRuf]

Another header comment which would be strippable in production mode (copyright-containing comments are probably not strippable).

Done. Please review the changes.

[orlangur]

@DanielRuf, /*! no exclamation mark in patch comment so that it can be stripped.

Diff LGTM:

-                for (d in e) a = g[d], c = e[d], g !== c && (j && c && (n.isPlainObject(c) || (b = n.isArray(c))) ? (b ? (b = !1, f = a && n.isArray(a) ? a : []) : f = a && n.isPlainObject(a) ? a : {}, g[d] = n.extend(j, f, c)) : void 0 !== c && (g[d] = c));
+                for (d in e) a = g[d], c = e[d], d !== "__proto__" && g !== c && (j && c && (n.isPlainObject(c) || (b = n.isArray(c))) ? (b ? (b = !1, f = a && n.isArray(a) ? a : []) : f = a && n.isPlainObject(a) ? a : {}, g[d] = n.extend(j, f, c)) : void 0 !== c && (g[d] = c));

Please squash into single commit after last change.

[DanielRuf]

Ok, I will change it and squash the commits.

[DanielRuf]

Ok, I will change it and squash the commits.

Done.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-4804 has been created to process this Pull Request

[DanielRuf]

Any update? Would be good to have this fixed and as SUPEE patch or link to my patches at https://github.com/DanielRuf/snyk-js-jquery-174006

[orlangur]

@sdzhepa @stoleksiy @VasylShvorak sorry for pinging directly, could you please proceed with this PR?

[sdzhepa]

Hello
@DanielRuf @orlangur

Thank you for contribution and collaboration!

During testing, I faced an issue.
Could you please have a look and clarify is it issue with a solution or I should use another case for testing?
At this momet, it seems as initial issue is not fixed

Manual testing scenario:

1. Use Examle of code from the description or from article https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/

jQuery.extend(true, {},
  JSON.parse('{"__proto__": {"test": true}}')
);
console.log( "test" in {} ); // true

2. Use it in some js file in Magento.
3. I added it to the "app/code/Magento/Catalog/view/frontend/web/js/product/breadcrumbs.js" file, like

define([
    'jquery',
    'Magento_Theme/js/model/breadcrumb-list'
], function ($, breadcrumbList) {
    'use strict';

    return function (widget) {
$.extend(true, {},
        JSON.parse('{"__proto__": {"test": true}}')
    );
    console.log( "test" in {} ); // true

        $.widget('mage.breadcrumbs', widget, {

3. Cleared all from "var/*" and "pub/static/frontent"
4. Go to Storefront on any product page and open browser console

Result on 2.3-develop branch: true

Result on branch with fix: The same as on 2.3-develop - true

[DanielRuf]

Hi @sdzhepa,

Would be new for me that the patch would not work. I will check it again tomorrow but the replaced part should be correct.

[DanielRuf]

See https://github.com/DanielRuf/snyk-js-jquery-174006/blob/master/jquery-1.12.4.patch and the minified version, these should be the same.

[VladimirZaets]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @VladimirZaets. Thank you for your request. I'm working on Magento instance for you

[DanielRuf]

Your file does not seem to contain the fix.

[DanielRuf]

Seems this is a different jQuery file, see the comment at the top. https://github.com/DanielRuf/magento2/blob/e0cc1c98ebf326183b1e533df52bc97774df8141/lib/web/jquery/jquery.min.js#L1

[DanielRuf]

https://github.com/magento/magento2/search?utf8=✓&q=includes+sizzle&type=

Wait, why do we have jQuery twice?
I will change the unminified file too.

[DanielRuf]

Ok so 98214d7 removed the minified file and changed the imports and ebbccca added the minified file again.

Not sure why, also the move out of the jquery folder was probably not the best idea.

I will patch the other file too.
(Interestingly in our instance the lib/jquery/jquery.min.js file was used so the changes from 98214d7 changed it only in one or a few placss).

We should discuss this in another issue if we should revert the changes and remove the duplicated file(s).

[DanielRuf]

Makes not much sense to move only one file imo.
Anyway, I will add the required change to the unminified file too.

[DanielRuf]

Now it should work also with the unminified version of jQuery.

@sdzhepa @orlangur

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-4804 has been created to process this Pull Request

[VasylShvorak]

✔️ QA passed
Before:

After:

[m2-assistant]

Hi @DanielRuf, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.