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

Commit

Permalink
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
Browse files Browse the repository at this point in the history
Browsers mutate attributes values such as ` 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
  • Loading branch information
petebacondarwin committed Nov 3, 2017
1 parent 2c9c3a0 commit 667db46
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/ng/sanitizeUri.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function $$SanitizeUriProvider() {
return function sanitizeUri(uri, isImage) {
var regex = isImage ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
var normalizedVal;
normalizedVal = urlResolve(uri).href;
normalizedVal = urlResolve(uri && uri.trim()).href;
if (normalizedVal !== '' && !normalizedVal.match(regex)) {
return 'unsafe:' + normalizedVal;
}
Expand Down
8 changes: 3 additions & 5 deletions test/ngSanitize/sanitizeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,9 @@ describe('HTML', function() {
.toEqual('');
});

if (isChrome) {
it('should prevent mXSS attacks', function() {
expectHTML('<a href="&#x3000;javascript:alert(1)">CLICKME</a>').toBe('<a>CLICKME</a>');
});
}
it('should prevent mXSS attacks', function() {
expectHTML('<a href="&#x3000;javascript:alert(1)">CLICKME</a>').toBe('<a>CLICKME</a>');
});

it('should strip html comments', function() {
expectHTML('<!-- comment 1 --><p>text1<!-- comment 2 -->text2</p><!-- comment 3 -->')
Expand Down

0 comments on commit 667db46

Please sign in to comment.