New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport security fixes for #1736 #1751
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,12 +69,28 @@ export function template(templateSpec, env) { | |
if (!(name in obj)) { | ||
throw new Exception('"' + name + '" not defined in ' + obj); | ||
} | ||
return obj[name]; | ||
return container.lookupProperty(obj, name); | ||
}, | ||
lookupProperty: function(parent, propertyName) { | ||
let result = parent[propertyName]; | ||
if (result == null) { | ||
return result; | ||
} | ||
if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { | ||
return result; | ||
} | ||
|
||
if (!Utils.dangerousPropertyRegex.test(String(propertyName))) { | ||
return result; | ||
} | ||
|
||
return undefined; | ||
}, | ||
lookup: function(depths, name) { | ||
const len = depths.length; | ||
for (let i = 0; i < len; i++) { | ||
if (depths[i] && depths[i][name] != null) { | ||
let result = depths[i] && container.lookupProperty(depths[i], name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This covers compat mode. |
||
if (result != null) { | ||
return depths[i][name]; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,30 @@ | ||
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"}}', {}, ''); | ||
}); | ||
checkPropertyAccess({}); | ||
|
||
describe('in compat-mode', function() { | ||
checkPropertyAccess({ compat: true }); | ||
}); | ||
|
||
describe('in strict-mode', function() { | ||
checkPropertyAccess({ strict: true }); | ||
}); | ||
|
||
|
||
function checkPropertyAccess(compileOptions) { | ||
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'); | ||
expectTemplate('{{constructor.name}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({'constructor': { | ||
'name': 'here we go' | ||
}}) | ||
.toCompileTo('here we go'); | ||
expectTemplate('{{lookup (lookup this "constructor") "name"}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({'constructor': { | ||
'name': 'here we go' | ||
}}) | ||
.toCompileTo('here we go'); | ||
}); | ||
|
||
it('should allow prototype properties that are not constructors', function() { | ||
|
@@ -24,11 +37,55 @@ describe('security issues', function() { | |
} | ||
}); | ||
|
||
shouldCompileTo('{{#with this}}{{this.abc}}{{/with}}', | ||
new TestClass(), 'xyz'); | ||
shouldCompileTo('{{#with this}}{{lookup this "abc"}}{{/with}}', | ||
new TestClass(), 'xyz'); | ||
|
||
expectTemplate('{{#with this}}{{this.abc}}{{/with}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput(new TestClass()) | ||
.toCompileTo('xyz'); | ||
|
||
expectTemplate('{{#with this}}{{lookup this "abc"}}{{/with}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput(new TestClass()) | ||
.toCompileTo('xyz'); | ||
}); | ||
|
||
it('should not allow constructors to be accessed', function() { | ||
expectTemplate('{{lookup (lookup this "constructor") "name"}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({}) | ||
.toCompileTo(''); | ||
if (compileOptions.strict) { | ||
expectTemplate('{{constructor.name}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({}) | ||
.toThrow(TypeError); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was already the case in 3.x before this change and remains after this change currently. |
||
} else { | ||
expectTemplate('{{constructor.name}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({}) | ||
.toCompileTo(''); | ||
} | ||
}); | ||
|
||
it('should not allow __proto__ to be accessed', function() { | ||
expectTemplate('{{lookup (lookup this "__proto__") "name"}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({}) | ||
.toCompileTo(''); | ||
if (compileOptions.strict) { | ||
expectTemplate('{{__proto__.name}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({}) | ||
.toThrow(TypeError); | ||
} else { | ||
expectTemplate('{{__proto__.name}}') | ||
.withCompileOptions(compileOptions) | ||
.withInput({}) | ||
.toCompileTo(''); | ||
} | ||
}); | ||
|
||
} | ||
}); | ||
|
||
describe('GH-1595', function() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears
strict
mode is already covered by the check injavascript-compiler.js
:handlebars.js/lib/handlebars/compiler/javascript-compiler.js
Lines 16 to 19 in 16bd606
However, the reason why this vulnerability appeared in 4.x was because that check was removed and this change was present. Better to make this change to make it completely clear.