From 1adf29af13890d61286840177607edd552a9df97 Mon Sep 17 00:00:00 2001 From: Chirayu Krishnappa Date: Fri, 21 Jun 2013 12:33:03 -0700 Subject: [PATCH] fix($compile): sanitize values bound to img[src] Ref: 9532234bf1c408af9a6fd2c4743fdb585b920531 BREAKING CHANGE: img[src] URLs are now sanitized using the same whitelist as a[href] URLs. The most obvious impact is if you were using data: URIs. data: URIs will be whitelisted for img[src] in a future commit. --- src/ng/compile.js | 20 +++--- test/ng/compileSpec.js | 151 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 162 insertions(+), 9 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 851f24049272..d85af28c6eba 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -215,14 +215,15 @@ function $CompileProvider($provide) { * * @description * Retrieves or overrides the default regular expression that is used for whitelisting of safe - * urls during a[href] sanitization. + * urls during a[href] and img[src] sanitization. * * The sanitization is a security measure aimed at prevent XSS attacks via html links. * - * Any url about to be assigned to a[href] via data-binding is first normalized and turned into an - * absolute url. Afterwards the url is matched against the `urlSanitizationWhitelist` regular - * expression. If a match is found the original url is written into the dom. Otherwise the - * absolute url is prefixed with `'unsafe:'` string and only then it is written into the DOM. + * Any url about to be assigned to a[href] or img[src] via data-binding is first normalized and + * turned into an absolute url. Afterwards, the url is matched against the + * `urlSanitizationWhitelist` regular expression. If a match is found, the original url is written + * into the dom. Otherwise, the absolute url is prefixed with `'unsafe:'` string and only then is + * it written into the DOM. * * @param {RegExp=} regexp New regexp to whitelist urls with. * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for @@ -264,7 +265,8 @@ function $CompileProvider($provide) { $set: function(key, value, writeAttr, attrName) { var booleanKey = getBooleanAttrName(this.$$element[0], key), $$observers = this.$$observers, - normalizedVal; + normalizedVal, + nodeName; if (booleanKey) { this.$$element.prop(key, value); @@ -284,8 +286,10 @@ function $CompileProvider($provide) { } - // sanitize a[href] values - if (nodeName_(this.$$element[0]) === 'A' && key === 'href') { + // sanitize a[href] and img[src] values + nodeName = nodeName_(this.$$element); + if ((nodeName === 'A' && key === 'href') || + (nodeName === 'IMG' && key === 'src')){ urlSanitizationNode.setAttribute('href', value); // href property always returns normalized absolute url, so we can match against that diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 8b48ea085950..b713476b8753 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -2536,7 +2536,156 @@ describe('$compile', function() { }); - describe('href sanitization', function() { + describe('img[src] sanitization', function() { + it('should NOT require trusted values for img src', inject(function($rootScope, $compile) { + element = $compile('')($rootScope); + $rootScope.testUrl = 'http://example.com/image.png'; + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image.png'); + })); + + it('should sanitize javascript: urls', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('unsafe:javascript:doEvilStuff()'); + })); + + it('should sanitize data: urls', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "data:evilPayload"; + $rootScope.$apply(); + + expect(element.attr('src')).toBe('unsafe:data:evilPayload'); + })); + + + it('should sanitize obfuscated javascript: urls', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + + // case-sensitive + $rootScope.testUrl = "JaVaScRiPt:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].src).toBe('unsafe:javascript:doEvilStuff()'); + + // tab in protocol + $rootScope.testUrl = "java\u0009script:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].src).toMatch(/(http:\/\/|unsafe:javascript:doEvilStuff\(\))/); + + // space before + $rootScope.testUrl = " javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].src).toBe('unsafe:javascript:doEvilStuff()'); + + // ws chars before + $rootScope.testUrl = " \u000e javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].src).toMatch(/(http:\/\/|unsafe:javascript:doEvilStuff\(\))/); + + // post-fixed with proper url + $rootScope.testUrl = "javascript:doEvilStuff(); http://make.me/look/good"; + $rootScope.$apply(); + expect(element[0].src).toBeOneOf( + 'unsafe:javascript:doEvilStuff(); http://make.me/look/good', + 'unsafe:javascript:doEvilStuff();%20http://make.me/look/good' + ); + })); + + it('should sanitize ng-src bindings as well', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + + expect(element[0].src).toBe('unsafe:javascript:doEvilStuff()'); + })); + + + it('should not sanitize valid urls', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + + $rootScope.testUrl = "foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('foo/bar'); + + $rootScope.testUrl = "/foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('/foo/bar'); + + $rootScope.testUrl = "../foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('../foo/bar'); + + $rootScope.testUrl = "#foo"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('#foo'); + + $rootScope.testUrl = "http://foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('http://foo/bar'); + + $rootScope.testUrl = " http://foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe(' http://foo/bar'); + + $rootScope.testUrl = "https://foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('https://foo/bar'); + + $rootScope.testUrl = "ftp://foo/bar"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('ftp://foo/bar'); + + // Fails on IE < 10 with "TypeError: Access is denied" when trying to set img[src] + if (!msie || msie > 10) { + $rootScope.testUrl = "mailto:foo@bar.com"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('mailto:foo@bar.com'); + } + + $rootScope.testUrl = "file:///foo/bar.html"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('file:///foo/bar.html'); + })); + + + it('should not sanitize attributes other than src', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + + expect(element.attr('title')).toBe('javascript:doEvilStuff()'); + })); + + + it('should allow reconfiguration of the src whitelist', function() { + module(function($compileProvider) { + expect($compileProvider.urlSanitizationWhitelist() instanceof RegExp).toBe(true); + var returnVal = $compileProvider.urlSanitizationWhitelist(/javascript:/); + expect(returnVal).toBe($compileProvider); + }); + + inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + + // Fails on IE < 10 with "TypeError: Object doesn't support this property or method" when + // trying to set img[src] + if (!msie || msie > 10) { + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('javascript:doEvilStuff()'); + } + + $rootScope.testUrl = "http://recon/figured"; + $rootScope.$apply(); + expect(element.attr('src')).toBe('unsafe:http://recon/figured'); + }); + }); + + }); + + + describe('a[href] sanitization', function() { it('should sanitize javascript: urls', inject(function($compile, $rootScope) { element = $compile('')($rootScope);