From 714bddba24bb392902a906b1c70d7f26c531604d Mon Sep 17 00:00:00 2001 From: maxwellium Date: Sun, 10 Nov 2013 15:11:09 +0100 Subject: [PATCH] Revert "feat($parse): secure expressions by hiding "private" properties" Reverting change introduced by chirayuk which needs further discussion. Check line 29 (26 after commit): "The goal is to prevent exploits against the expression language, but not to prevent exploits that were enabled by exposing sensitive JavaScript or browser apis on Scope." Chirayuk's commit enforces convention to regard all properties beginning or ending with underscore as private. Breaks every nonSQL db implementation i know, forcing overhead code. While white-/blacklisting accessible properties for expressions is a nice feat, this is imho the wrong approach and does not add much in terms of security. An attacker at this level is able to read everything on scope. The advocated change and the respective tests convey a false sense of security to unaware devs and will lead to complacency. This reverts commit 3d6a89e8888b14ae5cb5640464e12b7811853c7e. --- docs/content/error/parse/isecprv.ngdoc | 50 ------------------- src/ng/parse.js | 30 +++--------- test/ng/parseSpec.js | 67 +------------------------- 3 files changed, 10 insertions(+), 137 deletions(-) delete mode 100644 docs/content/error/parse/isecprv.ngdoc diff --git a/docs/content/error/parse/isecprv.ngdoc b/docs/content/error/parse/isecprv.ngdoc deleted file mode 100644 index 4bb02426073a..000000000000 --- a/docs/content/error/parse/isecprv.ngdoc +++ /dev/null @@ -1,50 +0,0 @@ -@ngdoc error -@name $parse:isecprv -@fullName Referencing private Field in Expression - -@description - -Occurs when an Angular expression attempts to access a private field. - -Fields with names that begin or end with an underscore are considered -private fields.  Angular expressions are not allowed to reference such -fields on the scope chain.  This only applies to Angular expressions -(e.g. {{ }} interpolation and calls to `$parse` with a string expression -argument) – Javascript itself has no such notion. - -To resolve this error, use an alternate non-private field if available -or make the field public (by removing any leading and trailing -underscore characters from its name.) - -Example expression that would result in this error: - -```html -
{{user._private_field}}
-``` - -Background: -Though Angular expressions are written and controlled by the developer -and are trusted, they do represent an attack surface due to the -following two factors: - -- they typically deal with user input which is generally high risk -- they often don't get the kind of attention and test coverage that - JavaScript code would. - -If these expression were evaluated in a context with full trust, an -attacker, though unable to change the expression itself, can feed it -unexpected and dangerous input that could result in a security -breach/exploit. - -As such, Angular expressions are evaluated in a limited context.  They -do not have direct access to the global scope, Window, Document, the -Function constructor or "private" properties (names beginning or ending -with an underscore character) on the scope chain.  They should get their -work done via public properties and methods exposed on the scope chain -(keep in mind that this includes controllers as well as they are -published on the scope via the "controller as" syntax.) - -As a best practise, only "publish" properties on the scopes and -controllers that must be available to Angular expressions.  All other -members should either be in closures or be "private" by giving them -names with a leading or trailing underscore character. diff --git a/src/ng/parse.js b/src/ng/parse.js index eeb60c4e4e1f..2c7a19263dcd 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -17,14 +17,11 @@ var promiseWarning; // {}.toString.constructor(alert("evil JS code")) // // We want to prevent this type of access. For the sake of performance, during the lexing phase we -// disallow any "dotted" access to any member named "constructor" or to any member whose name begins -// or ends with an underscore. The latter allows one to exclude the private / JavaScript only API -// available on the scope and controllers from the context of an Angular expression. +// disallow any "dotted" access to any member named "constructor". // -// For reflective calls (a[b]), we check that the value of the lookup is not the Function -// constructor, Window or DOM node while evaluating the expression, which is a stronger but more -// expensive test. Since reflective calls are expensive anyway, this is not such a big deal compared -// to static dereferencing. +// For reflective calls (a[b]) we check that the value of the lookup is not the Function constructor +// while evaluating the expression, which is a stronger but more expensive test. Since reflective +// calls are expensive anyway, this is not such a big deal compared to static dereferencing. // // This sandboxing technique is not perfect and doesn't aim to be. The goal is to prevent exploits // against the expression language, but not to prevent exploits that were enabled by exposing @@ -38,20 +35,12 @@ var promiseWarning; // In general, it is not possible to access a Window object from an angular expression unless a // window or some DOM object that has a reference to window is published onto a Scope. -function ensureSafeMemberName(name, fullExpression, allowConstructor) { - if (typeof name !== 'string' && toString.apply(name) !== "[object String]") { - return name; - } - if (name === "constructor" && !allowConstructor) { +function ensureSafeMemberName(name, fullExpression) { + if (name === "constructor") { throw $parseMinErr('isecfld', 'Referencing "constructor" field in Angular expressions is disallowed! Expression: {0}', fullExpression); } - if (name.charAt(0) === '_' || name.charAt(name.length-1) === '_') { - throw $parseMinErr('isecprv', - 'Referencing private fields in Angular expressions is disallowed! Expression: {0}', - fullExpression); - } return name; } @@ -735,10 +724,7 @@ Parser.prototype = { return extend(function(self, locals) { var o = obj(self, locals), - // In the getter, we will not block looking up "constructor" by name in order to support user defined - // constructors. However, if value looked up is the Function constructor, we will still block it in the - // ensureSafeObject call right after we look up o[i] (a few lines below.) - i = ensureSafeMemberName(indexFn(self, locals), parser.text, true /* allowConstructor */), + i = indexFn(self, locals), v, p; if (!o) return undefined; @@ -754,7 +740,7 @@ Parser.prototype = { return v; }, { assign: function(self, value, locals) { - var key = ensureSafeMemberName(indexFn(self, locals), parser.text); + var key = indexFn(self, locals); // prevent overwriting of Function.constructor which would break ensureSafeObject check var safe = ensureSafeObject(obj(self, locals), parser.text); return safe[key] = value; diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index c72b7e818749..d7d0d94169de 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -591,57 +591,6 @@ describe('parser', function() { }); describe('sandboxing', function() { - describe('private members', function() { - it('should NOT allow access to private members', function() { - forEach(['_name', 'name_', '_', '_name_'], function(name) { - function _testExpression(expression) { - scope.a = {b: name}; - scope[name] = {a: scope.a}; - scope.piece_1 = "XX" + name.charAt(0) + "XX"; - scope.piece_2 = "XX" + name.substr(1) + "XX"; - expect(function() { - scope.$eval(expression); - }).toThrowMinErr( - '$parse', 'isecprv', 'Referencing private fields in Angular expressions is disallowed! ' + - 'Expression: ' + expression); - } - - function testExpression(expression) { - if (expression.indexOf('"NAME"') != -1) { - var concatExpr = 'piece_1.substr(2, 1) + piece_2.substr(2, LEN)'.replace('LEN', name.length-1); - _testExpression(expression.replace(/"NAME"/g, concatExpr)); - _testExpression(expression.replace(/"NAME"/g, '(' + concatExpr + ')')); - } - _testExpression(expression.replace(/NAME/g, name)); - } - - // Not all of these are exploitable. The tests ensure that the contract is honored - // without caring about the implementation or exploitability. - testExpression('NAME'); testExpression('NAME = 1'); - testExpression('(NAME)'); testExpression('(NAME) = 1'); - testExpression('a.NAME'); testExpression('a.NAME = 1'); - testExpression('NAME.b'); testExpression('NAME.b = 1'); - testExpression('a.NAME.b'); testExpression('a.NAME.b = 1'); - testExpression('NAME()'); testExpression('NAME() = 1'); - testExpression('(NAME)()'); testExpression('(NAME = 1)()'); - testExpression('(NAME).foo()'); testExpression('(NAME = 1).foo()'); - testExpression('a.NAME()'); testExpression('a.NAME() = 1'); - testExpression('a.NAME.foo()'); testExpression('a.NAME.foo()'); - testExpression('foo(NAME)'); testExpression('foo(NAME = 1)'); - testExpression('foo(a.NAME)'); testExpression('foo(a.NAME = 1)'); - testExpression('foo(1, a.NAME)'); testExpression('foo(1, a.NAME = 1)'); - testExpression('foo(a["NAME"])'); testExpression('foo(a["NAME"] = 1)'); - testExpression('foo(1, a["NAME"])'); testExpression('foo(1, a["NAME"] = 1)'); - testExpression('foo(b = a["NAME"])'); testExpression('foo(b = (a["NAME"] = 1))'); - testExpression('a["NAME"]'); testExpression('a["NAME"] = 1'); - testExpression('a["NAME"]()'); - testExpression('a["NAME"].foo()'); - testExpression('a.b["NAME"]'); testExpression('a.b["NAME"] = 1'); - testExpression('a["b"]["NAME"]'); testExpression('a["b"]["NAME"] = 1'); - }); - }); - }); - describe('Function constructor', function() { it('should NOT allow access to Function constructor in getter', function() { expect(function() { @@ -702,29 +651,17 @@ describe('parser', function() { expect(function() { scope.$eval('{}.toString["constructor"]["constructor"] = 1'); }).toThrowMinErr( - '$parse', 'isecfld', 'Referencing "constructor" field in Angular expressions is disallowed! ' + + '$parse', 'isecfn', 'Referencing Function in Angular expressions is disallowed! ' + 'Expression: {}.toString["constructor"]["constructor"] = 1'); scope.key1 = "const"; scope.key2 = "ructor"; - expect(function() { - scope.$eval('{}.toString[key1 + key2].foo'); - }).toThrowMinErr( - '$parse', 'isecfn', 'Referencing Function in Angular expressions is disallowed! ' + - 'Expression: {}.toString[key1 + key2].foo'); - - expect(function() { - scope.$eval('{}.toString[key1 + key2] = 1'); - }).toThrowMinErr( - '$parse', 'isecfld', 'Referencing "constructor" field in Angular expressions is disallowed! ' + - 'Expression: {}.toString[key1 + key2] = 1'); - expect(function() { scope.$eval('{}.toString[key1 + key2].foo = 1'); }).toThrowMinErr( '$parse', 'isecfn', 'Referencing Function in Angular expressions is disallowed! ' + - 'Expression: {}.toString[key1 + key2].foo = 1'); + 'Expression: {}.toString[key1 + key2].foo = 1'); expect(function() { scope.$eval('{}.toString["constructor"]["a"] = 1');