Skip to content

Commit

Permalink
fix: prevent RCE through the "lookup"-helper
Browse files Browse the repository at this point in the history
- The "lookup" helper could also be used to run a remote code execution
  by manipulating the template. The same check as for regular
  path queries now also is done in the "lookup"-helper
  • Loading branch information
nknapp committed Apr 11, 2019
1 parent c454d94 commit cd38583
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
8 changes: 7 additions & 1 deletion lib/handlebars/helpers/lookup.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
export default function(instance) {
instance.registerHelper('lookup', function(obj, field) {
return obj && obj[field];
if (!obj) {
return obj;
}
if (field === 'constructor' && !obj.propertyIsEnumerable(field)) {
return undefined;
}
return obj[field];
});
}
7 changes: 7 additions & 0 deletions spec/security.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@ describe('security issues', function() {
describe('GH-1495: Prevent Remote Code Execution via constructor', function() {
it('should not allow constructors to be accessed', function() {
shouldCompileTo('{{constructor.name}}', {}, '');
shouldCompileTo('{{lookup (lookup this "constructor") "name"}}', {}, '');
});

it('should allow the "constructor" property to be accessed if it is enumerable', function() {
shouldCompileTo('{{constructor.name}}', {'constructor': {
'name': 'here we go'
}}, 'here we go');
shouldCompileTo('{{lookup (lookup this "constructor") "name"}}', {'constructor': {
'name': 'here we go'
}}, 'here we go');
});

it('should allow prototype properties that are not constructors', function() {
Expand All @@ -23,6 +27,9 @@ describe('security issues', function() {

shouldCompileTo('{{#with this as |obj|}}{{obj.abc}}{{/with}}',
new TestClass(), 'xyz');
shouldCompileTo('{{#with this as |obj|}}{{lookup obj "abc"}}{{/with}}',
new TestClass(), 'xyz');

});
});
});

0 comments on commit cd38583

Please sign in to comment.