jQuery.parseHTML: Mitigate XSS vulnerability #1505

fhemberger · 2014-01-31T13:50:05Z

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 jQuery ticket #13921.

EDIT: Agreed to the CLA.

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 jQuery ticket #13921.

dmethvin · 2014-01-31T15:48:34Z

I like the sentiment behind this! I just put together some wording to clarify the problem at jquery/api.jquery.com@6137d67 but am not really happy about it.

Do you know the range of browser support? It looks like this may be usable on all browsers that jQuery 2.x supports but not jQuery 1.x: http://stackoverflow.com/questions/18003264/document-implementation-createhtmldocument-browser-support

Mainly I'd be worried about Android, especially Android 2.3.

fhemberger · 2014-01-31T15:56:33Z

I'll modify this PR to check for support with jQuery.isFunction() first, falling back to document. Just to be sure. Also I'm already adding this to the 1.x branch (PR follows in a few minutes 😃 ).

…MLDocument

fhemberger · 2014-01-31T16:28:15Z

I'm now checking for document.implementation.createHTMLDocument first, so it should be fine.

The current implementation relies on `document.implementation.createHTMLDocument`, which may not be available in older Android browsers (<= 2.x).

gibson042 · 2014-02-02T18:50:24Z

src/core/parseHTML.js

+		defaultContext = jQuery.isFunction( document.implementation.createHTMLDocument )
+			? document.implementation.createHTMLDocument()
+			: document;
+		/* jshint laxbreak: false */


Our style guide calls for the ? and : at end of line (i.e., no laxbreak).

gibson042 · 2014-02-02T18:59:21Z

This seems really nice, especially if we can drop the fallback in 2.x. Thanks, @fhemberger!

dmethvin · 2014-02-07T02:07:38Z

See the comment here.

fhemberger · 2014-02-07T07:02:44Z

See the comment here.

fhemberger · 2014-02-20T14:50:21Z

Any news on this issue? Can it be merged in it's current state?

dmethvin · 2014-02-20T14:56:07Z

@fhemberger This will go into 1.12/2.2 so we don't want to land it in master if a 1.11.1/2.1.1 is going to come out soon. We haven't forgotten about it though!

fhemberger · 2014-02-20T15:47:11Z

@dmethvin Great, thanks.

We're already working on a separate plug-in for strict security-related DOM filtering, which will work nicely together with this patch.

mgol · 2014-02-23T21:35:17Z

A few remarks regarding the commit:

Second line must always be empty
Commit message line 3 too long: 82 characters, only 80 allowed.
Please don't add new lines longer than 100 characters (we'll going to enforce the limit in the near future once we fix all the code).

gibson042 · 2014-12-08T12:21:31Z

src/core/parseHTML.js

@@ -15,7 +15,10 @@ jQuery.parseHTML = function( data, context, keepScripts ) {
 		keepScripts = context;
 		context = false;
 	}
-	context = context || document;
+	// document.implementation stops scripts or inline event handlers from being executed immediately
+	context = context || ( jQuery.isFunction( document.implementation.createHTMLDocument ) ?


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

Do you already have a place in one of the files for those kind of checks?

Several, actually.

This particular test could go in the base var/support.js, but if not then probably in a new core/support.js similar to manipulation/support.js.

I have a slight preference for the former, but would opt for the latter if it meant a smaller gzipped output file.

Fixes #14746

Since getter was removed in fdd78fa there is no longer a need to wrap option element in order to get its value Fixes #14756

More to come later. (cherry picked from commit f6f8848)

Ref 6748ba3

Ref 3c13f4c

@TheDistantSea

Thanks to @TheDistantSea for the report! Fixes gh-1790 Closes gh-1643

Closes gh-1685

Some environments do not support data-uri in "src" attribute of script element. Mitigate it with equality assertion Ref a467f86

The hook is still defined; not using it could cause issues in IE<11. Also, IE10 no longer throws when value not set but it still doesn't trim the value. IE11 has all those issues fixed; support comments are updated. Fixes gh-1902 Closes gh-1901

Closes gh-1510

See #1505 (comment)

fhemberger · 2014-12-09T16:09:25Z

Waaah! Rebasing from master added all commits … relevant is only 6a4672c. If you want, I can send a new, clean PR which should be easier to merge.

markelog · 2014-12-09T16:22:37Z

You probably didn't rebase but merge it.

if you want, I can send a new, clean PR which should be easier to merge.

That might be the best way to do it

gibson042 · 2014-12-09T16:26:53Z

You could also git rebase -i master to remove all commits other than yours and then git push -f origin parsehtml_2.x to restore this PR.

mgol · 2014-12-10T00:07:16Z

Unfortunately, we had to back it out for now because of Safari 8 bugs: b779831.

fhemberger · 2014-12-10T08:19:50Z

This is the related WebKit bug report: https://bugs.webkit.org/show_bug.cgi?id=137337

mgol · 2014-12-10T10:09:00Z

@fhemberger

This is the related WebKit bug report: https://bugs.webkit.org/show_bug.cgi?id=137337

Thanks a lot for finding it!

This seems hard to patch on our side (at least without adding a lot of code) and using document.implementation.createHTMLDocument('') everywhere except Safari 8 would be quite weird as it's a security-related feature; it could make developers trust their inputs a little too much and put Safari 8 users at risk.

What do you all think? Judging by the fact that the bug is still open I don't see a quick solution on Safari side.

fhemberger · 2014-12-10T12:44:39Z

Well, you could detect this specific bug using the code of the test case:

var doc = document.implementation.createHTMLDocument('');
var bodyEl = doc.documentElement.lastChild;
bodyEl.innerHTML = '<form></form><form></form>';
var isSafariBug = (bodyEl.childElementCount === 1);

Add this check to the general support methods and if it returns true, just fall back to document. It's not a nice solution but it would be a solution nonetheless.

dmethvin · 2014-12-10T12:58:39Z

Oh 💩.

Well, here for the second time in a week we are faced with a crippling Safari bug and no way to know what Apple may decide what to do. How about feature-detect this similar to what @fhemberger describes and skip using the technique in Safari as a result? Then we can simply warn people that using Safari is less safe.

fhemberger · 2014-12-10T13:06:34Z

Yes, I think that's the best way to deal with this right now … then the changes of this PR could be backported 1:1 for the 1.x branch (as we don't have any fallback there for IE8 as well).

Close gh-1505

The document.implementation.createHTMLDocument("") method creates inert documents which is good but using it has introduced issues around anchor elements href property not resolving according to the current document. Because of that, this patch is getting backed out on 1.x/2.x branches. Refs cfe468f Refs gh-1505 Fixes gh-2941

mgol · 2016-03-02T11:40:49Z

This was originally backported to 1.12/2.2 but due to #2941 we're backing out the patch on those branches (as they wen't meant to be non-breaking releases and there may always be more issues around that); we'll try to fix the issue for 3.0.

The document.implementation.createHTMLDocument("") method creates inert documents which is good but using it has introduced issues around anchor elements href property not resolving according to the current document. Because of that, this patch is getting backed out on 1.x/2.x branches. (cherry-picked from c5c3073) Refs cfe468f Refs gh-1505 Fixes gh-2941

fhemberger mentioned this pull request Jan 31, 2014

jQuery.parseHTML: Mitigate XSS vulnerability #1506

Closed

jQuery.parseHTML: Check existance of document.implementation.createHT…

b617d19

…MLDocument

fhemberger mentioned this pull request Jan 31, 2014

Security: Mitigate DOM XSS vulnerability HubSpot/youmightnotneedjquery#21

Merged

jQuery.parseHTML: Make XSS test conditional

0bc190c

The current implementation relies on `document.implementation.createHTMLDocument`, which may not be available in older Android browsers (<= 2.x).

fhemberger mentioned this pull request Feb 2, 2014

jQuery.parseHTML: Disable inline event handlers when removing scripts #1508

Closed

gibson042 reviewed Feb 2, 2014
View reviewed changes

Remove jshint:laxbeak, use Globals.register for test

b78b93b

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

fhemberger mentioned this pull request Nov 3, 2014

XSS vulnerability in demo alohaeditor/Aloha-Editor#1270

Closed

gibson042 reviewed Dec 8, 2014
View reviewed changes

rwaldron and others added 7 commits December 9, 2014 17:07

Core: Remove special case try/catch in isPlainObject

b32dd62

Fixes #14746

Build: Switch to explicit dependencies versions in bower.json

326677d

Attrs: Simplify an option hook

5d06053

Since getter was removed in fdd78fa there is no longer a need to wrap option element in order to get its value Fixes #14756

Selector: update Sizzle

5d58724

Build: Upgrade QUnit to 1.14.0

f4edd4d

Docs: Change raises to throws in README test assertions

d99ca61

Support: Document box-sizing was unprefixed in Firefox 29

234ec01

gibson042 and others added 9 commits December 9, 2014 17:07

Tests: Minor updates for QUnit 1.16 compatibility

e0aae1f

More to come later. (cherry picked from commit f6f8848)

Tests: Accommodate page changes from the QUnit HTML reporter

1c529c6

Ref 6748ba3

Tests: Tilt at a few style guide windmills

f8b8d2c

Ref 3c13f4c

Callbacks: Disabling a callback should prevent firing

6530c50

Thanks to @TheDistantSea for the report! Fixes gh-1790 Closes gh-1643

Event: Fully clean up events in unit test

d11f8be

Closes gh-1685

Manipulation: improve test for data-URI

bd60848

Some environments do not support data-uri in "src" attribute of script element. Mitigate it with equality assertion Ref a467f86

Core: Simplify and speed up .each

adec44f

Closes gh-1510

Core: Remove inline check for createHTMLDocument, add to 'support'

6a4672c

See #1505 (comment)

timmywil closed this in 58c2460 Dec 9, 2014

dmethvin reopened this Dec 10, 2014

timmywil self-assigned this Dec 10, 2014

timmywil closed this in cfe468f Dec 10, 2014

markelog pushed a commit that referenced this pull request Nov 10, 2015

Core: use document.implemenation.createHTMLDocument in jQuery.parseHTML

d23a95f

Close gh-1505

timmywil added a commit that referenced this pull request Nov 10, 2015

Core: re-introduce createHTMLDocument in parseHTML; Safari 8 left out

ac1e6ea

Close gh-1505

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jQuery.parseHTML: Mitigate XSS vulnerability #1505

jQuery.parseHTML: Mitigate XSS vulnerability #1505

fhemberger commented Jan 31, 2014

dmethvin commented Jan 31, 2014

fhemberger commented Jan 31, 2014

fhemberger commented Jan 31, 2014

gibson042 Feb 2, 2014

gibson042 commented Feb 2, 2014

dmethvin commented Feb 7, 2014

fhemberger commented Feb 7, 2014

fhemberger commented Feb 20, 2014

dmethvin commented Feb 20, 2014

fhemberger commented Feb 20, 2014

mgol commented Feb 23, 2014

gibson042 Dec 8, 2014

fhemberger Dec 9, 2014

gibson042 Dec 9, 2014

fhemberger commented Dec 9, 2014

markelog commented Dec 9, 2014

gibson042 commented Dec 9, 2014

mgol commented Dec 10, 2014

fhemberger commented Dec 10, 2014

mgol commented Dec 10, 2014

fhemberger commented Dec 10, 2014

dmethvin commented Dec 10, 2014

fhemberger commented Dec 10, 2014

mgol commented Mar 2, 2016

jQuery.parseHTML: Mitigate XSS vulnerability #1505

jQuery.parseHTML: Mitigate XSS vulnerability #1505

Conversation

fhemberger commented Jan 31, 2014

dmethvin commented Jan 31, 2014

fhemberger commented Jan 31, 2014

fhemberger commented Jan 31, 2014

gibson042 Feb 2, 2014

Choose a reason for hiding this comment

gibson042 commented Feb 2, 2014

dmethvin commented Feb 7, 2014

fhemberger commented Feb 7, 2014

fhemberger commented Feb 20, 2014

dmethvin commented Feb 20, 2014

fhemberger commented Feb 20, 2014

mgol commented Feb 23, 2014

gibson042 Dec 8, 2014

Choose a reason for hiding this comment

fhemberger Dec 9, 2014

Choose a reason for hiding this comment

gibson042 Dec 9, 2014

Choose a reason for hiding this comment

fhemberger commented Dec 9, 2014

markelog commented Dec 9, 2014

gibson042 commented Dec 9, 2014

mgol commented Dec 10, 2014

fhemberger commented Dec 10, 2014

mgol commented Dec 10, 2014

fhemberger commented Dec 10, 2014

dmethvin commented Dec 10, 2014

fhemberger commented Dec 10, 2014

mgol commented Mar 2, 2016