Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Backport security fixes for #1736 #1751

Closed
wants to merge 3 commits into from
Closed

Backport security fixes for #1736 #1751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e61e016
fix: check dangerous property access in strict-mode (#1736)
bitwiseman May 18, 2021
3315df5
fix: escape property names in compat mode (#1736)
nknapp Feb 13, 2021
5519e05
Synchronize pagckage.json and package-lock.json
bitwiseman May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/handlebars/compiler/javascript-compiler.js
View file
Open in desktop
Expand Up @@ -28,7 +28,12 @@ JavaScriptCompiler.prototype = {
}
},
depthedLookup: function(name) {
return [this.aliasable('this.lookup'), '(depths, "', name, '")'];
return [
this.aliasable('this.lookup'),
'(depths, ',
JSON.stringify(name),
')'
];
},

compilerInfo: function() {
Expand Down
20 changes: 18 additions & 2 deletions lib/handlebars/runtime.js
View file
Open in desktop
Expand Up @@ -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);
Copy link
Author

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 in javascript-compiler.js:

handlebars.js/lib/handlebars/compiler/javascript-compiler.js

Lines 16 to 19 in 16bd606

if (dangerousPropertyRegex.test(name)) {
const isOwnProperty = [ this.aliasable('Object.prototype.hasOwnProperty'), '.call(', parent, ',', JSON.stringify(name), ')'];
return ['(', isOwnProperty, '?', _actualLookup(), ' : undefined)'];
}

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.

},
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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This covers compat mode.

if (result != null) {
return depths[i][name];
}
}
Expand Down