Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Test failure on Chrome 62 #16288

Closed
Narretz opened this issue Oct 18, 2017 · 5 comments · Fixed by #16311
Closed

Test failure on Chrome 62 #16288

Narretz opened this issue Oct 18, 2017 · 5 comments · Fixed by #16311
Assignees
@petebacondarwin
Labels
browser: chrome needs: investigation severity: security
Milestone
1.7.0

Comments

@Narretz
Copy link
Contributor

Narretz commented Oct 18, 2017 

Chrome 62.0.3202 (Windows 10 0.0.0) HTML should prevent mXSS attacks FAILED

       Expected '<a href="&#12288;javascript:alert(1)">CLICKME</a>' to be '<a>CLICKME</a>'.

           at Object.<anonymous> (test/ngSanitize/sanitizeSpec.js:242:71)
```
@Narretz Narretz added this to the Ice Box milestone Oct 18, 2017
@petebacondarwin petebacondarwin modified the milestones: Ice Box, 1.7.0 Oct 31, 2017
@petebacondarwin
Copy link
Contributor

This isn't picked up by Travis CI because SauceLabs does not currently offer Chrome v62

@petebacondarwin petebacondarwin self-assigned this Oct 31, 2017
@Narretz
Copy link
Contributor Author

Narretz commented Oct 31, 2017

I think I remember why I didn't mark this as security: the string is not actually executed as javascript because of the leading IDEOGRAPHIC SPACE (&#x3000;).
So I'm not actually sure what this is testing. Also note that this test runs only in Chrome. Maybe C62 is now aligned with other browsers and doesn't need special treatment? The commit is unfortunately very light on this mxss stuff.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Oct 31, 2017
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
bf1f5d0 
Chrome 62 was not sanitizing dangerous URLs containing
JavaScript, if they started with these "whitespace" characters.

Closes angular#16288
@petebacondarwin petebacondarwin mentioned this issue Oct 31, 2017
Merged
3 tasks
@petebacondarwin
Copy link
Contributor

Created a fix here: #16311
It won't be tested via Travis (and SauceLabs) though, sadly.

@mgol
Copy link
Member

mgol commented Oct 31, 2017 •
edited
Loading

The fact that those tests are run in Chrome only means we wouldn't catch similar regressions in other browsers, that sounds bad. Can we enable them everywhere?

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Nov 1, 2017
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
80cfb04 
Chrome 62 was not sanitizing dangerous URLs containing
JavaScript, if they started with these "whitespace" characters.

---

Browsers convert `&angular#12288;javascript:alert(1)` as an attribute value to
`javascript:alert(1)`. So the sanitizer gets the second string and is
able to strip the javascript from the attribute.

But Chrome (<62) only did this after you read it and wrote it back again.
So we added a bit of code that tried to get Chrome to do its conversion
before sanitizing it:
https://github.com/angular/angular.js/blob/817ac56719505680ac4c9997972e8f39eb40a6d0/src/ngSanitize/sanitize.js#L406-L417

I believe that Chrome 62 now does not do this conversion any more, instead
it just leaves the attribute value alone, whatever you do to it.

This fix uses `trim()` to remove the problematic whitespace before
sanitizing, which appears to solve the problem.

Closes angular#16288
@petebacondarwin
Copy link
Contributor

I have enabled that test across the board now.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Nov 1, 2017
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
769306d 
Chrome 62 was not sanitizing dangerous URLs containing
JavaScript, if they started with these "whitespace" characters.

---

Browsers convert `&angular#12288;javascript:alert(1)` as an attribute value to
`javascript:alert(1)`. So the sanitizer gets the second string and is
able to strip the javascript from the attribute.

But Chrome (<62) only did this after you read it and wrote it back again.
So we added a bit of code that tried to get Chrome to do its conversion
before sanitizing it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

I believe that Chrome 62 now does not do this conversion any more, instead
it just leaves the attribute value alone, whatever you do to it.

This fix uses `trim()` to remove the problematic whitespace before
sanitizing, which appears to solve the problem.

Closes angular#16288
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Nov 2, 2017
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
ec3673d 
Browsers mutate attributes values such as `&angular#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes angular#16288
petebacondarwin added a commit that referenced this issue Nov 3, 2017
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
667db46 
Browsers mutate attributes values such as `&#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes #16288
petebacondarwin added a commit that referenced this issue Nov 3, 2017
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
ddeb1df 
Browsers mutate attributes values such as `&#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes #16288
@dependencies dependencies bot mentioned this issue Dec 5, 2017
Merged
cwick pushed a commit to udemy/angular.js that referenced this issue Nov 23, 2019
@petebacondarwin
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
b415dc0 
Browsers mutate attributes values such as `&angular#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes angular#16288
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@petebacondarwin petebacondarwin

Labels
browser: chrome needs: investigation severity: security
Projects
None yet
Milestone
1.7.0
Development

Successfully merging a pull request may close this issue.

fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars petebacondarwin/angular.js
3 participants
@petebacondarwin @Narretz @mgol