From ff090d752b2ddd3d1ef1b301625997fc4996a887 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Fri, 12 Feb 2016 14:01:10 +0100 Subject: [PATCH 1/8] fix($compile): lower the $sce context for src on video, audio, and track. Previously, video, audio, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks are possible through these attributes as far as I can tell. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. --- src/ng/compile.js | 15 +++++++++------ src/ng/sce.js | 2 +- test/ng/compileSpec.js | 12 ++++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index e61307f3760f..dd3c8e2412d9 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3151,13 +3151,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { return $sce.HTML; } var tag = nodeName_(node); + // All tags with src attributes require a RESOURCE_URL value, except for + // img and various html5 media tags. + if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') { + if (['img', 'video', 'audio', 'track'].indexOf(tag) === -1) { + return $sce.RESOURCE_URL; + } // maction[xlink:href] can source SVG. It's not limited to . - if (attrNormalizedName === 'xlinkHref' || - (tag === 'form' && attrNormalizedName === 'action') || - // links can be stylesheets or imports, which can run script in the current origin - (tag === 'link' && attrNormalizedName === 'href') || - (tag !== 'img' && (attrNormalizedName === 'src' || - attrNormalizedName === 'ngSrc'))) { + } else if (attrNormalizedName === 'xlinkHref' || + (tag === 'form' && attrNormalizedName === 'action') + ) { return $sce.RESOURCE_URL; } } diff --git a/src/ng/sce.js b/src/ng/sce.js index f16aee5597a8..ac693fbf4c56 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -537,7 +537,7 @@ function $SceDelegateProvider() { * | `$sce.HTML` | For HTML that's safe to source into the application. The {@link ng.directive:ngBindHtml ngBindHtml} directive uses this context for bindings. If an unsafe value is encountered and the {@link ngSanitize $sanitize} module is present this will sanitize the value instead of throwing an error. | * | `$sce.CSS` | For CSS that's safe to source into the application. Currently unused. Feel free to use it in your own directives. | * | `$sce.URL` | For URLs that are safe to follow as links. Currently unused (`
Note that `$sce.RESOURCE_URL` makes a stronger statement about the URL than `$sce.URL` does and therefore contexts requiring values trusted for `$sce.RESOURCE_URL` can be used anywhere that values trusted for `$sce.URL` are required. | + * | `$sce.RESOURCE_URL` | For URLs that are not only safe to follow as links, but whose contents are also safe to include in your application. Examples include `ng-include`, `src` / `ngSrc` bindings for tags other than `IMG`, `VIDEO`, `AUDIO` and `TRACK` (e.g. `IFRAME`, `OBJECT`, etc.)

Note that `$sce.RESOURCE_URL` makes a stronger statement about the URL than `$sce.URL` does and therefore contexts requiring values trusted for `$sce.RESOURCE_URL` can be used anywhere that values trusted for `$sce.URL` are required. | * | `$sce.JS` | For JavaScript that is safe to execute in your application's context. Currently unused. Feel free to use it in your own directives. | * * ## Format of items in {@link ng.$sceDelegateProvider#resourceUrlWhitelist resourceUrlWhitelist}/{@link ng.$sceDelegateProvider#resourceUrlBlacklist Blacklist} diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 800fb1be9d27..91ef2f8eac6d 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -10152,6 +10152,18 @@ describe('$compile', function() { expect(element.attr('src')).toEqual('http://example.com/image2.png'); })); + it('should NOT require trusted values for video / audio / track src', inject(function($rootScope, $compile, $sce) { + element = $compile('')($rootScope); + $rootScope.testUrl = 'http://example.com/image.mp4'; + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image.mp4'); + + // But it should accept trusted values anyway. + $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image2.mp4'); + })); + it('should not sanitize attributes other than src', inject(function($compile, $rootScope) { element = $compile('')($rootScope); $rootScope.testUrl = 'javascript:doEvilStuff()'; From 5839dcee277aaef585d73a512dfb3c2722b6f7f4 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Fri, 12 Feb 2016 14:57:01 +0100 Subject: [PATCH 2/8] fix($compileSpec): disable older msie for the video test and sort tests. Older msies reject my video element test, and format things better, no more tests for video[src] contexts under a img[src] sanitization describe. --- test/ng/compileSpec.js | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 91ef2f8eac6d..9f5960db2f7d 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -10138,8 +10138,7 @@ describe('$compile', function() { }); }); - - describe('img[src] sanitization', function() { + describe('*[src] context requirement', function() { it('should NOT require trusted values for img src', inject(function($rootScope, $compile, $sce) { element = $compile('')($rootScope); @@ -10152,17 +10151,24 @@ describe('$compile', function() { expect(element.attr('src')).toEqual('http://example.com/image2.png'); })); - it('should NOT require trusted values for video / audio / track src', inject(function($rootScope, $compile, $sce) { - element = $compile('')($rootScope); - $rootScope.testUrl = 'http://example.com/image.mp4'; - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image.mp4'); + // Older IEs seem to reject the video tag with "Error: Not implemented" + if (!msie || msie > 9) { + it('should NOT require trusted values for video src', + inject(function($rootScope, $compile, $sce) { + element = $compile('')($rootScope); + $rootScope.testUrl = 'http://example.com/image.mp4'; + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image.mp4'); - // But it should accept trusted values anyway. - $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image2.mp4'); - })); + // But it should accept trusted values anyway. + $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image2.mp4'); + })); + } + }); + + describe('img[src] sanitization', function() { it('should not sanitize attributes other than src', inject(function($compile, $rootScope) { element = $compile('')($rootScope); From ec31b3d9adf176475b414e9059ca233bdbe0cdb8 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 18 Feb 2016 16:44:55 +0100 Subject: [PATCH 3/8] refactor(compileSpec): Test retrocompatibility for video src. The context reduction didn't test retrocompatibility with trustAsResourceUrl(...) values. --- test/ng/compileSpec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 9f5960db2f7d..5e7992ee40ce 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -10164,6 +10164,11 @@ describe('$compile', function() { $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); $rootScope.$digest(); expect(element.attr('src')).toEqual('http://example.com/image2.mp4'); + + // and trustedResourceUrls for retrocompatibility + $rootScope.testUrl = $sce.trustAsResourceUrl('http://example.com/image3.mp4'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image3.mp4'); })); } }); From 02ce357f4757dd6d982d7311599bb82a5b15b6d9 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 18 Feb 2016 16:48:07 +0100 Subject: [PATCH 4/8] fix($compile): Bump track src from URL to RESOURCE_URL. Track files might contain CSS, and haven't been around for long. Keeping the high security context as a precaution is justified. --- src/ng/compile.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index dd3c8e2412d9..683946d48327 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3152,9 +3152,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } var tag = nodeName_(node); // All tags with src attributes require a RESOURCE_URL value, except for - // img and various html5 media tags. + // img and various html5 media tags. Note that track src allows files + // containing CSS, so leave that to RESOURCE_URL level. if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') { - if (['img', 'video', 'audio', 'track'].indexOf(tag) === -1) { + if (['img', 'video', 'audio'].indexOf(tag) === -1) { return $sce.RESOURCE_URL; } // maction[xlink:href] can source SVG. It's not limited to . From 511527507dde63d364ebb834b880ebf021395c23 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 18 Feb 2016 16:50:03 +0100 Subject: [PATCH 5/8] feat($compile): Reduce source src from RESOURCE_URL to URL context. Source src is only for media files (videos, audio, images), and present no known script execution possibilities. We also don't expect new ones to pop up, as this tags is only supported on recent browsers. --- src/ng/compile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 683946d48327..0cba94f03a16 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3155,7 +3155,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // img and various html5 media tags. Note that track src allows files // containing CSS, so leave that to RESOURCE_URL level. if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') { - if (['img', 'video', 'audio'].indexOf(tag) === -1) { + if (['img', 'video', 'audio', 'source'].indexOf(tag) === -1) { return $sce.RESOURCE_URL; } // maction[xlink:href] can source SVG. It's not limited to . From 02fdd5caeca95034ad49abdd779a5159285a6ace Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Mon, 22 Feb 2016 14:02:17 +0100 Subject: [PATCH 6/8] docs($sce): Update for the track src and source src security contexts. Forgot to update the $sce doc, now done. --- src/ng/sce.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/sce.js b/src/ng/sce.js index ac693fbf4c56..a530f2f379c3 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -537,7 +537,7 @@ function $SceDelegateProvider() { * | `$sce.HTML` | For HTML that's safe to source into the application. The {@link ng.directive:ngBindHtml ngBindHtml} directive uses this context for bindings. If an unsafe value is encountered and the {@link ngSanitize $sanitize} module is present this will sanitize the value instead of throwing an error. | * | `$sce.CSS` | For CSS that's safe to source into the application. Currently unused. Feel free to use it in your own directives. | * | `$sce.URL` | For URLs that are safe to follow as links. Currently unused (`
Note that `$sce.RESOURCE_URL` makes a stronger statement about the URL than `$sce.URL` does and therefore contexts requiring values trusted for `$sce.RESOURCE_URL` can be used anywhere that values trusted for `$sce.URL` are required. | + * | `$sce.RESOURCE_URL` | For URLs that are not only safe to follow as links, but whose contents are also safe to include in your application. Examples include `ng-include`, `src` / `ngSrc` bindings for tags other than `IMG`, `VIDEO`, `AUDIO` and `SOURCE` (e.g. `IFRAME`, `OBJECT`, etc.)

Note that `$sce.RESOURCE_URL` makes a stronger statement about the URL than `$sce.URL` does and therefore contexts requiring values trusted for `$sce.RESOURCE_URL` can be used anywhere that values trusted for `$sce.URL` are required. | * | `$sce.JS` | For JavaScript that is safe to execute in your application's context. Currently unused. Feel free to use it in your own directives. | * * ## Format of items in {@link ng.$sceDelegateProvider#resourceUrlWhitelist resourceUrlWhitelist}/{@link ng.$sceDelegateProvider#resourceUrlBlacklist Blacklist} From b775385bf2fc7138e58103988db149773552a3d1 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Wed, 17 Aug 2016 22:32:11 +0200 Subject: [PATCH 7/8] re-add link context, add tests for audio and source --- src/ng/compile.js | 4 +++- test/ng/compileSpec.js | 35 +++++++++++++++++++---------------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 0cba94f03a16..862de4683dc9 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3160,7 +3160,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } // maction[xlink:href] can source SVG. It's not limited to . } else if (attrNormalizedName === 'xlinkHref' || - (tag === 'form' && attrNormalizedName === 'action') + (tag === 'form' && attrNormalizedName === 'action') || + // links can be stylesheets or imports, which can run script in the current origin + (tag === 'link' && attrNormalizedName === 'href') ) { return $sce.RESOURCE_URL; } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 5e7992ee40ce..e8fb135307ad 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -10151,25 +10151,28 @@ describe('$compile', function() { expect(element.attr('src')).toEqual('http://example.com/image2.png'); })); - // Older IEs seem to reject the video tag with "Error: Not implemented" + // IE9 rejects the video / audio tag with "Error: Not implemented" and the source tag with + // "Unable to get value of the property 'childNodes': object is null or undefined" if (!msie || msie > 9) { - it('should NOT require trusted values for video src', - inject(function($rootScope, $compile, $sce) { - element = $compile('')($rootScope); - $rootScope.testUrl = 'http://example.com/image.mp4'; - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image.mp4'); + they('should NOT require trusted values for $prop src', ['video', 'audio', 'source'], + function(tag) { + inject(function($rootScope, $compile, $sce) { + element = $compile('<' + tag + ' src="{{testUrl}}">')($rootScope); + $rootScope.testUrl = 'http://example.com/image.mp4'; + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image.mp4'); - // But it should accept trusted values anyway. - $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image2.mp4'); + // But it should accept trusted values anyway. + $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image2.mp4'); - // and trustedResourceUrls for retrocompatibility - $rootScope.testUrl = $sce.trustAsResourceUrl('http://example.com/image3.mp4'); - $rootScope.$digest(); - expect(element.attr('src')).toEqual('http://example.com/image3.mp4'); - })); + // and trustedResourceUrls for retrocompatibility + $rootScope.testUrl = $sce.trustAsResourceUrl('http://example.com/image3.mp4'); + $rootScope.$digest(); + expect(element.attr('src')).toEqual('http://example.com/image3.mp4'); + }); + }); } }); From e9f2f1c86a0a6ed0abfa0e74af105d6319cfb066 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Wed, 24 Aug 2016 22:55:20 +0200 Subject: [PATCH 8/8] readd track, improve tests --- src/ng/compile.js | 5 ++--- test/ng/compileSpec.js | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 862de4683dc9..57f2b03d5feb 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3152,10 +3152,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } var tag = nodeName_(node); // All tags with src attributes require a RESOURCE_URL value, except for - // img and various html5 media tags. Note that track src allows files - // containing CSS, so leave that to RESOURCE_URL level. + // img and various html5 media tags. if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') { - if (['img', 'video', 'audio', 'source'].indexOf(tag) === -1) { + if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) { return $sce.RESOURCE_URL; } // maction[xlink:href] can source SVG. It's not limited to . diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index e8fb135307ad..d91dce5d5355 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -10154,7 +10154,7 @@ describe('$compile', function() { // IE9 rejects the video / audio tag with "Error: Not implemented" and the source tag with // "Unable to get value of the property 'childNodes': object is null or undefined" if (!msie || msie > 9) { - they('should NOT require trusted values for $prop src', ['video', 'audio', 'source'], + they('should NOT require trusted values for $prop src', ['video', 'audio'], function(tag) { inject(function($rootScope, $compile, $sce) { element = $compile('<' + tag + ' src="{{testUrl}}">')($rootScope); @@ -10173,6 +10173,26 @@ describe('$compile', function() { expect(element.attr('src')).toEqual('http://example.com/image3.mp4'); }); }); + + they('should NOT require trusted values for $prop src', ['source', 'track'], + function(tag) { + inject(function($rootScope, $compile, $sce) { + element = $compile('')($rootScope); + $rootScope.testUrl = 'http://example.com/image.mp4'; + $rootScope.$digest(); + expect(element.find(tag).attr('src')).toEqual('http://example.com/image.mp4'); + + // But it should accept trusted values anyway. + $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4'); + $rootScope.$digest(); + expect(element.find(tag).attr('src')).toEqual('http://example.com/image2.mp4'); + + // and trustedResourceUrls for retrocompatibility + $rootScope.testUrl = $sce.trustAsResourceUrl('http://example.com/image3.mp4'); + $rootScope.$digest(); + expect(element.find(tag).attr('src')).toEqual('http://example.com/image3.mp4'); + }); + }); } });