fix($parse): add quick check for Function constructor in fast path

2014-11-03 19:14:58 -08:00
parent d87b7912df
commit 756640f5aa
2 changed files with 79 additions and 34 deletions
@@ -878,6 +878,10 @@ function setter(obj, path, setValue, fullExp, options) {

 var getterFnCache = {};

+function isPossiblyDangerousMemberName(name) {
+  return name == 'constructor';
+}
+
 /**
 * Implementation of the "Black Hole" variant from:
 * - http://jsperf.com/angularjs-parse-getter/4
@@ -889,29 +893,37 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
  ensureSafeMemberName(key2, fullExp);
  ensureSafeMemberName(key3, fullExp);
  ensureSafeMemberName(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;

  return !options.unwrapPromises
      ? function cspSafeGetter(scope, locals) {
          var pathVal = (locals && locals.hasOwnProperty(key0)) ? locals : scope;

          if (pathVal == null) return pathVal;
-          pathVal = pathVal[key0];
+          pathVal = eso0(pathVal[key0]);

          if (!key1) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key1];
+          pathVal = eso1(pathVal[key1]);

          if (!key2) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key2];
+          pathVal = eso2(pathVal[key2]);

          if (!key3) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key3];
+          pathVal = eso3(pathVal[key3]);

          if (!key4) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key4];
+          pathVal = eso4(pathVal[key4]);

          return pathVal;
        }
@@ -921,72 +933,78 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {

          if (pathVal == null) return pathVal;

-          pathVal = pathVal[key0];
+          pathVal = eso0(pathVal[key0]);
          if (pathVal && pathVal.then) {
            promiseWarning(fullExp);
            if (!("$$v" in pathVal)) {
              promise = pathVal;
              promise.$$v = undefined;
-              promise.then(function(val) { promise.$$v = val; });
+              promise.then(function(val) { promise.$$v = eso0(val); });
            }
-            pathVal = pathVal.$$v;
+            pathVal = eso0(pathVal.$$v);
          }

          if (!key1) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key1];
+          pathVal = eso1(pathVal[key1]);
          if (pathVal && pathVal.then) {
            promiseWarning(fullExp);
            if (!("$$v" in pathVal)) {
              promise = pathVal;
              promise.$$v = undefined;
-              promise.then(function(val) { promise.$$v = val; });
+              promise.then(function(val) { promise.$$v = eso1(val); });
            }
-            pathVal = pathVal.$$v;
+            pathVal = eso1(pathVal.$$v);
          }

          if (!key2) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key2];
+          pathVal = eso2(pathVal[key2]);
          if (pathVal && pathVal.then) {
            promiseWarning(fullExp);
            if (!("$$v" in pathVal)) {
              promise = pathVal;
              promise.$$v = undefined;
-              promise.then(function(val) { promise.$$v = val; });
+              promise.then(function(val) { promise.$$v = eso2(val); });
            }
-            pathVal = pathVal.$$v;
+            pathVal = eso2(pathVal.$$v);
          }

          if (!key3) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key3];
+          pathVal = eso3(pathVal[key3]);
          if (pathVal && pathVal.then) {
            promiseWarning(fullExp);
            if (!("$$v" in pathVal)) {
              promise = pathVal;
              promise.$$v = undefined;
-              promise.then(function(val) { promise.$$v = val; });
+              promise.then(function(val) { promise.$$v = eso3(val); });
            }
-            pathVal = pathVal.$$v;
+            pathVal = eso3(pathVal.$$v);
          }

          if (!key4) return pathVal;
          if (pathVal == null) return undefined;
-          pathVal = pathVal[key4];
+          pathVal = eso4(pathVal[key4]);
          if (pathVal && pathVal.then) {
            promiseWarning(fullExp);
            if (!("$$v" in pathVal)) {
              promise = pathVal;
              promise.$$v = undefined;
-              promise.then(function(val) { promise.$$v = val; });
+              promise.then(function(val) { promise.$$v = eso4(val); });
            }
-            pathVal = pathVal.$$v;
+            pathVal = eso4(pathVal.$$v);
          }
          return pathVal;
        };
 }

+function getterFnWithExtraArgs(fn, fullExpression) {
+  return function(s, l) {
+    return fn(s, l, promiseWarning, ensureSafeObject, fullExpression);
+  };
+}
+
 function getterFn(path, options, fullExp) {
  // Check whether the cache has this getter already.
  // We can use hasOwnProperty directly on the cache because we ensure,
@@ -1019,35 +1037,45 @@ function getterFn(path, options, fullExp) {
    }
  } else {
    var code = 'var p;\n';
+    var needsEnsureSafeObject = false;
    forEach(pathKeys, function(key, index) {
      ensureSafeMemberName(key, fullExp);
-      code += 'if(s == null) return undefined;\n' +
-              's='+ (index
+      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
-                      : '((k&&k.hasOwnProperty("' + key + '"))?k:s)') + '["' + key + '"]' + ';\n' +
-              (options.unwrapPromises
-                ? 'if (s && s.then) {\n' +
+                      : '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '["' + key + '"]';
+      var wrapWithEso = isPossiblyDangerousMemberName(key);
+      if (wrapWithEso) {
+        lookupJs = 'eso(' + lookupJs + ', fe)';
+        needsEnsureSafeObject = true;
+      }
+      code += 'if(s == null) return undefined;\n' +
+              's=' + lookupJs + ';\n';
+      if (options.unwrapPromises) {
+        code += 'if (s && s.then) {\n' +
                  ' pw("' + fullExp.replace(/(["\r\n])/g, '\\$1') + '");\n' +
                  ' if (!("$$v" in s)) {\n' +
                    ' p=s;\n' +
                    ' p.$$v = undefined;\n' +
-                    ' p.then(function(v) {p.$$v=v;});\n' +
+                    ' p.then(function(v) {p.$$v=' + (wrapWithEso ? 'eso(v)' : 'v') + ';});\n' +
                    '}\n' +
-                  ' s=s.$$v\n' +
-                '}\n'
-                : '');
+                  ' s=' + (wrapWithEso ? 'eso(s.$$v)' : 's.$$v') + '\n' +
+                '}\n';
+
+      }
    });
    code += 'return s;';

    /* jshint -W054 */
-    var evaledFnGetter = new Function('s', 'k', 'pw', code); // s=scope, k=locals, pw=promiseWarning
+    // s=scope, l=locals, pw=promiseWarning, eso=ensureSafeObject, fe=fullExpression
+    var evaledFnGetter = new Function('s', 'l', 'pw', 'eso', 'fe', code);
    /* jshint +W054 */
    evaledFnGetter.toString = valueFn(code);
-    fn = options.unwrapPromises ? function(scope, locals) {
-      return evaledFnGetter(scope, locals, promiseWarning);
-    } : evaledFnGetter;
+    if (needsEnsureSafeObject || options.unwrapPromises) {
+      evaledFnGetter = getterFnWithExtraArgs(evaledFnGetter, fullExp);
+    }
+    fn = evaledFnGetter;
  }

  // Only cache the value if it's not going to mess up the cache object
@@ -649,9 +649,26 @@ describe('parser', function() {
          expect(scope.$eval('a + \n b.c + \r "\td" + \t \r\n\r "\r\n\n"')).toEqual("abc\td\r\n\n");
        });

-
        describe('sandboxing', function() {
          describe('Function constructor', function() {
+            it('should not tranverse the Function constructor in the getter', function() {
+              expect(function() {
+                scope.$eval('{}.toString.constructor');
+              }).toThrowMinErr(
+                      '$parse', 'isecfn', 'Referencing Function in Angular expressions is disallowed! ' +
+                      'Expression: {}.toString.constructor');
+
+            });
+
+            it('should not allow access to the Function prototype in the getter', function() {
+              expect(function() {
+                scope.$eval('toString.constructor.prototype');
+              }).toThrowMinErr(
+                      '$parse', 'isecfn', 'Referencing Function in Angular expressions is disallowed! ' +
+                      'Expression: toString.constructor.prototype');
+
+            });
+
            it('should NOT allow access to Function constructor in getter', function() {

              expect(function() {