From 38deedd6e3d806eb8262bb43f26d47245f6c2739 Mon Sep 17 00:00:00 2001
From: Chirayu Krishnappa <chirayu@chirayuk.com>
Date: Fri, 21 Jun 2013 19:00:45 -0700
Subject: [PATCH] fix($compile): reject multi-expression interpolations for src
 attribute

BREAKING CHANGE: Concatenating expressions makes it hard to reason about
    whether some combination of concatenated values are unsafe to use
    and could easily lead to XSS.  By requiring that a single expression
    be used for *[src/ng-src] such as iframe[src], object[src], etc.
    (but not img[src/ng-src] since that value is sanitized), we ensure that the value
    that's used is assigned or constructed by some JS code somewhere
    that is more testable or make it obvious that you bound the value to
    some user controlled value.  This helps reduce the load when
    auditing for XSS issues.

    To migrate your code, follow the example below:

        Before:
            JS:
                scope.baseUrl = 'page';
                scope.a = 1;
                scope.b = 2;
            HTML:
                <!-- Are a and b properly escaped here? Is baseUrl
                     controlled by user? -->
                <iframe src="{{baseUrl}}?a={{a}&b={{b}}">

        After:
            JS:
                var baseUrl = "page";
                scope.getIframeSrc = function() {
                  // There are obviously better ways to do this.  The
                  // key point is that one will think about this and do
                  // it the right way.
                  var qs = ["a", "b"].map(function(value, name) {
                      return encodeURIComponent(name) + "=" +
                             encodeURIComponent(value);
                    }).join("&");
                  // baseUrl isn't on scope so it isn't bound to a user
                  // controlled value.
                  return baseUrl + "?" + qs;
                }
            HTML: <iframe src="{{getIframeSrc()}}">
---
 src/ng/compile.js                     | 12 +++++++++++-
 src/ng/interpolate.js                 | 24 +++++++++++++++++++++--
 test/ng/directive/booleanAttrsSpec.js | 28 ++++++++++++++++++++++++---
 test/ng/interpolateSpec.js            | 24 +++++++++++++++++++++++
 4 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/src/ng/compile.js b/src/ng/compile.js
index 91844f45e358..caa4e8ce547e 100644
--- a/src/ng/compile.js
+++ b/src/ng/compile.js
@@ -1157,6 +1157,16 @@ function $CompileProvider($provide) {
     }
 
 
+    function isTrustedContext(node, attrNormalizedName) {
+      // maction[xlink:href] can source SVG.  It's not limited to <maction>.
+      if (attrNormalizedName == "xlinkHref" ||
+          (nodeName_(node) != "IMG" && (attrNormalizedName == "src" ||
+                                        attrNormalizedName == "ngSrc"))) {
+        return true;
+      }
+    }
+
+
     function addAttrInterpolateDirective(node, directives, value, name) {
       var interpolateFn = $interpolate(value, true);
 
@@ -1177,7 +1187,7 @@ function $CompileProvider($provide) {
 
           // we need to interpolate again, in case the attribute value has been updated
           // (e.g. by another directive's compile function)
-          interpolateFn = $interpolate(attr[name], true);
+          interpolateFn = $interpolate(attr[name], true, isTrustedContext(node, name));
 
           // if attribute was updated so that there is no interpolation going on we don't want to
           // register any observers
diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js
index 51f4630d81a9..8e94fe24cc96 100644
--- a/src/ng/interpolate.js
+++ b/src/ng/interpolate.js
@@ -1,5 +1,7 @@
 'use strict';
 
+var $interpolateMinErr = minErr('$interpolate');
+
 /**
  * @ngdoc object
  * @name ng.$interpolateProvider
@@ -82,6 +84,12 @@ function $InterpolateProvider() {
      * @param {boolean=} mustHaveExpression if set to true then the interpolation string must have
      *    embedded expression in order to return an interpolation function. Strings with no
      *    embedded expression will return null for the interpolation function.
+     * @param {boolean=} isTrustedContext when true, requires that the interpolation string does not
+     *    contain any concatenations - i.e. the interpolation string is a single expression.
+     *    Interpolations for *[src] and *[ng-src] (except IMG, since itwhich sanitizes its value)
+     *    pass true for this parameter.  This helps avoid hunting through the template code to
+     *    figure out of some iframe[src], object[src], etc. was interpolated with a concatenation
+     *    that ended up introducing a XSS.
      * @returns {function(context)} an interpolation function which is used to compute the interpolated
      *    string. The function has these parameters:
      *
@@ -89,7 +97,7 @@ function $InterpolateProvider() {
      *      against.
      *
      */
-    function $interpolate(text, mustHaveExpression) {
+    function $interpolate(text, mustHaveExpression, isTrustedContext) {
       var startIndex,
           endIndex,
           index = 0,
@@ -121,6 +129,18 @@ function $InterpolateProvider() {
         length = 1;
       }
 
+      // Concatenating expressions makes it hard to reason about whether some combination of concatenated
+      // values are unsafe to use and could easily lead to XSS.  By requiring that a single
+      // expression be used for iframe[src], object[src], etc., we ensure that the value that's used
+      // is assigned or constructed by some JS code somewhere that is more testable or make it
+      // obvious that you bound the value to some user controlled value.  This helps reduce the load
+      // when auditing for XSS issues.
+      if (isTrustedContext && parts.length > 1) {
+          throw $interpolateMinErr('noconcat',
+              "Error while interpolating: {0}\nYou may not use multiple expressions when " +
+              "interpolating this expression.", text);
+      }
+
       if (!mustHaveExpression  || hasInterpolation) {
         concat.length = length;
         fn = function(context) {
@@ -139,7 +159,7 @@ function $InterpolateProvider() {
             return concat.join('');
           }
           catch(err) {
-            var newErr = minErr('$interpolate')('interr', "Can't interpolate: {0}\n{1}", text, err.toString());
+            var newErr = $interpolateMinErr('interr', "Can't interpolate: {0}\n{1}", text, err.toString());
             $exceptionHandler(newErr);
           }
         };
diff --git a/test/ng/directive/booleanAttrsSpec.js b/test/ng/directive/booleanAttrsSpec.js
index aa48afffdb85..f2ea003c9a97 100644
--- a/test/ng/directive/booleanAttrsSpec.js
+++ b/test/ng/directive/booleanAttrsSpec.js
@@ -89,19 +89,41 @@ describe('boolean attr directives', function() {
 
 describe('ngSrc', function() {
   it('should interpolate the expression and bind to src', inject(function($compile, $rootScope) {
-    var element = $compile('<div ng-src="some/{{id}}"></div>')($rootScope);
+    var element = $compile('<div ng-src="{{id}}"></div>')($rootScope);
 
     $rootScope.$digest();
-    expect(element.attr('src')).toEqual('some/');
+    expect(element.attr('src')).toBeUndefined();
 
     $rootScope.$apply(function() {
       $rootScope.id = 1;
     });
-    expect(element.attr('src')).toEqual('some/1');
+    expect(element.attr('src')).toEqual('1');
 
     dealoc(element);
   }));
 
+  describe('isTrustedContext', function() {
+    it('should NOT interpolate a multi-part expression for non-img src attribute', inject(function($compile, $rootScope) {
+      expect(function() {
+          var element = $compile('<div ng-src="some/{{id}}"></div>')($rootScope);
+          dealoc(element);
+        }).toThrow(
+            "[$interpolate:noconcat] Error while interpolating: some/{{id}}\nYou may not use " +
+            "multiple expressions when interpolating this expression.");
+    }));
+
+    it('should interpolate a multi-part expression for regular attributes', inject(function($compile, $rootScope) {
+      var element = $compile('<div foo="some/{{id}}"></div>')($rootScope);
+      $rootScope.$digest();
+      expect(element.attr('foo')).toBe('some/');
+      $rootScope.$apply(function() {
+        $rootScope.id = 1;
+      });
+      expect(element.attr('foo')).toEqual('some/1');
+    }));
+
+  });
+
   if (msie) {
     it('should update the element property as well as the attribute', inject(
         function($compile, $rootScope) {
diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js
index 454d81aae7ba..7569c0e2e96b 100644
--- a/test/ng/interpolateSpec.js
+++ b/test/ng/interpolateSpec.js
@@ -149,6 +149,29 @@ describe('$interpolate', function() {
   });
 
 
+  describe('isTrustedContext', function() {
+    it('should NOT interpolate a multi-part expression when isTrustedContext is true', inject(function($interpolate) {
+      var isTrustedContext = true;
+      expect(function() {
+          $interpolate('constant/{{var}}', true, isTrustedContext);
+        }).toThrow(
+            "[$interpolate:noconcat] Error while interpolating: constant/{{var}}\nYou may not use " +
+            "multiple expressions when interpolating this expression.");
+      expect(function() {
+          $interpolate('{{foo}}{{bar}}', true, isTrustedContext);
+        }).toThrow(
+            "[$interpolate:noconcat] Error while interpolating: {{foo}}{{bar}}\nYou may not use " +
+            "multiple expressions when interpolating this expression.");
+    }));
+
+    it('should interpolate a multi-part expression when isTrustedContext is false', inject(function($interpolate) {
+      expect($interpolate('some/{{id}}')()).toEqual('some/');
+      expect($interpolate('some/{{id}}')({id: 1})).toEqual('some/1');
+      expect($interpolate('{{foo}}{{bar}}')({foo: 1, bar: 2})).toEqual('12');
+    }));
+  });
+
+
   describe('startSymbol', function() {
 
     beforeEach(module(function($interpolateProvider) {
@@ -199,4 +222,5 @@ describe('$interpolate', function() {
       expect($interpolate.endSymbol()).toBe('))');
     }));
   });
+
 });