From 0b439090464507e0ea168075e4a5b196b8332a95 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Tue, 21 Feb 2017 16:19:24 +0100 Subject: [PATCH 1/2] feat($compile): Lower the security context of SVG's a and image xlink:href attributes. They should go through regular sanitization, RESOURCE_URL is overkill there. This does not change the context for the rest of xlink:href attributes. This is a breaking change in very unlikely cases. If someone whitelisted RESOURCE_URL for the purpose of binding into xlink:href, and that these can't pass the regular URL sanitization, they'll get broken: the fix is to whitelist in the $compileProvider's URL sanitization. --- src/ng/compile.js | 9 ++++++--- test/ng/compileSpec.js | 39 +++++++++++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 028901f0644d..4826979f52f9 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1671,7 +1671,8 @@ 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'); } else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) { @@ -3254,8 +3255,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..5ea8d23c11ae 100644 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -11122,23 +11122,42 @@ 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) { + element = $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(element.find('a').attr('xlink:href')).toBe('https://clean.example.org'); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + }); + }); + + 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 +11172,18 @@ 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() { From 0a22243d3d1a8c192359a1e09a6a2660f75089e6 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Fri, 3 Mar 2017 14:17:16 +0100 Subject: [PATCH 2/2] fix($compile): image xlink:href should be sanitized as images, not as links. --- src/ng/compile.js | 3 ++- test/ng/compileSpec.js | 12 ++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 4826979f52f9..eff02b1a4201 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1674,7 +1674,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { (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 = ''; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 5ea8d23c11ae..791c21ee062c 100644 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -11127,15 +11127,20 @@ describe('$compile', function() { $provide.value('$$sanitizeUri', $$sanitizeUri); }); inject(function($compile, $rootScope) { - element = $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(element.find('a').attr('xlink:href')).toBe('https://clean.example.org'); - expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false); + 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); }); }); @@ -11173,7 +11178,6 @@ describe('$compile', function() { }); }); - it('should have a RESOURCE_URL context for xlink:href by default', function() { inject(function($compile, $rootScope) { element = $compile('')($rootScope);