From 756640f5aa8f3fd0084bff50534e23976a6fff00 Mon Sep 17 00:00:00 2001 From: Chirayu Krishnappa Date: Mon, 3 Nov 2014 19:14:58 -0800 Subject: [PATCH] fix($parse): add quick check for Function constructor in fast path --- src/ng/parse.js | 94 ++++++++++++++++++++++++++++---------------- test/ng/parseSpec.js | 19 ++++++++- 2 files changed, 79 insertions(+), 34 deletions(-) diff --git a/src/ng/parse.js b/src/ng/parse.js index b48e589b8..85191539c 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -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 diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index 3fcf71561..faac169c1 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -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() {