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
handlebars-lang · Sep 26, 2019 · 213c0bb · 213c0bb
1 parent 050cca0
commit 213c0bb
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 15 deletions.
diff --git 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
@@ -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}}', {}, '');
+        });
+    });
 });