From 1863cf6be059cca52a8927017176a9e5b8b71bc1 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Fri, 12 Feb 2016 14:01:10 +0100 Subject: [PATCH 1/6] 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 | 12 ++++++++---- src/ng/sce.js | 2 +- test/ng/compileSpec.js | 12 ++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 3db3fc9f7985..c19f2244da96 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2808,11 +2808,15 @@ 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") || - (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 2cd1198390c4..a90648f4e17d 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -533,7 +533,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 83c6c8bf8955..97101709cfda 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -8780,6 +8780,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) { /* jshint scripturl:true */ element = $compile('')($rootScope); From 4be150c5897e4caaff71100554445d99a07c2ca9 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Fri, 12 Feb 2016 14:57:01 +0100 Subject: [PATCH 2/6] 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 97101709cfda..ab185a7b4703 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -8766,8 +8766,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); @@ -8780,17 +8779,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) { /* jshint scripturl:true */ From ba6fc085d30b3525bc76a671cad331867e9f5f84 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 18 Feb 2016 16:44:55 +0100 Subject: [PATCH 3/6] 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 ab185a7b4703..34f78ebb4000 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -8792,6 +8792,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 a0ea9ebafbb18b7b7e50acd22b5f3fb328c2acd8 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 18 Feb 2016 16:48:07 +0100 Subject: [PATCH 4/6] 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 c19f2244da96..8b3542d03518 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2809,9 +2809,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 0f76c625e64063e80627c747fcc098ded3aba418 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Thu, 18 Feb 2016 16:50:03 +0100 Subject: [PATCH 5/6] 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 8b3542d03518..f46925b1bf7b 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2812,7 +2812,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 e9083f37df9ab2a7a74aaaa735b235745d2102f8 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Mon, 22 Feb 2016 14:02:17 +0100 Subject: [PATCH 6/6] 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 a90648f4e17d..761595f1b2eb 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -533,7 +533,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}