diff --git a/docs/content/error/parse/isecprv.ngdoc b/docs/content/error/parse/isecprv.ngdoc new file mode 100644 index 000000000000..4bb02426073a --- /dev/null +++ b/docs/content/error/parse/isecprv.ngdoc @@ -0,0 +1,50 @@ +@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 c93d07de1860..ede3f24bcdf7 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -8,18 +8,23 @@ 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. +// obtaining a reference to native JS functions such as the Function constructor, thw 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. // // 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". +// 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. // -// 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. +// 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. // // 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 @@ -33,12 +38,20 @@ 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) { - if (name === "constructor") { +function ensureSafeMemberName(name, fullExpression, allowConstructor) { + if (typeof name !== 'string' && toString.apply(name) !== "[object String]") { + return name; + } + if (name === "constructor" && !allowConstructor) { 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; } @@ -722,7 +735,10 @@ Parser.prototype = { return extend(function(self, locals) { var o = obj(self, locals), - i = indexFn(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 */), v, p; if (!o) return undefined; @@ -738,7 +754,7 @@ Parser.prototype = { return v; }, { assign: function(self, value, locals) { - var key = indexFn(self, locals); + var key = ensureSafeMemberName(indexFn(self, locals), parser.text); // 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 d7d0d94169de..c72b7e818749 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -591,6 +591,57 @@ 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() { @@ -651,17 +702,29 @@ describe('parser', function() { expect(function() { scope.$eval('{}.toString["constructor"]["constructor"] = 1'); }).toThrowMinErr( - '$parse', 'isecfn', 'Referencing Function in Angular expressions is disallowed! ' + + '$parse', 'isecfld', 'Referencing "constructor" field 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');