Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 0 additions & 50 deletions docs/content/error/parse/isecprv.ngdoc
View file
Open in desktop

This file was deleted.

30 changes: 8 additions & 22 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
67 changes: 2 additions & 65 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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');
Expand Down