- Notifications
You must be signed in to change notification settings - Fork20.6k
jQuery.parseHTML: Mitigate XSS vulnerability#1506
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 hasto set a different context deliberately for this issue to occur.See also GitHub issue#1505.
I haven't managed to get this also running in IE8 and below yet. Basically, it should look something like varaxContext,defaultContext=document;if(jQuery.isFunction(document.implementation.createHTMLDocument)){defaultContext=document.implementation.createHTMLDocument();}elseif("ActiveXObject"inwindow){axContext=newActiveXObject("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. |
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).
Note: I'll update this PR as soon as#1505 is merged into 2.x |
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. |
Just to give it a ticket, this is#11974 which can be reopened. So to be clear, the most common internal use of 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. |
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. |
Per thecomment above I think this is actually part of the solution forgh-1747 because it prevents some inline code from running before the HTML is appended to the document. The two PRs could be merged though. |
@dmethvin misinterpreted your comment, now i see the conundrum, so we have two problems: So Sounds good? |
#1505 is for jQuery 2.x, the proposed solution works fine there. Regarding this PR, IE8 could use anActiveX 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 The Active X component would require some additional work and could be added to a later patch release. |
I think the quick incomplete solution would be fine. That leaves the problem unsolved for IE8, but many problems are unsolved for IE8. 😈 |
Fixed execution with activex component, but it was throwing in indeed unexpected places, given that 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 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of being run inline, this check should be executed once with the results captured as a support property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, please undo the jshint exception and move the ternary?
and:
to the end of the preceding lines (seehttp://contribute.jquery.org/style-guide/js/#multi-line-statements ).
@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! |
Closed by828a718 |
Wow, great. Thank you! 👍 |
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 hasto set a different context deliberately for this issue to occur.
See also GitHub issue#1505.
EDIT: Agreed to the CLA.