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..c93d07de1860 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -8,23 +8,18 @@ var promiseWarning; // ------------------------------ // Angular expressions are generally considered safe because these expressions only have direct // access to $scope and locals. However, one can obtain the ability to execute arbitrary JS code by -// obtaining a reference to native JS functions such as the Function constructor, the global Window -// or Document object. In addition, many powerful functions for use by JavaScript code are -// published on scope that shouldn't be available from within an Angular expression. +// obtaining a reference to native JS functions such as the Function constructor. // // As an example, consider the following Angular expression: // // {}.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 +33,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 +722,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 +738,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');