Improve security mitigations

Instead of validating the AST in the parser, fix the compiler instead by
handling the types in a safe way.

lib/handlebars/compiler/base.js

@@ -1,7 +1,6 @@
 import parser from './parser';
 import WhitespaceControl from './whitespace-control';
 import * as Helpers from './helpers';
-import Exception from '../exception';
 import { extend } from '../utils';
 
 export { parser };
@@ -12,9 +11,6 @@ extend(yy, Helpers);
 export function parseWithoutProcessing(input, options) {
   // Just return if an already-compiled AST was passed in.
   if (input.type === 'Program') {
-    // When a pre-parsed AST is passed in, validate all node values to prevent
-    // code injection via type-confused literals.
-    validateInputAst(input);
     return input;
   }
 
@@ -36,66 +32,3 @@ export function parse(input, options) {
 
   return strip.accept(ast);
 }
-
-function validateInputAst(ast) {
-  validateAstNode(ast);
-}
-
-function validateAstNode(node) {
-  if (node == null) {
-    return;
-  }
-
-  if (Array.isArray(node)) {
-    node.forEach(validateAstNode);
-    return;
-  }
-
-  if (typeof node !== 'object') {
-    return;
-  }
-
-  if (node.type === 'PathExpression') {
-    if (!isValidDepth(node.depth)) {
-      throw new Exception(
-        'Invalid AST: PathExpression.depth must be an integer'
-      );
-    }
-    if (!Array.isArray(node.parts)) {
-      throw new Exception('Invalid AST: PathExpression.parts must be an array');
-    }
-    for (let i = 0; i < node.parts.length; i++) {
-      if (typeof node.parts[i] !== 'string') {
-        throw new Exception(
-          'Invalid AST: PathExpression.parts must only contain strings'
-        );
-      }
-    }
-  } else if (node.type === 'NumberLiteral') {
-    if (typeof node.value !== 'number' || !isFinite(node.value)) {
-      throw new Exception('Invalid AST: NumberLiteral.value must be a number');
-    }
-  } else if (node.type === 'BooleanLiteral') {
-    if (typeof node.value !== 'boolean') {
-      throw new Exception(
-        'Invalid AST: BooleanLiteral.value must be a boolean'
-      );
-    }
-  }
-
-  Object.keys(node).forEach(propertyName => {
-    if (propertyName === 'loc') {
-      return;
-    }
-    validateAstNode(node[propertyName]);
-  });
-}
-
-function isValidDepth(depth) {
-  return (
-    typeof depth === 'number' &&
-    isFinite(depth) &&
-    Math.floor(depth) === depth &&
-    depth >= 0
-  );
-}

lib/handlebars/compiler/compiler.js

@@ -1,7 +1,13 @@
 /* eslint-disable new-cap */
 
 import Exception from '../exception';
-import { isArray, indexOf, extend } from '../utils';
+import {
+  isArray,
+  indexOf,
+  extend,
+  sanitizeDepth,
+  sanitizeParts
+} from '../utils';
 import AST from './ast';
 
 const slice = [].slice;
@@ -243,7 +249,7 @@ Compiler.prototype = {
       name = path.parts[0],
       isBlock = program != null || inverse != null;
 
-    this.opcode('getContext', path.depth);
+    this.opcode('getContext', sanitizeDepth(path.depth));
 
     this.opcode('pushProgram', program);
     this.opcode('pushProgram', inverse);
@@ -288,29 +294,32 @@ Compiler.prototype = {
   },
 
   PathExpression: function(path) {
-    this.addDepth(path.depth);
+    // Sanitize untrusted AST values at the compiler boundary.
-    this.opcode('getContext', path.depth);
+    // javascript-compiler.js trusts all opcode arguments to be safe.
+    const depth = sanitizeDepth(path.depth);
+    const parts = sanitizeParts(path.parts);
 
-    let name = path.parts[0],
+    this.addDepth(depth);
+    this.opcode('getContext', depth);
+
+    let name = parts[0],
       scoped = AST.helpers.scopedId(path),
-      blockParamId = !path.depth && !scoped && this.blockParamIndex(name);
+      blockParamId = !depth && !scoped && this.blockParamIndex(name);
 
     if (blockParamId) {
-      this.opcode('lookupBlockParam', blockParamId, path.parts);
+      this.opcode(
+        'lookupBlockParam',
+        [Number(blockParamId[0]), Number(blockParamId[1])],
+        parts
+      );
     } else if (!name) {
       // Context reference, i.e. `{{foo .}}` or `{{foo ..}}`
       this.opcode('pushContext');
     } else if (path.data) {
       this.options.data = true;
-      this.opcode('lookupData', path.depth, path.parts, path.strict);
+      this.opcode('lookupData', depth, parts, path.strict);
     } else {
-      this.opcode(
+      this.opcode('lookupOnContext', parts, path.falsy, path.strict, scoped);
-        'lookupOnContext',
-        path.parts,
-        path.falsy,
-        path.strict,
-        scoped
-      );
     }
   },
 
@@ -319,11 +328,11 @@ Compiler.prototype = {
   },
 
   NumberLiteral: function(number) {
-    this.opcode('pushLiteral', number.value);
+    this.opcode('pushLiteral', Number(number.value));
   },
 
   BooleanLiteral: function(bool) {
-    this.opcode('pushLiteral', bool.value);
+    this.opcode('pushLiteral', bool.value === true ? 'true' : 'false');
   },
 
   UndefinedLiteral: function() {
@@ -410,16 +419,16 @@ Compiler.prototype = {
 
   pushParam: function(val) {
     let value = val.value != null ? val.value : val.original || '';
+    let depth = sanitizeDepth(val.depth);
 
     if (this.stringParams) {
       if (value.replace) {
         value = value.replace(/^(\.?\.\/)*/g, '').replace(/\//g, '.');
       }
-
+      if (depth) {
-      if (val.depth) {
+        this.addDepth(depth);
-        this.addDepth(val.depth);
       }
-      this.opcode('getContext', val.depth || 0);
+      this.opcode('getContext', depth);
       this.opcode('pushStringParam', value, val.type);
 
       if (val.type === 'SubExpression') {

lib/handlebars/utils.js

@@ -114,3 +114,30 @@ export function blockParams(params, ids) {
 export function appendContextPath(contextPath, id) {
   return (contextPath ? contextPath + '.' : '') + id;
 }
+
+/**
+ * Coerce an untrusted depth value to a safe non-negative integer.
+ * Returns `0` for any value that is not a finite, non-negative number.
+ *
+ * @param {unknown} depth - The depth value to sanitize.
+ * @returns {number} A non-negative integer.
+ */
+export function sanitizeDepth(depth) {
+  let number = Number(depth);
+  if (!Number.isFinite(number) || number < 0) {
+    return 0;
+  }
+  return Math.floor(number);
+}
+
+/**
+ * Return a sanitized copy of a PathExpression AST node's parts array.
+ * Coerces each element to a string, or returns an empty array if parts
+ * is not an array.
+ *
+ * @param {unknown} parts - The parts value to sanitize.
+ * @returns {string[]} A safe string array.
+ */
+export function sanitizeParts(parts) {
+  return Array.isArray(parts) ? parts.map(String) : [];
+}

spec/compiler.js

@@ -128,121 +128,90 @@ describe('compiler', function() {
       );
     });
 
-    it('should reject AST with invalid PathExpression depth', function() {
+    function createPathExpressionAST(depth, parts) {
-      shouldThrow(
+      return {
-        function() {
+        type: 'Program',
-          Handlebars.compile({
+        body: [
-            type: 'Program',
+          {
-            body: [
+            type: 'MustacheStatement',
-              {
+            escaped: true,
-                type: 'MustacheStatement',
+            strip: { open: false, close: false },
-                escaped: true,
+            path: {
-                strip: { open: false, close: false },
+              type: 'PathExpression',
-                path: {
+              data: false,
-                  type: 'PathExpression',
+              depth: depth,
-                  data: false,
+              parts: parts,
-                  depth: '0',
+              original: 'this'
-                  parts: ['this'],
+            },
-                  original: 'this'
+            params: []
-                },
+          }
-                params: []
+        ]
-              }
+      };
-            ]
+    }
-          })();
+
-        },
+    it('should safely handle AST with non-integer PathExpression depth', function() {
-        Error,
+      // depth '0' is coerced to 0 via Number(), compiles safely
-        'Invalid AST: PathExpression.depth must be an integer'
+      var result = Handlebars.compile(createPathExpressionAST('0', ['this']))();
-      );
+      expect(result).to.be.a('string');
     });
 
-    it('should reject AST with non-array PathExpression parts', function() {
+    it('should safely handle AST with negative PathExpression depth', function() {
-      shouldThrow(
+      // Negative depth is clamped to 0
-        function() {
+      var result = Handlebars.compile(createPathExpressionAST(-1, ['this']))();
-          Handlebars.compile({
+      expect(result).to.be.a('string');
-            type: 'Program',
-            body: [
-              {
-                type: 'MustacheStatement',
-                escaped: true,
-                strip: { open: false, close: false },
-                path: {
-                  type: 'PathExpression',
-                  data: false,
-                  depth: 0,
-                  parts: 'this',
-                  original: 'this'
-                },
-                params: []
-              }
-            ]
-          })();
-        },
-        Error,
-        'Invalid AST: PathExpression.parts must be an array'
-      );
     });
 
-    it('should reject AST with non-string PathExpression part', function() {
+    it('should safely handle AST with fractional PathExpression depth', function() {
-      shouldThrow(
+      // Fractional depth is floored to an integer
-        function() {
+      var result = Handlebars.compile(createPathExpressionAST(0.5, ['this']))();
-          Handlebars.compile({
+      expect(result).to.be.a('string');
-            type: 'Program',
-            body: [
-              {
-                type: 'MustacheStatement',
-                escaped: true,
-                strip: { open: false, close: false },
-                path: {
-                  type: 'PathExpression',
-                  data: false,
-                  depth: 0,
-                  parts: [1],
-                  original: 'this'
-                },
-                params: []
-              }
-            ]
-          })();
-        },
-        Error,
-        'Invalid AST: PathExpression.parts must only contain strings'
-      );
     });
 
-    it('should reject AST with invalid BooleanLiteral value type', function() {
+    it('should safely handle AST with non-array PathExpression parts', function() {
-      shouldThrow(
+      // Non-array parts are coerced to empty array, compiles safely
-        function() {
+      var result = Handlebars.compile(createPathExpressionAST(0, 'this'))();
-          Handlebars.compile({
+      expect(result).to.be.a('string');
-            type: 'Program',
-            body: [
-              {
-                type: 'MustacheStatement',
-                escaped: true,
-                strip: { open: false, close: false },
-                path: {
-                  type: 'PathExpression',
-                  data: false,
-                  depth: 0,
-                  parts: ['if'],
-                  original: 'if'
-                },
-                params: [
-                  {
-                    type: 'BooleanLiteral',
-                    value: 'true',
-                    original: true
-                  }
-                ]
-              }
-            ]
-          })();
-        },
-        Error,
-        'Invalid AST: BooleanLiteral.value must be a boolean'
-      );
     });
 
-    it('should ignore loc metadata while validating AST nodes', function() {
+    it('should safely handle AST with non-string PathExpression part', function() {
+      // Non-string parts are coerced to strings via String()
+      var result = Handlebars.compile(createPathExpressionAST(0, [1]))();
+      expect(result).to.be.a('string');
+    });
+
+    it('should safely handle AST with non-boolean BooleanLiteral value type', function() {
+      // The compiler coerces BooleanLiteral.value via === true before
+      // emitting a pushLiteral opcode, so a non-boolean value like the
+      // string 'true' becomes the literal 'false'.
+      var loc = {
+        source: null,
+        start: { line: 1, column: 0 },
+        end: { line: 1, column: 10 }
+      };
+      var result = Handlebars.compile({
+        type: 'Program',
+        body: [
+          {
+            type: 'MustacheStatement',
+            escaped: true,
+            strip: { open: false, close: false },
+            loc: loc,
+            path: {
+              type: 'BooleanLiteral',
+              value: 'true',
+              original: true,
+              loc: loc
+            },
+            params: []
+          }
+        ]
+      })();
+      // 'true' !== true, so the compiler emits pushLiteral('false').
+      // Handlebars does not render falsy values, so the output is empty.
+      expect(result).to.equal('');
+    });
+
+    it('should ignore loc metadata in AST nodes', function() {
       equal(
         Handlebars.compile({
           type: 'Program',

spec/security.js

@@ -450,6 +450,11 @@ describe('security issues', function() {
     }
 
     function createInjectedProgram() {
+      var loc = {
+        source: null,
+        start: { line: 1, column: 0 },
+        end: { line: 1, column: 20 }
+      };
       return {
         type: 'Program',
         body: [
@@ -460,12 +465,14 @@ describe('security issues', function() {
               open: false,
               close: false
             },
+            loc: loc,
             path: {
               type: 'PathExpression',
               data: false,
               depth: 0,
               parts: ['lookup'],
-              original: 'lookup'
+              original: 'lookup',
+              loc: loc
             },
             params: [
               {
@@ -473,12 +480,14 @@ describe('security issues', function() {
                 data: false,
                 depth: 0,
                 parts: [],
-                original: 'this'
+                original: 'this',
+                loc: loc
               },
               {
                 type: 'NumberLiteral',
                 value: '{},{})) + (Function) + (({}',
-                original: 1
+                original: 1,
+                loc: loc
               }
             ]
           }
@@ -486,11 +495,13 @@ describe('security issues', function() {
       };
     }
 
-    it('should reject AST NumberLiteral type confusion in compile()', function() {
+    it('should neutralize AST NumberLiteral type confusion in compile()', function() {
-      expect(function() {
+      // The compiler coerces NumberLiteral.value via Number() before
-        var template = Handlebars.compile(createInjectedProgram());
+      // emitting a pushLiteral opcode, so a type-confused string value
-        template({});
+      // becomes NaN, preventing code injection.
-      }).to.throw(/Invalid AST/);
+      var template = Handlebars.compile(createInjectedProgram());
+      var result = template({});
+      expect(result).to.not.contain('Function');
     });
 
     it('should reject AST objects passed via dynamic partial lookup', function() {
@@ -499,7 +510,61 @@ describe('security issues', function() {
         template({
           payload: createInjectedProgram()
         });
-      }).to.throw(/Invalid AST|could not be found/);
+      }).to.throw(/could not be found/);
+    });
+
+    it('should sanitize param depth in stringParams mode', function() {
+      // pushParam passes val.depth directly to the getContext opcode.
+      // In stringParams mode, getContext stores the depth in lastContext,
+      // which contextName interpolates into generated code as
+      // 'depths[' + depth + ']'. A malicious depth string can escape the
+      // bracket expression and inject arbitrary code at template runtime.
+      //
+      // With sanitization the depth becomes 0, producing 'depth0' (safe).
+      // Without sanitization the injected expression executes and throws.
+      var loc = {
+        source: null,
+        start: { line: 1, column: 0 },
+        end: { line: 1, column: 20 }
+      };
+      var maliciousAST = {
+        type: 'Program',
+        body: [
+          {
+            type: 'MustacheStatement',
+            escaped: true,
+            strip: { open: false, close: false },
+            loc: loc,
+            path: {
+              type: 'PathExpression',
+              data: false,
+              depth: 0,
+              parts: ['lookup'],
+              original: 'lookup',
+              loc: loc
+            },
+            params: [
+              {
+                type: 'PathExpression',
+                data: false,
+                depth: 'function(){throw new Error("INJECTION")}()',
+                parts: [],
+                original: '',
+                loc: loc
+              }
+            ]
+          }
+        ]
+      };
+
+      var template = Handlebars.compile(maliciousAST, {
+        stringParams: true
+      });
+      // After sanitization the depth is 0, so the template runs without
+      // executing the injected throw expression.
+      expect(function() {
+        template({});
+      }).to.not.throw();
     });
   });