Permalink
Browse files

fix($parse): forbid __{define,lookup}{Getter,Setter}__ properties

It was possible to use `{}.__defineGetter__.call(null, 'alert', (0).valueOf.bind(0))` to set
`window.alert` to a false-ish value, thereby breaking the `isWindow` check, which might lead
to arbitrary code execution in browsers that let you obtain the window object using Array methods.
Prevent that by blacklisting the nasty __{define,lookup}{Getter,Setter}__ properties.

BREAKING CHANGE:
This prevents the use of __{define,lookup}{Getter,Setter}__ inside angular
expressions. If you really need them for some reason, please wrap/bind them to make them
less dangerous, then make them available through the scope object.
1 parent 528be29 commit 48fa3aadd546036c7e69f71046f659ab1de244c6 @thejh thejh committed with IgorMinar Jun 8, 2014
Showing with 83 additions and 0 deletions.
  1. +10 −0 src/ng/parse.js
  2. +73 −0 test/ng/parseSpec.js
View
@@ -36,6 +36,11 @@ function ensureSafeMemberName(name, fullExpression) {
throw $parseMinErr('isecfld',
'Referencing "constructor" field in Angular expressions is disallowed! Expression: {0}',
fullExpression);
+ } else if (name === "__defineGetter__" || name === "__defineSetter__"
+ || name === "__lookupGetter__" || name === "__lookupSetter__") {
+ throw $parseMinErr('isecgetset',
+ 'Defining and looking up getters and setters in Angular expressions is disallowed! '
+ +'Expression: {0}', fullExpression);
}
return name;
}
@@ -62,6 +67,11 @@ function ensureSafeObject(obj, fullExpression) {
throw $parseMinErr('isecobj',
'Referencing Object in Angular expressions is disallowed! Expression: {0}',
fullExpression);
+ } else if (obj === ({}).__defineGetter__ || obj === ({}).__defineSetter__
+ || obj === ({}).__lookupGetter__ || obj === ({}).__lookupSetter__) {
+ throw $parseMinErr('isecgetset',
+ 'Defining and looking up getters and setters in Angular expressions is disallowed! '
+ +'Expression: {0}', fullExpression);
}
}
return obj;
View
@@ -840,6 +840,79 @@ describe('parser', function() {
expect(function() { scope.$eval('array'); }).not.toThrow();
});
});
+
+ describe('getters and setters', function() {
+ it('should NOT allow invocation of __defineGetter__', function() {
+ expect(function() {
+ scope.$eval('{}.__defineGetter__("a", "".charAt)');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__defineGetter__("a", "".charAt)');
+
+ expect(function() {
+ scope.$eval('{}.__defineGetter__.call({}, "a", "".charAt)');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__defineGetter__.call({}, "a", "".charAt)');
+
+ expect(function() {
+ scope.$eval('{}["__defineGetter__"].call({}, "a", "".charAt)');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}["__defineGetter__"].call({}, "a", "".charAt)');
+ });
+
+ it('should NOT allow invocation of __defineSetter__', function() {
+ expect(function() {
+ scope.$eval('{}.__defineSetter__("a", "".charAt)');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__defineSetter__("a", "".charAt)');
+
+ expect(function() {
+ scope.$eval('{}.__defineSetter__.call({}, "a", "".charAt)');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__defineSetter__.call({}, "a", "".charAt)');
+ });
+
+ it('should NOT allow invocation of __lookupGetter__', function() {
+ expect(function() {
+ scope.$eval('{}.__lookupGetter__("a")');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__lookupGetter__("a")');
+
+ expect(function() {
+ scope.$eval('{}.__lookupGetter__.call({}, "a")');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__lookupGetter__.call({}, "a")');
+ });
+
+ it('should NOT allow invocation of __lookupSetter__', function() {
+ expect(function() {
+ scope.$eval('{}.__lookupSetter__("a")');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__lookupSetter__("a")');
+
+ expect(function() {
+ scope.$eval('{}.__lookupSetter__.call({}, "a")');
+ }).toThrowMinErr(
+ '$parse', 'isecgetset', 'Defining and looking up getters and setters in '+
+ 'Angular expressions is disallowed! Expression: '+
+ '{}.__lookupSetter__.call({}, "a")');
+ });
+ });
});
describe('overriding constructor', function() {

0 comments on commit 48fa3aa

Please sign in to comment.