From 213c0bbe3c4bd83a534d67384e5afa0000347ff6 Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Thu, 26 Sep 2019 23:55:14 +0200 Subject: [PATCH] Use Object.prototype.propertyIsEnumerable to check for constructors - context.propertyIsEnumerable can be replaced via __definedGetter__ - This is a fix specific to counter a known RCE exploit. Other fixes will follow. closes #1563 --- .../compiler/javascript-compiler.js | 36 +++++++++++-------- spec/security.js | 12 ++++++- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/lib/handlebars/compiler/javascript-compiler.js b/lib/handlebars/compiler/javascript-compiler.js index 1b9b2318d..3491aad8e 100644 --- a/lib/handlebars/compiler/javascript-compiler.js +++ b/lib/handlebars/compiler/javascript-compiler.js @@ -14,12 +14,20 @@ JavaScriptCompiler.prototype = { // alternative compiled forms for name lookup and buffering semantics nameLookup: function(parent, name/* , type*/) { if (name === 'constructor') { - return ['(', parent, '.propertyIsEnumerable(\'constructor\') ? ', parent, '.constructor : undefined', ')']; + return ['(', _isEnumerable(), '?', _actualLookup(), ' : undefined)']; } - if (JavaScriptCompiler.isValidJavaScriptVariableName(name)) { - return [parent, '.', name]; - } else { - return [parent, '[', JSON.stringify(name), ']']; + return _actualLookup(); + + function _isEnumerable() { + return `Object.prototype.propertyIsEnumerable.call(${parent},'constructor')`; + } + + function _actualLookup() { + if (JavaScriptCompiler.isValidJavaScriptVariableName(name)) { + return [parent, '.', name]; + } else { + return [parent, '[', JSON.stringify(name), ']']; + } } }, depthedLookup: function(name) { @@ -339,9 +347,9 @@ JavaScriptCompiler.prototype = { params.splice(1, 0, current); this.pushSource([ - 'if (!', this.lastHelper, ') { ', - current, ' = ', this.source.functionCall(blockHelperMissing, 'call', params), - '}']); + 'if (!', this.lastHelper, ') { ', + current, ' = ', this.source.functionCall(blockHelperMissing, 'call', params), + '}']); }, // [appendContent] @@ -686,16 +694,16 @@ JavaScriptCompiler.prototype = { if (!this.options.strict) { lookup[0] = '(helper = '; lookup.push( - ' != null ? helper : ', - this.aliasable('container.hooks.helperMissing') + ' != null ? helper : ', + this.aliasable('container.hooks.helperMissing') ); } this.push([ - '(', lookup, - (helper.paramsInit ? ['),(', helper.paramsInit] : []), '),', - '(typeof helper === ', this.aliasable('"function"'), ' ? ', - this.source.functionCall('helper', 'call', helper.callParams), ' : helper))' + '(', lookup, + (helper.paramsInit ? ['),(', helper.paramsInit] : []), '),', + '(typeof helper === ', this.aliasable('"function"'), ' ? ', + this.source.functionCall('helper', 'call', helper.callParams), ' : helper))' ]); }, diff --git a/spec/security.js b/spec/security.js index 4c092b1c0..418541f67 100644 --- a/spec/security.js +++ b/spec/security.js @@ -33,7 +33,7 @@ describe('security issues', function() { }); }); - describe('GH-xxxx: Prevent explicit call of helperMissing-helpers', function() { + describe('GH-1558: Prevent explicit call of helperMissing-helpers', function() { if (!Handlebars.compile) { return; } @@ -88,4 +88,14 @@ describe('security issues', function() { }); }); }); + + describe('GH-1563', function() { + it('should not allow to access constructor after overriding via __defineGetter__', function() { + shouldCompileTo('{{__defineGetter__ "undefined" valueOf }}' + + '{{#with __lookupGetter__ }}' + + '{{__defineGetter__ "propertyIsEnumerable" (this.bind (this.bind 1)) }}' + + '{{constructor.name}}' + + '{{/with}}', {}, ''); + }); + }); });