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

mgol · 2020-05-20T09:25:33Z

AngularJS is in LTS mode

We are no longer accepting changes that are not critical bug fixes into this project.
See https://blog.angular.io/stable-angularjs-and-long-term-support-7e077635ee9c for more detail.

Does this PR fix a regression since 1.7.0, a security flaw, or a problem caused by a new browser version?

Yes

What is the current behavior? (You can also link to an open issue here)

The regex-based input HTML replacement may turn sanitized code into unsanitized one. An analogous jQuery advisory: GHSA-gxr4-xjj5-5px2
Wrapping <option> elements in <select> ones changes parsing behavior, leading to possibly unsanitizing sanitized code. An analogous jQuery advisory: GHSA-jpcq-cgw6-v4j6

What is the new behavior (if this is a feature change)?

The issues are fixed. The second one is a breaking change so a new method restoring legacy insecure behavior:

angular.UNSAFE_restoreLegacyJqLiteXHTMLReplacement()

was added.

Does this PR introduce a breaking change?

Yes

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

src/jqLite.js

koto · 2020-05-20T10:08:38Z

src/jqLite.js

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


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 under tbody or table.

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, use document.createElement and appendChild and append to the newly created one.

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

You do. See https://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, use document.createElement and appendChild and append to the newly created one.

That would work.

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.

src/Angular.js

src/jqLite.js

petebacondarwin · 2020-05-20T10:12:05Z

test/jqLiteSpec.js

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


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 the onerror 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 different onerror handler? Then assume that if this second handler is called but the xss one is not called then we are good... Then we could make the it async and only call done() once the second real error handler has been called.

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 · 2020-05-20T10:15:00Z

src/jqLite.js

@@ -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;


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 through TrustedHTML objects that DOMPurify might return).

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.

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

gkalpak · 2020-05-20T13:29:47Z

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>'


I think that the following test cases were useful for jQuery (which used a slightly different regex) but not for jqLite's XHTML_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.)

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 · 2020-05-26T15:09:22Z

@koto I refactored the whole thing to not rely on passing strings to innerHTML at all except in IE 9 where it's unavoidable - IE 9 doesn't let you to assign '<td></td>' to a tr 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 by 407 bytes (before gzip) due to duplication but at least it should be more secure in modern browsers.

Please take a look.

petebacondarwin

On master the angular.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

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

mgol · 2020-05-26T16:58:39Z

@petebacondarwin PR updated.

petebacondarwin

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

koto · 2020-05-26T17:36:00Z

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 passing TrustedHTML objects to jqLite).

You don't modify the payload in jqLiteBuildFragment anymore for most cases, but TrustedHTML would still not be able to be passed there, as it fails the argIsString check in JQLite function, TrustedHTMLs are objects - typeof trustedTypes.emptyHTML == 'object'.

How about changing it to :

  if (argIsString || (typeof TrustedHTML == "function" && element instanceof TrustedHTML)) {
      jqLiteAddNodes(this, jqLiteParseHTML(element));
  } // ...

that would pipe the TrustedHTML objects to jqLiteBuildFragment.

For testing, one can create TrustedHTML objects like so:

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

mgol · 2020-05-26T19:29:45Z

@koto

  if (argIsString || (typeof TrustedHTML == "function" && element instanceof TrustedHTML)) {
      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 like after 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. Avoiding innerHTML for wrapping like done in this PR would work but we wouldn't want to mention the TrustedHTML 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.

src/Angular.js

src/jqLite.js

test/jqLiteSpec.js

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

Splaktar

LGTM

petebacondarwin · 2020-05-26T20:16:20Z

@koto - thanks for your review.

Regarding the suggestion of handling passing TrustedHTML 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?

src/jqLite.js

koto · 2020-05-27T07:48:09Z

@koto

  if (argIsString || (typeof TrustedHTML == "function" && element instanceof TrustedHTML)) {
      jqLiteAddNodes(this, jqLiteParseHTML(element));
  } // ...

This won't work cross-frame, will it?

Correct. Trusted Types don't work if passed to different realms (i.e. DOM would reject them if Types are enforced), so it makes sense that TT would not be recognised as special values in jqLite as well (instanceof check would fail). But that doesn't break the application, unless it already opted into enforcing Trusted Types via CSP. In such case there's no way out of refactoring the application into creating a trusted type locally.

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.

I'd like to understand this more. jQuery would be a TT consumer, the intention of a potential PR is not to make all applications using jQuery compliant with TT restrictions, but only to make apps that already create types able to use jQuery without losing them. Some refactoring is needed at the application side (to create types in the first place, but also to possibly change insecure patterns) But this is off-topic here, let's discuss in jQuery or separate bugs.

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 like after 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.

Fully agreed.

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. Avoiding innerHTML for wrapping like done in this PR would work but we wouldn't want to mention the TrustedHTML 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.

Let's move this to jQuery. Thanks Michał and all for the fix!

Concatenating HTML strings in buildFragment is a possible security risk as it creates an opportunity of escaping the concatenated wrapper. It also makes it impossible to support secure HTML wrappers like [trusted types](https://web.dev/trusted-types/). It's safer to create wrapper elements using `document.createElement` & `appendChild`. The previous way was needed in jQuery <4 because IE <10 doesn't accept table parts set via `innerHTML`, even if the element which contents are set is a proper table element, e.g.: ```js tr.innerHTML = "<td></td>"; ``` The whole structure needs to be passed in one HTML string. jQuery 4 drops support for IE <11 so this is no longer an issue; in older version we'd have to duplicate the code paths. IE <10 needed to have `<option>` elements wrapped in `<select multiple="multiple">` but we no longer need that on master which makes the `document.createElement` way shorter as we don't have to call `setAttribute`. jQuery 1.x sometimes needed to have more than one element in the wrapper that would precede parts wrapping HTML input so descending needed to use `lastChild`. Since all wrappers are single-element now, we can use `firstChild` which compresses better as it's used in other places in the code as well. All these improvements, apart from making logic more secure, decrease the gzipped size by 55 bytes. Ref jquerygh-4409 Ref angular/angular.js#17028

angular/angular.js#17028

Concatenating HTML strings in buildFragment is a possible security risk as it creates an opportunity of escaping the concatenated wrapper. It also makes it impossible to support secure HTML wrappers like [trusted types](https://web.dev/trusted-types/). It's safer to create wrapper elements using `document.createElement` & `appendChild`. The previous way was needed in jQuery <4 because IE <10 doesn't accept table parts set via `innerHTML`, even if the element which contents are set is a proper table element, e.g.: ```js tr.innerHTML = "<td></td>"; ``` The whole structure needs to be passed in one HTML string. jQuery 4 drops support for IE <11 so this is no longer an issue; in older version we'd have to duplicate the code paths. IE <10 needed to have `<option>` elements wrapped in `<select multiple="multiple">` but we no longer need that on master which makes the `document.createElement` way shorter as we don't have to call `setAttribute`. jQuery 1.x sometimes needed to have more than one element in the wrapper that would precede parts wrapping HTML input so descending needed to use `lastChild`. Since all wrappers are single-element now, we can use `firstChild` which compresses better as it's used in other places in the code as well. All these improvements, apart from making logic more secure, decrease the gzipped size by 55 bytes. Ref jquerygh-4409 Ref angular/angular.js#17028

Concatenating HTML strings in buildFragment is a possible security risk as it creates an opportunity of escaping the concatenated wrapper. It also makes it impossible to support secure HTML wrappers like [trusted types](https://web.dev/trusted-types/). It's safer to create wrapper elements using `document.createElement` & `appendChild`. The previous way was needed in jQuery <4 because IE <10 doesn't accept table parts set via `innerHTML`, even if the element which contents are set is a proper table element, e.g.: ```js tr.innerHTML = "<td></td>"; ``` The whole structure needs to be passed in one HTML string. jQuery 4 drops support for IE <11 so this is no longer an issue; in older version we'd have to duplicate the code paths. IE <10 needed to have `<option>` elements wrapped in `<select multiple="multiple">` but we no longer need that on master which makes the `document.createElement` way shorter as we don't have to call `setAttribute`. All these improvements, apart from making logic more secure, decrease the gzipped size by 58 bytes. Closes gh-4724 Ref gh-4409 Ref angular/angular.js#17028 Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

This new version of AngularJS fixes a security issue (CVE-2020-7676). angular/angular.js#17028 https://github.com/angular/angular.js/blob/v1.8.0/CHANGELOG.md Change-Id: Id5a19f74e025c4de4ec2e6170eb4f01a6a606328

…ment Updated To have angular#17028

zelivans · 2020-10-08T09:41:24Z

It appears CVE-2020-7676 was assigned in association with this PR.
The CVE description mentions the the two problems fixed here so it is currently confusing. Not sure if it should be split, up to the maintainers.

mgol · 2020-10-08T10:42:34Z

@zelivans The two issues are caused by code in the same area and they're quite similar so it was decided to use one only. Equivalent jQuery issues had two CVEs created.

I think it's fine both ways.

angular/angular.js#17028

GHSA-mhp6-pxh8-r675

GHSA-mhp6-pxh8-r675 NOTE: since we only support using with jquery 3.5.1 in Salesforce/vlocity we've modified the tests to account for this and drop older versions of jquery.

mgol requested a review from petebacondarwin May 20, 2020 09:25

googlebot added the cla: yes label May 20, 2020

mgol force-pushed the xss-htmlprefilter branch from 9f24c2d to 1e45057 Compare May 20, 2020 09:30

mgol commented May 20, 2020

View reviewed changes

src/jqLite.js Outdated Show resolved Hide resolved

petebacondarwin added component: jqLite severity: security type: bug labels May 20, 2020

mgol added this to the 1.8.x milestone May 20, 2020

koto reviewed May 20, 2020

View reviewed changes

petebacondarwin reviewed May 20, 2020

View reviewed changes

koto reviewed May 20, 2020

View reviewed changes

gkalpak approved these changes May 20, 2020

View reviewed changes

mgol force-pushed the xss-htmlprefilter branch 4 times, most recently from 0b474b3 to f21f2d8 Compare May 26, 2020 15:06

petebacondarwin reviewed May 26, 2020

View reviewed changes

src/jqLite.js Outdated Show resolved Hide resolved

petebacondarwin reviewed May 26, 2020

View reviewed changes

src/jqLite.js Show resolved Hide 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 9 as IE 9 has restrictions that make it impossible to make it as secure.

mgol force-pushed the xss-htmlprefilter branch from f21f2d8 to 2df43c0 Compare May 26, 2020 16:58

petebacondarwin approved these changes May 26, 2020

View reviewed changes

mgol mentioned this pull request May 26, 2020

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

Closed

Splaktar reviewed May 26, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

fix(jqLite): apply suggestions from code review

05cf606

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

Splaktar approved these changes May 26, 2020

View reviewed changes

mgol commented May 26, 2020

View reviewed changes

src/jqLite.js Outdated Show resolved Hide resolved

fix(jqLite): improve documentation

c8b7c16

petebacondarwin merged commit c8b7c16 into angular:master May 27, 2020

mgol mentioned this pull request Jun 1, 2020

Manipulation: Avoid concatenating strings in buildFragment jquery/jquery#4724

Merged

3 tasks

petebacondarwin referenced this pull request in mgol/angular.js Jun 4, 2020

tests(jQuery): test on both oldest & latest supported jQuery version

244ce08

evilaliv3 added a commit to globaleaks/globaleaks-whistleblowing-software that referenced this pull request Jun 4, 2020

Bump angularjs to version 1.8.0 that fix possible XSS

b09f5a1

angular/angular.js#17028

mgol deleted the xss-htmlprefilter branch June 4, 2020 21:36

nskazki added a commit to Crowd9/angular.js that referenced this pull request Jun 8, 2020

gleam - fix CVE-2020-11022 (more: angular#17028)

e0fd334

PPInfy added a commit to PPInfy/angular.js that referenced this pull request Oct 5, 2020

Security fixes - prevent possible XSS due to regex-based HTML replace…

4edfb09

…ment Updated To have angular#17028

PPInfy mentioned this pull request Oct 5, 2020

Security fixes - prevent possible XSS due to regex-based HTML replace… #17084

Closed

3 tasks

evilaliv3 added a commit to globaleaks/globaleaks-whistleblowing-software that referenced this pull request Jul 27, 2021

Bump angularjs to version 1.8.0 that fix possible XSS

c8b8638

angular/angular.js#17028

mgoldspink-salesforce added a commit to vlocityinc/angular.js that referenced this pull request Jan 20, 2022

backport angular#17028 to fix CVE-2020-7676

f7ececc

GHSA-mhp6-pxh8-r675

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mgol commented May 20, 2020

koto May 20, 2020 •

edited

Loading

mgol May 20, 2020

koto May 20, 2020

petebacondarwin May 20, 2020

mgol May 20, 2020

koto May 20, 2020 •

edited

Loading

mgol May 20, 2020

mgol May 20, 2020 •

edited

Loading

gkalpak May 20, 2020

mgol May 20, 2020

mgol commented May 26, 2020 •

edited

Loading

petebacondarwin left a comment

mgol commented May 26, 2020

petebacondarwin left a comment

koto commented May 26, 2020

mgol commented May 26, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

Splaktar left a comment

petebacondarwin commented May 26, 2020

koto commented May 27, 2020

zelivans commented Oct 8, 2020

mgol commented Oct 8, 2020

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

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

Conversation

mgol commented May 20, 2020

AngularJS is in LTS mode

koto May 20, 2020 • edited Loading

Choose a reason for hiding this comment

mgol May 20, 2020

Choose a reason for hiding this comment

koto May 20, 2020

Choose a reason for hiding this comment

petebacondarwin May 20, 2020

Choose a reason for hiding this comment

mgol May 20, 2020

Choose a reason for hiding this comment

koto May 20, 2020 • edited Loading

Choose a reason for hiding this comment

mgol May 20, 2020

Choose a reason for hiding this comment

mgol May 20, 2020 • edited Loading

Choose a reason for hiding this comment

gkalpak May 20, 2020

Choose a reason for hiding this comment

mgol May 20, 2020

Choose a reason for hiding this comment

mgol commented May 26, 2020 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

mgol commented May 26, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

koto commented May 26, 2020

mgol commented May 26, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

Splaktar left a comment

Choose a reason for hiding this comment

petebacondarwin commented May 26, 2020

koto commented May 27, 2020

zelivans commented Oct 8, 2020

mgol commented Oct 8, 2020

koto May 20, 2020 •

edited

Loading

koto May 20, 2020 •

edited

Loading

mgol May 20, 2020 •

edited

Loading

mgol commented May 26, 2020 •

edited

Loading

mgol commented May 26, 2020 •

edited

Loading