Permalink
Browse files

feat($parse): secure expressions by hiding "private" properties

BREAKING CHANGE:
This commit introduces the notion of "private" properties (properties
whose names begin and/or end with an underscore) on the scope chain.
These properties will not be available to Angular expressions (i.e. {{
}} interpolation in templates and strings passed to `$parse`)  They are
freely available to JavaScript code (as before).

Motivation
----------
Angular expressions execute in a limited context.  They do not have
direct access to the global scope, Window, Document or the Function
constructor.  However, they have direct access to names/properties on
the scope chain.  It has been a long standing best practice to keep
sensitive APIs outside of the scope chain (in a closure or your
controller.)  That's easier said that done for two reasons: (1)
JavaScript does not have a notion of private properties so if you need
someone on the scope chain for JavaScript use, you also expose it to
Angular expressions, and (2) the new "controller as" syntax that's now
in increased usage exposes the entire controller on the scope chain
greatly increaing the exposed surface.  Though Angular expressions are
written and controlled by the developer, they (1) typically deal with
user input and (2) don't get the kind of test coverage that JavaScript
code would.  This commit provides a way, via a naming convention, to
allow publishing/restricting properties from controllers/scopes to
Angular expressions enabling one to only expose those properties that
are actually needed by the expressions.
  • Loading branch information...
1 parent 5b62065 commit 3d6a89e8888b14ae5cb5640464e12b7811853c7e @chirayuk chirayuk committed Oct 18, 2013
Showing with 140 additions and 11 deletions.
  1. +50 −0 docs/content/error/parse/isecprv.ngdoc
  2. +25 −9 src/ng/parse.js
  3. +65 −2 test/ng/parseSpec.js
@@ -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
+<div>{{user._private_field}}</div>
+```
+
+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.
View
@@ -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;
View
@@ -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');

11 comments on commit 3d6a89e

kloy replied Nov 11, 2013

While I understand the intent of this change, I completely disagree with the release strategy. AngularJS had over a year of "unstable" point releases and 3 release candidates to try out breaking changes. Why would you release a breaking change of this type in the first major release of 1.2.x?

This is functionality we use at our company across multiple apps and we keep an eye on all unstable releases to ensure forward compatibility. We do not have time to monitor every commit and rely on the AngularJS team to follow the best practices that have been set forth and utilize the processes the team already has in place such as unstable branches and release candidates.

What's the recommended practice for conforming to this change? We typically define RESTful javascript models and use private naming conventions for filtering out properties that are only used to store and display ui-related state but shouldn't be sent to the server when saving the model. In other words, we use _ primarily to store ui-related information that would be used in an Angular expression, so being unable to access these properties in Angular is a very big inconvenience and we'll need to rethink our practices to upgrade to 1.2. Surely the solution can't be to define accessor functions on scope for each private property that we need to access in an Angular expression?

kloy replied Nov 12, 2013

@jr314159 We took the approach of prefixing UI specific properties with ui_ when dealing with a similar situation where we would decorate collections of data from our REST services. We considered using $ since that is another popular convention in the JS world of denoting limited privilege but decided not to since $ and $$ are popularly used in the AngularJS library.

Wouldn't it be pretty easy to make this new functionality configurable through $parseProvider? That way the check could be enabled by default, but be disabled in a .config block to ease version upgrades. Or am I not seeing the whole picture here?

This is totally unexpected. Broke my MongoDB app as well.

This seems very unreasonable. MongoDB and CouchDB use _id for, well, id. Forcing everyone to write a wrapper to hide 'id' to pretend it to be "not private" is not a very efficient way to enforce this capricious restriction. If someone wants such a restriction, it should be a configuration option with prefix being specified (i.e. "privateFieldPrefix" so people can set it to '$' or '' or something else if they want to enforce the restriction).

I very much agree with @kloy . Why would you wait until AFTER all of the release candidates to make such a change? This is very disruptive and unexpected from such an otherwise well run project. Can you at least not make this behavior configurable like you did for the unwrapping of promises?

8-uh replied Nov 14, 2013

While this doesn't "fix" this change, this is the solution I've come up with to get around it. Yes, it's obvious, but it gets the work done.

http://stackoverflow.com/questions/19983635/angular-1-2-0-error-referencing-private-fields-in-angular-expressions-is-disa

Someone explain to me what this accomplishes that

// some js code that establishes a scope
var somePrivateVariableHiddenFromTheTemplate = "supersecret";

wouldn't accomplish, without breaking things?

Contributor

jbdeboer replied Nov 15, 2013

This commit has been reverted in AngularJS 1.2.1. http://blog.angularjs.org/2013/11/angularjs-121-underscore-empathy.html

It is good hear this change was reverted. While I understand the need to mark something as private in certain cases, using such a common naming pattern to indicate a private member is not a good idea. It broke my app. :(

Please sign in to comment.