jquery PCI Compliance vulnerability CVE-2015-9251

[mitcht]

Preconditions

1. Magento 2.x or Magento 1.9.x

Steps to reproduce

1. Run a PCI compliance scan. Scan was provided by https://www.trustwave.com/

Expected result

1. Pass

Actual result

1. Fail

This vulnerability applies to Magento 2.x and Magento 1.x

jQuery is vulnerable to Cross-site Scripting (XSS) attacks when a cross-domain Asynchronous JavaScript and Extensible Markup Language (AJAX) Request is performed without the dataType option, causing text/javascript responses to be executed. This finding is based on version information which may not have been updated by previously installed patches (e.g., Red Hat "back ports"). All Cross-Site Scripting vulnerabilities are considered non-compliant by PCI.

CVE: CVE-2015-9251 http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-9251
NVD: CVE-2015-9251
CVSSv2: AV:N/AC:M/Au:N/C:N/I:P/A:N
Service: http
Application: nginx:nginx
Reference:
jquery/jquery#2432
https://snyk.io/vuln/npm:jquery:20150627

Evidence:
Match: '1.12.4' is less than '3.0.0'

Remediation:
Upgrade jquery to version 3.0.0 or higher.

[DanielRuf]

jQuery 1 and 3 are different. Also this sounds very generic.

The solution is to pass the data-type in .ajax/get/post calls.

[mitcht]

Not sure about how this is generic... I just know that I can't go in and replace the jQuery without breaking the deployment, since its making a change to core magento files (jquery). Yes, I could do some bash shell trickery, or maybe something in local.xml and use a module, but that seems like such a workaround / unreliable.

[DanielRuf]

Reference: jquery/jquery#2432

Still, jQuery 1 can not be easily replaced by jQuery 3.

Can you point us to specific files which are jQuery 1.x or jQuery 2.x so we can migrate the core and other modules?

[DanielRuf]

Generic in means of: there is a CVE (with low and medium scores) that can be misused but there is not yet a reference to a specific line or lines of code.

Also if you feel like there is a vulnerability in Magento (XSS in this case) which can be actually abused and there is a proof (please don't post it publicly) then inform the security team of Magento.

It highly depends on the usage of jQuery in Magento. Also no user can change the generated JS code. We might just have to check if all XHR requests done by jQuery set the proper data type to prevent the processing by eval.

[mitcht]

lib/web/jquery/jquery.min.js looks like it is at 1.12.x at the moment. I’m on my phone I’ll have to do a more exhaustive search Monday when I’m back to work.

[DanielRuf]

https://github.com/magento/magento2/blob/2.2-develop/lib/web/jquery.js

But replacing it with jQuery 2 or 3 will break things for sure.

[mitcht]

@daniel many pci scanners utilize the existence of the outdated .js file as a flag for failure. Simply loading it is enough to trigger the scan to detect it whether or not magento is using the compromised functions. Anything that causes a Medium to High can be considered a failure.

[DanielRuf]

Sure, but upgrading will be an epic (multiple issues) and not be a simple "fix".

[mitcht]

I don’t have access to anything to organize an epic. I’m used to setting them up in Jira, so I probably could at least identify all the affected files and create tickets for all of them, or at least their modules.

[DanielRuf]

This would be helpful. And probably one extra ticket which is linked from the others where we can see the current state (open / closed).

[hostep]

This is just FYI, but it looks like there is already some action going on for upgrading jQuery to v3 and adding support to other libraries for that version, just search through the PR's: https://github.com/magento/magento2/pulls?utf8=%E2%9C%93&q=is%3Apr+jquery

[mitcht]

Where do these tickets go? Where’s the ticket system. I’m not a project team developer with access like that.

[DanielRuf]

@mitcht these are normal pull requests which reference normal issues of magento/magento2 afaik. Or what exactly do you mean?

[kirmorozov]

I'm pushing this one forward: #14637
Upgrade to 3.3.1 is really easy. 2.3 will be ready for drop-in replacement.
2.4 will go with jQuery 3.

[DanielRuf]

Do we have a backport for 2.2-develop?

[mitcht]

Jquery is consumed mostly by the RWD theme. It Could be used all the way to 1.9

[kirmorozov]

@DanielRuf @mitcht Not sure about backporting so far. It has backward-incompatible changes.
jQuery 3 removed some methods.
@okorshenko Knows much more about it.
It seems to be an effort to update all the way to 1.10.x because of incompatibility.

[cadencelabs-master]

Can existing versions not be patched by adding the proper ajaxPrefilter to the end of the jQuery file? This should emulate the fix for the CVE - users will still need to dispute / false positive on PCI scans.

// Prevent auto-execution of scripts when no explicit dataType was provided (See gh-2432)
jQuery.ajaxPrefilter( function( s ) {
    if ( s.crossDomain ) {
        s.contents.script = false;
    }
}

After adding this patch, running the XSS check in the console turns up that it's no longer vulnerable:
jQuery.get('https://sakurity.com/jqueryxss');

[DrewML]

Hey all. I'd like to see us land a fix for this to mitigate the current risks. Some comments:

1. We're not quite ready to upgrade to jQuery 3.x yet. That upgrade will likely break a decent number of custom themes and extensions, and we'd like to plan a migration strategy to eliminate as much pain as possible.

2. We should avoid getting into the habit of modifying any vendored files, as it becomes easy to lose custom changes during an upgrade.

3. We should patch jQuery from an external file, and do the minimal patch necessary. Since we already use app/code/Magento/Theme/view/base/requirejs-config.js as a place to configure jQuery (call to $.noConflict), it seems like a natural location to include more jQuery init code.

I've created a gist with the fix I'd like to see contributed. Do we have any volunteers to open the PR and carry this through to the finish line?

[kirmorozov]

@DrewML Oh come one, you told me that back in Milan, in April.
PR for jQuery 3 #15531 is open almost 3 months.
There are 3 patches in total to make Magento compatible with jQuery 3, all mentioned as part of #13685

P.S. 3 months is enough time for vendors to patch code, if you give notice.
Production systems will be updated 3-6 months after major release after main wave of bugfixes.

[DrewML]

@kirmorozov I'm not strictly against an eventual upgrade to jQuery 3, but for a security fix I want the minimal amount of changes necessary so it can be shoved into a release and moved along.

I'm not involved in the current processing of those 3 PRs you have mentioned, but it's likely there are higher priority items that are being addressed at the moment.

[cadencelabs-master]

@DrewML I like your idea - only concern would be that the preFilter code should run immediately after jQuery is included without the possibility of any other code running before it. I assume you've done the research to ensure that base file is the right location?

[DrewML]

@cadencelabs-master Yeah, so the order of execution is roughly:

1. require.js runtime loads (sync)
2. mixins.js loads (sync)
3. requirejs-config.js loads (sync)
   a. First call to require.config() in requirejs-config.js executes (which, because of the merging, will be the theme module's requirejs-config.js first)

Everything happens synchronously so it should be the correct order unless something changes about the merging logic.

You do make a good point, though. It might be better if we can make it obvious in the code that execution order is important. We could map requests for jQuery to jquery-init.js, with the following definition:

// jquery-init.js
define(['jquery'], function ($) {
   $.noConflict();
   // XSS patch code
   return $;
});

The only painful parts there will be setting up the RequireJS config in such a way that:

1. require() and define() calls that pull files from the jquery/ directory don't get re-written to jquery-init/
2. That jquery-init.js doesn't create an unresolvable circular dependency by importing jquery

[DanielRuf]

I thought there was no upstream patch? Also wouldn't break this some things?

And I guess this should be done with patch, patch-package or any other patch solution - these tools are meant for this.

This here is just some "workaround" to fix it somehow.
And didn't we change jquery.js so this vendored is already different from the original one?

[DanielRuf]

Also I think this can only land in 2.3 as it will break many things.

[DrewML]

Appreciate the input, @DanielRuf.

I thought there was no upstream patch

There is not. However, the CVE is due to bad defaults. A "fix" is to correct those defaults so you have to explicitly opt-in to dangerous behavior - this is what the snippet above addresses.

Also wouldn't break this some things

It will break any code that is making a request with $.ajax (or its friend $.get) for an asset with a content type of text/javascript or text/ecmascript from a different hostname from the document, and expecting that code to be evaluated.

And I guess this should be done with patch, patch-package or any other patch solution - these tools are meant for this.

Can't speak to patch, but patch-package is typically used (my personal experience, anyway) when there is code that can't be monkey-patched or modified externally, and requires a hard edit of the source code for a lib. In this case, jQuery provides the necessary hooks. Also worth noting this isn't much of a monkey patch, since it's a configurable setting exposed by jQuery.

And didn't we change jquery.js so this vendored is already different from the original one?

I did a diff (ignoring whitespace) and the lib was identical to the official copy. Looks like we adjusted formatting

[ghost]

@mitcht, thank you for your report.
We've acknowledged the issue and added to our backlog.

[omiroshnichenko]

Issue already fixed in 2.3
86fdea3#diff-f20d0f384ca4b6aeab9458e178935ecb and will be available in 2.2-develop after 2.2.7 release.

[mitcht]

Thanks for the concern for all of us 1.x users.

[ihor-sviziev]

Issue already fixed in 2.2.7 release.
Related commits:

• ebbdf7d