From 6ccbfa65d60a3dc396d0cf6da21b993ad74653fd Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Tue, 21 Feb 2017 16:19:24 +0100 Subject: [PATCH] feat($compile): lower the `xlink:href` security context for SVG's `a` and `image` elements Previously, `xlink:href` on SVG's `` and `` elements, was `$sce.RESOURCE_URL`. While this makes sense for other `xlink:href` usecases, it was an overkill for these elements. This commit lowers the `xlink:href` security context for these specific elements, treating it in the same way as `a[href]` or `img[src]` respectively. The `xlink:href` security context for other elements is not affected. BREAKING CHANGE: In the unlikely case that an app relied on RESOURCE_URL whitelisting for the purpose of binding to the `xlink:href` property of SVG's `` or `` elements and if the values do not pass the regular URL sanitization, they will break. To fix this you need to ensure that the values used for binding to the affected `xlink:href` contexts are considered safe URLs, e.g. by whitelisting them in `$compileProvider`'s `aHrefSanitizationWhitelist` (for `` elements) or `imgSrcSanitizationWhitelist` (for `` elements). Closes #15736 --- src/ng/compile.js | 11 +++++++---- test/ng/compileSpec.js | 43 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index d5e237c6adc2..92e271e44732 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1673,9 +1673,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) || - (nodeName === 'img' && key === 'src')) { + (nodeName === 'img' && key === 'src') || + (nodeName === 'image' && key === 'xlinkHref')) { // sanitize a[href] and img[src] values - this[key] = value = $$sanitizeUri(value, key === 'src'); + this[key] = value = $$sanitizeUri(value, nodeName === 'img' || nodeName === 'image'); } else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) { // sanitize img[srcset] values var result = ''; @@ -3256,8 +3257,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) { return $sce.RESOURCE_URL; } - // maction[xlink:href] can source SVG. It's not limited to . - } else if (attrNormalizedName === 'xlinkHref' || + } else if ( + // Some xlink:href are okay, most aren't + (attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) || + // Formaction (tag === 'form' && attrNormalizedName === 'action') || // If relative URLs can go where they are not expected to, then // all sorts of trust issues can arise. diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 59c685753f99..791c21ee062c 100644 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -11122,23 +11122,47 @@ describe('$compile', function() { }); it('should use $$sanitizeUri when working with svg and xlink:href', function() { + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope) { + var elementA = $compile('')($rootScope); + var elementImage = $compile('')($rootScope); + + //both of these fail the RESOURCE_URL test, that shouldn't be run + $rootScope.testUrl = 'https://bad.example.org'; + $$sanitizeUri.and.returnValue('https://clean.example.org'); + + $rootScope.$apply(); + expect(elementA.find('a').attr('xlink:href')).toBe('https://clean.example.org'); + expect(elementImage.find('image').attr('xlink:href')).toBe('https://clean.example.org'); + // is navigational, so the second argument should be false to reach the aHref whitelist + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl + 'aTag' , false); + // is media inclusion, it should use the imgSrc whitelist + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl + 'imageTag', true); + }); + }); + + it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() { var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); module(function($provide) { $provide.value('$$sanitizeUri', $$sanitizeUri); }); inject(function($compile, $rootScope) { element = $compile('')($rootScope); - $rootScope.testUrl = 'evilUrl'; + //both of these fail the RESOURCE_URL test, that shouldn't be run + $rootScope.testUrl = 'https://bad.example.org'; + $$sanitizeUri.and.returnValue('https://clean.example.org'); - $$sanitizeUri.and.returnValue('someSanitizedUrl'); $rootScope.$apply(); - expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl'); + expect(element.find('a').prop('href').baseVal).toBe('https://clean.example.org'); expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); }); }); - it('should use $$sanitizeUri when working with svg and xlink:href', function() { + it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() { var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); module(function($provide) { $provide.value('$$sanitizeUri', $$sanitizeUri); @@ -11153,6 +11177,17 @@ describe('$compile', function() { expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); }); }); + + it('should have a RESOURCE_URL context for xlink:href by default', function() { + inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = 'https://bad.example.org'; + + expect(function() { + $rootScope.$apply(); + }).toThrowError(/\$sce:insecurl/); + }); + }); }); describe('interpolation on HTML DOM event handler attributes onclick, onXYZ, formaction', function() {