From 9532234bf1c408af9a6fd2c4743fdb585b920531 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Tue, 19 Feb 2013 09:55:05 -0800 Subject: [PATCH] fix($compile): sanitize values bound to a[href] --- src/ng/compile.js | 60 +++++++++++-- src/ng/directive/booleanAttrs.js | 5 +- test/ng/compileSpec.js | 141 +++++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 9 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 788231abe330..11a86f3fd558 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -155,7 +155,8 @@ function $CompileProvider($provide) { Suffix = 'Directive', COMMENT_DIRECTIVE_REGEXP = /^\s*directive\:\s*([\d\w\-_]+)\s+(.*)$/, CLASS_DIRECTIVE_REGEXP = /(([\d\w\-_]+)(?:\:([^;]+))?;?)/, - MULTI_ROOT_TEMPLATE_ERROR = 'Template must have exactly one root element. was: '; + MULTI_ROOT_TEMPLATE_ERROR = 'Template must have exactly one root element. was: ', + urlSanitizationWhitelist = /^\s*(https?|ftp|mailto):/; /** @@ -209,11 +210,41 @@ function $CompileProvider($provide) { }; + /** + * @ngdoc function + * @name ng.$compileProvider#urlSanitizationWhitelist + * @methodOf ng.$compileProvider + * @function + * + * @description + * Retrieves or overrides the default regular expression that is used for whitelisting of safe + * urls during a[href] 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. + * + * @param {RegExp=} regexp New regexp to whitelist urls with. + * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for + * chaining otherwise. + */ + this.urlSanitizationWhitelist = function(regexp) { + if (isDefined(regexp)) { + urlSanitizationWhitelist = regexp; + return this; + } + return urlSanitizationWhitelist; + }; + + this.$get = [ '$injector', '$interpolate', '$exceptionHandler', '$http', '$templateCache', '$parse', - '$controller', '$rootScope', + '$controller', '$rootScope', '$document', function($injector, $interpolate, $exceptionHandler, $http, $templateCache, $parse, - $controller, $rootScope) { + $controller, $rootScope, $document) { var Attributes = function(element, attr) { this.$$element = element; @@ -235,7 +266,8 @@ function $CompileProvider($provide) { */ $set: function(key, value, writeAttr, attrName) { var booleanKey = getBooleanAttrName(this.$$element[0], key), - $$observers = this.$$observers; + $$observers = this.$$observers, + normalizedVal; if (booleanKey) { this.$$element.prop(key, value); @@ -254,6 +286,19 @@ function $CompileProvider($provide) { } } + + // sanitize a[href] values + if (nodeName_(this.$$element[0]) === 'A' && key === 'href') { + urlSanitizationNode.setAttribute('href', value); + + // href property always returns normalized absolute url, so we can match against that + normalizedVal = urlSanitizationNode.href; + if (!normalizedVal.match(urlSanitizationWhitelist)) { + this[key] = value = 'unsafe:' + normalizedVal; + } + } + + if (writeAttr !== false) { if (value === null || value === undefined) { this.$$element.removeAttr(attrName); @@ -297,7 +342,8 @@ function $CompileProvider($provide) { } }; - var startSymbol = $interpolate.startSymbol(), + var urlSanitizationNode = $document[0].createElement('a'), + startSymbol = $interpolate.startSymbol(), endSymbol = $interpolate.endSymbol(), denormalizeTemplate = (startSymbol == '{{' || endSymbol == '}}') ? identity @@ -748,7 +794,7 @@ function $CompileProvider($provide) { } break; } - + case '=': { parentGet = $parse(attrs[attrName]); parentSet = parentGet.assign || function() { @@ -1029,10 +1075,10 @@ function $CompileProvider($provide) { function addAttrInterpolateDirective(node, directives, value, name) { var interpolateFn = $interpolate(value, true); - // no interpolation found -> ignore if (!interpolateFn) return; + directives.push({ priority: 100, compile: valueFn(function attrInterpolateLinkFn(scope, element, attr) { diff --git a/src/ng/directive/booleanAttrs.js b/src/ng/directive/booleanAttrs.js index 739c539a7353..7e0e3a4205ee 100644 --- a/src/ng/directive/booleanAttrs.js +++ b/src/ng/directive/booleanAttrs.js @@ -340,8 +340,9 @@ forEach(['src', 'href'], function(attrName) { // on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist // then calling element.setAttribute('src', 'foo') doesn't do anything, so we need - // to set the property as well to achieve the desired effect - if (msie) element.prop(attrName, value); + // to set the property as well to achieve the desired effect. + // we use attr[attrName] value since $set can sanitize the url. + if (msie) element.prop(attrName, attr[attrName]); }); } }; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 6fbf99c303ed..d2eddafcba78 100644 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -2354,7 +2354,148 @@ describe('$compile', function() { expect(jqLite(element.find('span')[1]).text()).toEqual('T:true'); }); }); + }); + + + describe('href sanitization', function() { + + it('should sanitize javascript: urls', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + + expect(element.attr('href')).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('href')).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].href).toBe('unsafe:javascript:doEvilStuff()'); + + // tab in protocol + $rootScope.testUrl = "java\u0009script:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].href).toMatch(/(http:\/\/|unsafe:javascript:doEvilStuff\(\))/); + + // space before + $rootScope.testUrl = " javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].href).toBe('unsafe:javascript:doEvilStuff()'); + + // ws chars before + $rootScope.testUrl = " \u000e javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element[0].href).toMatch(/(http:\/\/|unsafe:javascript:doEvilStuff\(\))/); + + // post-fixed with proper url + $rootScope.testUrl = "javascript:doEvilStuff(); http://make.me/look/good"; + $rootScope.$apply(); + expect(element[0].href).toBeOneOf( + 'unsafe:javascript:doEvilStuff(); http://make.me/look/good', + 'unsafe:javascript:doEvilStuff();%20http://make.me/look/good' + ); + })); + + + it('should sanitize ngHref bindings as well', inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + + expect(element[0].href).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('href')).toBe('foo/bar'); + + $rootScope.testUrl = "/foo/bar"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('/foo/bar'); + + $rootScope.testUrl = "../foo/bar"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('../foo/bar'); + + $rootScope.testUrl = "#foo"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('#foo'); + $rootScope.testUrl = "http://foo/bar"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('http://foo/bar'); + $rootScope.testUrl = " http://foo/bar"; + $rootScope.$apply(); + expect(element.attr('href')).toBe(' http://foo/bar'); + + $rootScope.testUrl = "https://foo/bar"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('https://foo/bar'); + + $rootScope.testUrl = "ftp://foo/bar"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('ftp://foo/bar'); + + $rootScope.testUrl = "mailto:foo@bar.com"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('mailto:foo@bar.com'); + })); + + + it('should not sanitize href on elements other than anchor', inject(function($compile, $rootScope) { + element = $compile('
')($rootScope); + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + + expect(element.attr('href')).toBe('javascript:doEvilStuff()'); + })); + + + it('should not sanitize attributes other than href', 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 href 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); + + $rootScope.testUrl = "javascript:doEvilStuff()"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('javascript:doEvilStuff()'); + + $rootScope.testUrl = "http://recon/figured"; + $rootScope.$apply(); + expect(element.attr('href')).toBe('unsafe:http://recon/figured'); + }); + }); }); });