Skip to content
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

jQuery.parseHTML: Mitigate XSS vulnerability #1506

Closed
wants to merge 2 commits into
base: 1.x-master
from

Conversation

Projects
None yet
6 participants
@fhemberger
Copy link
Contributor

fhemberger commented Jan 31, 2014

Scripts passed in event attributes are executed in parseHTML immediately,
without any possibility for the user to intervene:

jQuery.parseHTML('<img src=x onerror=alert(1)>');

To mitigate this vulnerability document.implementation.createHTMLDocument()
should be used as standard context instead of document. Now the user has
to set a different context deliberately for this issue to occur.

See also GitHub issue #1505.

EDIT: Agreed to the CLA.

jQuery.parseHTML: Mitigate XSS vulnerability
Scripts passed in event attributes are executed in `parseHTML` immediately,
without any possibility for the user to intervene:

`jQuery.parseHTML('<img src=x onerror=alert(1)>');`

To mitigate this vulnerability `document.implementation.createHTMLDocument()`
should be used as standard context instead of `document`. Now the user has
to set a different context deliberately for this issue to occur.

See also GitHub issue #1505.
@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Jan 31, 2014

I haven't managed to get this also running in IE8 and below yet. Basically, it should look something like

var axContext,
    defaultContext = document;

if ( jQuery.isFunction( document.implementation.createHTMLDocument ) ) {
    defaultContext = document.implementation.createHTMLDocument();
} else if ( "ActiveXObject" in window ) {
    axContext = new ActiveXObject( "htmlfile" );
    axContext.write();
    axContext.close();
    defaultContext = axContext.body;
}
context = context || defaultContext || document;

But using the ActiveX context breaks quite a bunch of tests. But as this patch still helps for most other browsers, it's still an improvement.

jQuery.parseHTML: Make XSS test conditional
The current implementation relies on `document.implementation.createHTMLDocument`,
which IE8 and below are lacking. It also may not be available in older Android browsers (<= 2.x).
@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Feb 2, 2014

Note: I'll update this PR as soon as #1505 is merged into 2.x

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Feb 7, 2014

I think the problem with IE<=8 is that you end up with nodes from a foreign document that can't be added directly to the target document. If you were writing a plugin to fix this you could traverse and sanitize the elements in the foreign document, then serialize the sanitized tree to an HTML string and parse that in the target document.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Feb 7, 2014

Just to give it a ticket, this is #11974 which can be reopened.

So to be clear, the most common internal use of .parseHTML() will mean that the next step will be to append the HTML to the document, triggering the inline handlers. However, we have an open ticket #14228 to provide a hook point for sanitizing input, and perhaps we could integrate a strategy into that.

Again, I suspect a complete solution is a bit heavy for including into core but would like to make it easy for someone to do the work in a plugin.

@dmethvin dmethvin added this to the 1.12/2.2 milestone Feb 7, 2014

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Feb 7, 2014

That's why I called the PR "mitigate" not "fix". ;) I think it's a good start to address the issue at this point. I won't put more effort in fixing this for IE<=8 – this patch improves the handling for ~90% of the users, for the rest it's simply the same as before.

A plug-in seems like a good idea – would parseHTML be the only function to hook into? It would basically use the changes proposed in #1508, but as the user has to install this separately, it won't have unwanted side effects in core.

@mgol mgol force-pushed the jquery:1.x-master branch from 3fed4b3 to 73c1cea Nov 8, 2014

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Dec 3, 2014

Since this is the one of the last 1.x-master PR's and it seems solution wouldn't cover all browsers but fix for #1747 will, so i would like to close it and #1505 too.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Dec 3, 2014

Per the comment above I think this is actually part of the solution for gh-1747 because it prevents some inline code from running before the HTML is appended to the document. The two PRs could be merged though.

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Dec 3, 2014

@dmethvin misinterpreted your comment, now i see the conundrum, so we have two problems: createHTMLDocument doesn't exist in IE8 and in old android otherwise we good.

So createHTMLDocument does exist in android 2.3 and in IE8 we could use an active X object.

Sounds good?

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Dec 3, 2014

#1505 is for jQuery 2.x, the proposed solution works fine there.

Regarding this PR, IE8 could use an ActiveX component for filtering, but it seems to break some stuff in jQuery I wasn't able to figure out.

A quick (although not complete) solution would be to use document.implementation.createHTMLDocument where it's available, otherwise falling back to the current behavior. Although this would leave IE8 and below still vulnerable to those kind of injections, the issue would be fixed for all other browsers.

The Active X component would require some additional work and could be added to a later patch release.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Dec 3, 2014

I think the quick incomplete solution would be fine. That leaves the problem unsolved for IE8, but many problems are unsolved for IE8. 😈

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Dec 8, 2014

Fixed execution with activex component, but it was throwing in indeed unexpected places, given that parseHTML is one is the basis for many code paths i think risks are pretty high, so let's IE8 problem be unresolved.

Will try to take time for this and #1505 to land tomorrow

scripts = !keepScripts && [],
// document.implementation stops scripts or inline event handlers from being executed immediately
/* jshint laxbreak: true */
defaultContext = jQuery.isFunction( document.implementation.createHTMLDocument )

This comment has been minimized.

@gibson042

gibson042 Dec 8, 2014

Member

Instead of being run inline, this check should be executed once with the results captured as a support property.

This comment has been minimized.

@gibson042

gibson042 Dec 8, 2014

Member

Also, please undo the jshint exception and move the ternary ? and : to the end of the preceding lines (see http://contribute.jquery.org/style-guide/js/#multi-line-statements ).

// This XSS test is optional, as it will only pass when `document.implementation.createHTMLDocument`
// is implemented. It is not available in IE8 and below and might not be for older Android
// browsers (<= 2.x) either.
if ( jQuery.isFunction( document.implementation.createHTMLDocument ) ) {

This comment has been minimized.

@gibson042

gibson042 Dec 8, 2014

Member

We shouldn't be using the jQuery under test to make decisions about what to test. Either switch to supportjQuery or just drop the isFunction and check for existence of document.implementation.createHTMLDocument.

Also, this would make a great use of the newly-available QUnit.skip.

This comment has been minimized.

@dmethvin

dmethvin Dec 8, 2014

Member

Also, this would make a great use of the newly-available QUnit.skip.

Ooh yeah, i forgot that was being added!

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Dec 8, 2014

@fhemberger when @gibson042 comments would be taken into the account, we're happy to merge it :-).

btw, thank you for all your patience with this!

timmywil added a commit that referenced this pull request Dec 10, 2014

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Dec 10, 2014

Closed by 828a718

@timmywil timmywil closed this Dec 10, 2014

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Dec 10, 2014

Wow, great. Thank you! 👍

@mgol

This comment has been minimized.

Copy link
Member

mgol commented Mar 6, 2016

This PR was reverted on 1.12-stable so I'm removing the milestone. We'll still try to fix the issues that popped up in 3.0.0 and not revert this patch there so I'm keeping the milestone on #1505.

(the issue to resolve is #2941).

@mgol mgol removed this from the 3.0.0 milestone Mar 6, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.