Skip to content

Commit

Permalink
feat(security): allow url(...) style values.
Browse files Browse the repository at this point in the history
Allows sanitized URLs for CSS properties. These can be abused for information
leakage, but only if the CSS rules are already set up to allow for it. That is,
an attacker cannot cause information leakage without controlling the style rules
present, or a very particular setup.

Fixes #8514.
  • Loading branch information
mprobst committed May 17, 2016
1 parent dd50124 commit 15ae710
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 4 deletions.
34 changes: 30 additions & 4 deletions modules/@angular/platform-browser/src/security/style_sanitizer.ts
@@ -1,6 +1,8 @@
import {getDOM} from '../dom/dom_adapter';
import {assertionsEnabled} from '../../src/facade/lang';

import {sanitizeUrl} from './url_sanitizer';

/**
* Regular expression for safe style values.
*
Expand All @@ -19,10 +21,29 @@ const VALUES = '[-,."\'%_!# a-zA-Z0-9]+';
const TRANSFORMATION_FNS = '(?:matrix|translate|scale|rotate|skew|perspective)(?:X|Y|3d)?';
const COLOR_FNS = '(?:rgb|hsl)a?';
const FN_ARGS = '\\([-0-9.%, a-zA-Z]+\\)';

const SAFE_STYLE_VALUE =
new RegExp(`^(${VALUES}|(?:${TRANSFORMATION_FNS}|${COLOR_FNS})${FN_ARGS})$`, 'g');

/**
* Matches a `url(...)` value with an arbitrary argument as long as it does
* not contain parentheses.
*
* The URL value still needs to be sanitized separately.
*
* `url(...)` values are a very common use case, e.g. for `background-image`. With carefully crafted
* CSS style rules, it is possible to construct an information leak with `url` values in CSS, e.g.
* by observing whether scroll bars are displayed, or character ranges used by a font face
* definition.
*
* Angular only allows binding CSS values (as opposed to entire CSS rules), so it is unlikely that
* binding a URL value without further cooperation from the page will cause an information leak, and
* if so, it is just a leak, not a full blown XSS vulnerability.
*
* Given the common use case, low likelihood of attack vector, and low impact of an attack, this
* code is permissive and allows URLs that sanitize otherwise.
*/
const URL_RE = /^url\(([^)]+)\)$/;

/**
* Checks that quotes (" and ') are properly balanced inside a string. Assumes
* that neither escape (\) nor any other character that could result in
Expand Down Expand Up @@ -51,11 +72,16 @@ function hasBalancedQuotes(value: string) {
*/
export function sanitizeStyle(value: string): string {
value = String(value).trim(); // Make sure it's actually a string.
if (value.match(SAFE_STYLE_VALUE) && hasBalancedQuotes(value)) return value;

if (assertionsEnabled()) {
getDOM().log('WARNING: sanitizing unsafe style value ' + value);
// Single url(...) values are supported, but only for URLs that sanitize cleanly. See above for
// reasoning behind this.
let urlMatch = URL_RE.exec(value);
if ((urlMatch && sanitizeUrl(urlMatch[1]) === urlMatch[1]) ||
value.match(SAFE_STYLE_VALUE) && hasBalancedQuotes(value)) {
return value; // Safe style values.
}

if (assertionsEnabled()) getDOM().log('WARNING: sanitizing unsafe style value ' + value);

return 'unsafe';
}
Expand Up @@ -30,5 +30,10 @@ export function main() {
expectSanitize('translateX(12px, -5px)').toEqual('translateX(12px, -5px)');
expectSanitize('scale3d(1, 1, 2)').toEqual('scale3d(1, 1, 2)');
});
t.it('sanitizes URLs', () => {
expectSanitize('url(foo/bar.png)').toEqual('url(foo/bar.png)');
expectSanitize('url(javascript:evil())').toEqual('unsafe');
expectSanitize('url(strangeprotocol:evil)').toEqual('unsafe');
});
});
}

0 comments on commit 15ae710

Please sign in to comment.