fix($parse, events): prevent accidental misuse of properties on $event

Author: chirayuk

Diff for: src/ng/directive/ngEventDirs.js

@@ -53,7 +53,11 @@ forEach(
       return {
         restrict: 'A',
         compile: function($element, attr) {
-          var fn = $parse(attr[directiveName]);
+          // We expose the powerful $event object on the scope that provides access to the Window,
+          // etc. that isn't protected by the fast paths in $parse.  We explicitly request better
+          // checks at the cost of speed since event handler expressions are not executed as
+          // frequently as regular change detection.
+          var fn = $parse(attr[directiveName], /* interceptorFn */ null, /* expensiveChecks */ true);
           return function ngEventHandler(scope, element) {
             element.on(eventName, function(event) {
               var callback = function() {

Diff for: src/ng/parse.js

@@ -852,7 +852,8 @@ function setter(obj, path, setValue, fullExp) {
   return setValue;
 }
 
-var getterFnCache = createMap();
+var getterFnCacheDefault = createMap();
+var getterFnCacheExpensive = createMap();
 
 function isPossiblyDangerousMemberName(name) {
   return name == 'constructor';
@@ -863,7 +864,7 @@ function isPossiblyDangerousMemberName(name) {
  * - http://jsperf.com/angularjs-parse-getter/4
  * - http://jsperf.com/path-evaluation-simplified/7
  */
-function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
+function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, expensiveChecks) {
   ensureSafeMemberName(key0, fullExp);
   ensureSafeMemberName(key1, fullExp);
   ensureSafeMemberName(key2, fullExp);
@@ -872,11 +873,11 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
   var eso = function(o) {
     return ensureSafeObject(o, fullExp);
   };
-  var eso0 = isPossiblyDangerousMemberName(key0) ? eso : identity;
-  var eso1 = isPossiblyDangerousMemberName(key1) ? eso : identity;
-  var eso2 = isPossiblyDangerousMemberName(key2) ? eso : identity;
-  var eso3 = isPossiblyDangerousMemberName(key3) ? eso : identity;
-  var eso4 = isPossiblyDangerousMemberName(key4) ? eso : identity;
+  var eso0 = (expensiveChecks || isPossiblyDangerousMemberName(key0)) ? eso : identity;
+  var eso1 = (expensiveChecks || isPossiblyDangerousMemberName(key1)) ? eso : identity;
+  var eso2 = (expensiveChecks || isPossiblyDangerousMemberName(key2)) ? eso : identity;
+  var eso3 = (expensiveChecks || isPossiblyDangerousMemberName(key3)) ? eso : identity;
+  var eso4 = (expensiveChecks || isPossiblyDangerousMemberName(key4)) ? eso : identity;
 
   return function cspSafeGetter(scope, locals) {
     var pathVal = (locals && locals.hasOwnProperty(key0)) ? locals : scope;
@@ -911,23 +912,25 @@ function getterFnWithEnsureSafeObject(fn, fullExpression) {
 }
 
 function getterFn(path, options, fullExp) {
+  var expensiveChecks = options.expensiveChecks;
+  var getterFnCache = (expensiveChecks ? getterFnCacheExpensive : getterFnCacheDefault);
   var fn = getterFnCache[path];
-
   if (fn) return fn;
 
+
   var pathKeys = path.split('.'),
       pathKeysLength = pathKeys.length;
 
   // http://jsperf.com/angularjs-parse-getter/6
   if (options.csp) {
     if (pathKeysLength < 6) {
-      fn = cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp);
+      fn = cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp, expensiveChecks);
     } else {
       fn = function cspSafeGetter(scope, locals) {
         var i = 0, val;
         do {
           val = cspSafeGetterFn(pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++],
-                                pathKeys[i++], fullExp)(scope, locals);
+                                pathKeys[i++], fullExp, expensiveChecks)(scope, locals);
 
           locals = undefined; // clear after first iteration
           scope = val;
@@ -937,15 +940,18 @@ function getterFn(path, options, fullExp) {
     }
   } else {
     var code = '';
-    var needsEnsureSafeObject = false;
+    if (expensiveChecks) {
+      code += 's = eso(s, fe);\nl = eso(l, fe);\n';
+    }
+    var needsEnsureSafeObject = expensiveChecks;
     forEach(pathKeys, function(key, index) {
       ensureSafeMemberName(key, fullExp);
       var lookupJs = (index
                       // we simply dereference 's' on any .dot notation
                       ? 's'
                       // but if we are first then we check locals first, and if so read it first
                       : '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '.' + key;
-      if (isPossiblyDangerousMemberName(key)) {
+      if (expensiveChecks || isPossiblyDangerousMemberName(key)) {
         lookupJs = 'eso(' + lookupJs + ', fe)';
         needsEnsureSafeObject = true;
       }
@@ -1030,15 +1036,20 @@ function getValueOf(value) {
  *  service.
  */
 function $ParseProvider() {
-  var cache = createMap();
+  var cacheDefault = createMap();
+  var cacheExpensive = createMap();
 
-  var $parseOptions = {
-    csp: false
-  };
 
 
   this.$get = ['$filter', '$sniffer', function($filter, $sniffer) {
-    $parseOptions.csp = $sniffer.csp;
+    var $parseOptions = {
+          csp: $sniffer.csp,
+          expensiveChecks: false
+        },
+        $parseOptionsExpensive = {
+          csp: $sniffer.csp,
+          expensiveChecks: true
+        };
 
     function wrapSharedExpression(exp) {
       var wrapped = exp;
@@ -1055,13 +1066,14 @@ function $ParseProvider() {
       return wrapped;
     }
 
-    return function $parse(exp, interceptorFn) {
+    return function $parse(exp, interceptorFn, expensiveChecks) {
       var parsedExpression, oneTime, cacheKey;
 
       switch (typeof exp) {
         case 'string':
           cacheKey = exp = exp.trim();
 
+          var cache = (expensiveChecks ? cacheExpensive : cacheDefault);
           parsedExpression = cache[cacheKey];
 
           if (!parsedExpression) {
@@ -1070,8 +1082,9 @@ function $ParseProvider() {
               exp = exp.substring(2);
             }
 
-            var lexer = new Lexer($parseOptions);
-            var parser = new Parser(lexer, $filter, $parseOptions);
+            var parseOptions = expensiveChecks ? $parseOptionsExpensive : $parseOptions;
+            var lexer = new Lexer(parseOptions);
+            var parser = new Parser(lexer, $filter, parseOptions);
             parsedExpression = parser.parse(exp);
 
             if (parsedExpression.constant) {

Diff for: test/ng/directive/ngEventDirsSpec.js

@@ -90,6 +90,25 @@ describe('event directives', function() {
 
   });
 
+  describe('security', function() {
+    it('should allow access to the $event object', inject(function($rootScope, $compile) {
+      var scope = $rootScope.$new();
+      element = $compile('<button ng-click="e = $event">BTN</button>')(scope);
+      element.triggerHandler('click');
+      expect(scope.e.target).toBe(element[0]);
+    }));
+
+    it('should block access to DOM nodes (e.g. exposed via $event)', inject(function($rootScope, $compile) {
+      var scope = $rootScope.$new();
+      element = $compile('<button ng-click="e = $event.target">BTN</button>')(scope);
+      expect(function() {
+        element.triggerHandler('click');
+      }).toThrowMinErr(
+              '$parse', 'isecdom', 'Referencing DOM nodes in Angular expressions is disallowed! ' +
+              'Expression: e = $event.target');
+    }));
+  });
+
   describe('blur', function() {
 
     describe('call the listener asynchronously during $apply', function() {

Diff for: test/ng/parseSpec.js

@@ -3,9 +3,11 @@
 describe('parser', function() {
 
   beforeEach(function() {
-    /* global getterFnCache: true */
-    // clear cache
-    getterFnCache = createMap();
+    /* global getterFnCacheDefault: true */
+    /* global getterFnCacheExpensive: true */
+    // clear caches
+    getterFnCacheDefault = createMap();
+    getterFnCacheExpensive = createMap();
   });
 
 
@@ -783,6 +785,22 @@ describe('parser', function() {
                     'Expression: foo["bar"]');
 
           });
+
+          describe('expensiveChecks', function() {
+            it('should block access to window object even when aliased', inject(function($parse, $window) {
+              scope.foo = {w: $window};
+              // This isn't blocked for performance.
+              expect(scope.$eval($parse('foo.w'))).toBe($window);
+              // Event handlers use the more expensive path for better protection since they expose
+              // the $event object on the scope.
+              expect(function() {
+                scope.$eval($parse('foo.w', null, true));
+              }).toThrowMinErr(
+                      '$parse', 'isecwindow', 'Referencing the Window in Angular expressions is disallowed! ' +
+                      'Expression: foo.w');
+
+            }));
+          });
         });
 
         describe('Function prototype functions', function() {