Movatterモバイル変換

mgol force-pushed thexss-htmlprefilter branch from9f24c2d to1e45057Compare

May 20, 2020 09:30

mgol commented

src/jqLite.js OutdatedShow resolvedHide resolved

petebacondarwin added component: jqLite severity: security type: bug labels

mgol added this to the1.8.x milestone

koto reviewed

src/jqLite.js

		// the select element.
		if (msie < 10) {
		wrapMap.optgroup = wrapMap.option = [1, '<select multiple="multiple">', '</select>'];
		}

Copy link

Contributor

kotoMay 20, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not sure if you actually need the wrapping at all in modern browsers. At least in Chrome appending the elements directly works, even if, say,td is not undertbody ortable.

In any case, while the current patch fixes the security bug we currently know about, a better way would be to avoid concatenating HTML strings whatsoever. If, for example, you need wrapping, usedocument.createElement andappendChild and append to the newly created one.

Copy link

MemberAuthor

mgolMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not sure if you actually need the wrapping at all in modern browsers.

You do. Seehttps://jsbin.com/cibiwet/edit?html,js,console, it doesn't actually append the element, either in Firefox or in Chrome.

I'm curious how it worked for you, my test case is pretty basic.

If, for example, you need wrapping, usedocument.createElement andappendChild and append to the newly created one.

That would work.

Copy link

Contributor

kotoMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Heh, my bad. I would've sworn this worked last week (I was debugging jqLite, so all calls were inside a fragment too), but indeed it does not (only<option> works), sorry for the confusion.

petebacondarwin reviewed

src/Angular.js OutdatedShow resolvedHide resolved

src/jqLite.js OutdatedShow resolvedHide resolved

src/jqLite.jsShow resolvedHide resolved

test/jqLiteSpec.js

		window.setTimeout(function() {
		expect(window.xss).not.toHaveBeenCalledWith(index);
		donePartial();
		}, 1000);

Copy link

Contributor

petebacondarwinMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is it possible to get a false-positive (i.e. an anti-flake)?
If the timeout completes before the async changes have occurred, it is possible that the test could have failed if the timeout were longer.

I guess that theonerror handler is always called async?

Could we avoid relying upon the timeout being long enough. Perhaps create an additional "real" error that will call a differentonerror handler? Then assume that if this second handler is called but thexss one is not called then we are good... Then we could make theit async and only calldone() once the second real error handler has been called.

Copy link

MemberAuthor

mgolMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Feel free to experiment. I've tried a few things when I prepared a similar patch for jQuery & I wasn't able to achieve something that would not have the delay and that wouldn't suffer from race conditions. Maybe there's something but it might require extensive testing - we definitely don't want to miss the error just because it fired too late.

koto reviewed

src/jqLite.js Outdated

		@@ -215,7 +229,10 @@ function jqLiteBuildFragment(html, context) {
		tmp = fragment.appendChild(context.createElement('div'));
		tag = (TAG_NAME_REGEXP.exec(html) \|\| ['', ''])[1].toLowerCase();
		wrap = wrapMap[tag] \|\| wrapMap._default;

Copy link

Contributor

kotoMay 20, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You could get rid of_default, e.g. like this:

wrap=wrapMap[tag]finalHTML= ...tmp.innerHTML=wrap ?wrap[1]+finalHtml+wrap[2]  :finalHtml;i=wrap ?wrap[0] :0;while(i--){//...

That's a no-op for the security fix, but is shorter - and lets you pipe through the data without any string concatenation (e.g. letting throughTrustedHTML objects that DOMPurify might return).

Copy link

MemberAuthor

mgolMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sound sensible. That said, this would technically be a new feature and we're at the point where we mostly care about security fixes so I wouldn't want to spend too much time on tweaking this code.

Copy link

MemberAuthor

mgolMay 20, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

OK, apparently we have a few days to finish the PR; in that case, I can have a look early next week.

gkalpak approved these changes

test/jqLiteSpec.js

		'<noscript/><img src=url404 onerror=xss(10)>',
		'<noembed><noembed/><img src=url404 onerror=xss(11)>',

		'<option><style></option></select><img src=url404 onerror=xss(12)></style>'

Copy link

Member

gkalpakMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think that the following test cases were useful for jQuery (which used a slightly different regex) but not for jqLite'sXHTML_TAG_REGEXP (i.e. they would pass without the changes in this PR, so they don't add much value):

'<img alt="<x" title="/><img src=url404 onerror=xss(0)>">'
'<img alt="\n<x" title="/>\n<img src=url404 onerror=xss(1)>">'
'<foo" alt="" title="/><img src=url404 onerror=xss(8)>">'
'<img alt="<x" title="" src="/><img src=url404 onerror=xss(9)>">'

(That is because jQuery's tegex would match" as part of the tag name, while jqLite won't.)

Copy link

MemberAuthor

mgolMay 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We’re testing Angular with jQuery as well here. I’d leave them all; the purpose is not to match only what was broken but to prevent future regressions here.

mgol force-pushed thexss-htmlprefilter branch 4 times, most recently from0b474b3 tof21f2d8Compare

May 26, 2020 15:06

Copy link

MemberAuthor

mgol commentedMay 26, 2020•
edited
Loading

@koto I refactored the whole thing to not rely on passing strings toinnerHTML at all except in IE 9 where it's unavoidable - IE 9 doesn't let you to assign'<td></td>' to atr element even if it lies in a correct table structure; it needs the whole HTML up front.

This makes the PR increase the minified size by407 bytes (before gzip) due to duplication but at least it should be more secure in modern browsers.

Please take a look.

petebacondarwin reviewed

Copy link

Contributor

petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

On master theangular.min.js file is 176333 bytes.
This PR appears to make it 176724 (391 byte increase over master).
My two suggestions appear to make it 176544 (211 byte increase over master).

src/jqLite.js OutdatedShow resolvedHide resolved

petebacondarwin reviewed

src/jqLite.jsShow resolvedHide resolved

fix(jqLite): prevent possible XSS due to regex-based HTML replacement

2df43c0

This also splits the wrapping logic to one for modern browsers & one for IE 9as IE 9 has restrictions that make it impossible to make it as secure.

mgol force-pushed thexss-htmlprefilter branch fromf21f2d8 to2df43c0Compare

May 26, 2020 16:58

Copy link

MemberAuthor

mgol commentedMay 26, 2020

@petebacondarwin PR updated.

petebacondarwin approved these changes

Copy link

Contributor

petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We should see if@koto has any more feedback before we merge, yes?

Copy link

Contributor

koto commentedMay 26, 2020

It looks good (thanks!), but I have a small suggestion (and I realise this is not about fixing the bug, but rather adding support for passingTrustedHTML objects to jqLite).

You don't modify the payload injqLiteBuildFragment anymore for most cases, butTrustedHTML would still not be able to be passed there, as it fails theargIsString check inJQLite function, TrustedHTMLs are objects -typeof trustedTypes.emptyHTML == 'object'.

How about changing it to :

if(argIsString||(typeofTrustedHTML=="function"&&elementinstanceofTrustedHTML)){jqLiteAddNodes(this,jqLiteParseHTML(element));}// ...

that would pipe the TrustedHTML objects tojqLiteBuildFragment.

For testing, one can create TrustedHTML objects like so:

if(window.trustedTypes){varpolicy=trustedTypes.createPolicy('jqlitetests',{createHTML:function(s){returns}});vartrustedHTML=policy.createHTML('<div><img src=x></div>');}

Copy link

MemberAuthor

mgol commentedMay 26, 2020•
edited
Loading

@koto

if(argIsString||(typeofTrustedHTML=="function"&&elementinstanceofTrustedHTML)){jqLiteAddNodes(this,jqLiteParseHTML(element));}// ...

This won't work cross-frame, will it? I'm not sure if that's an important or even supported use case for AngularJS, I know this would be a big roadblock for jQuery.

In any case, this is getting a bit far from the original purpose of this PR. I may not have much more time to spend on this PR and if we want to add explicit support for trusted types, we should carefully look at all existing jqLite APIs likeafter and make sure they work as well by adding tests for them all. Can we merge this PR as-is and then handle remaining issues with trusted types in a separate one? This would also be clearer in the Git log.

A small offtop: as I'm involved in jQuery as well, I've been thinking about including similar improvements to jQuery. AngularJS has included code specific to APIs not available cross-browser like Chrome apps and now possibly trusted types but jQuery has historically avoided such things. AvoidinginnerHTML for wrapping like done in this PR would work but we wouldn't want to mention theTrustedHTML constructor explicitly as long as it's a Blink-only feature. Do you think there's any other way to support this use case without mentioning any trusted types API explicitly? I'm asking here as if it's possible maybe the same technique could be applied here. If you prefer to continue this part of the discussion on the jQuery side, there's an open issue about trusted types support:jquery/jquery#4409.

mgol mentioned this pull request

Have Trusted Types API built directly into the jQuery Core Filesjquery/jquery#4409

Closed

Splaktar reviewed

src/Angular.js OutdatedShow resolvedHide resolved

src/jqLite.js OutdatedShow resolvedHide resolved

test/jqLiteSpec.jsShow resolvedHide resolved

This comment has been minimized.

fix(jqLite): apply suggestions from code review

05cf606

Co-authored-by: Michael Prentice <splaktar@gmail.com>

Splaktar approved these changes

Copy link

Contributor

Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM

Copy link

Contributor

petebacondarwin commentedMay 26, 2020

@koto - thanks for your review.

Regarding the suggestion of handling passingTrustedHTML objects to jqLite. I think it is a good idea, but given the current status of the AngularJS project, I am loathe for us to make any more changes than necessary to the code. Any change we make is a potential opportunity to open up issues for projects that are already out there in the wild. This would make more work for us, and potentially lead to yet more changes in the project.

As I understand it, the suggestion is not an essential fix to help with security, right? If so, then we should leave the PR as it is here, get it merged and release AngularJS to mitigate the current security concern. Does that sounds reasonable or am I missing something?

mgol commented