Improve security mitigations

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

Author: jaylinski

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;
@@ -288,29 +294,27 @@ Compiler.prototype = {
   },
 
   PathExpression: function(path) {
-    this.addDepth(path.depth);
-    this.opcode('getContext', path.depth);
+    // Sanitize to prevent code injection via type-confused AST nodes.
+    const depth = sanitizeDepth(path.depth);
+    const parts = sanitizeParts(path.parts);
+
+    this.addDepth(depth);
+    this.opcode('getContext', depth);
 
-    let name = path.parts[0],
+    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', blockParamId, 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(
-        'lookupOnContext',
-        path.parts,
-        path.falsy,
-        path.strict,
-        scoped
-      );
+      this.opcode('lookupOnContext', parts, path.falsy, path.strict, scoped);
     }
   },
 
@@ -319,11 +323,11 @@ Compiler.prototype = {
   },
 
   NumberLiteral: function(number) {
-    this.opcode('pushLiteral', number.value);
+    this.opcode('pushNumber', number.value);
   },
 
   BooleanLiteral: function(bool) {
-    this.opcode('pushLiteral', bool.value);
+    this.opcode('pushBoolean', bool.value);
   },
 
   UndefinedLiteral: function() {

lib/handlebars/compiler/javascript-compiler.js

@@ -1,6 +1,6 @@
 import { COMPILER_REVISION, REVISION_CHANGES } from '../base';
 import Exception from '../exception';
-import { isArray } from '../utils';
+import { isArray, sanitizeDepth } from '../utils';
 import CodeGen from './code-gen';
 
 function Literal(value) {
@@ -469,7 +469,7 @@ JavaScriptCompiler.prototype = {
   //
   // Set the value of the `lastContext` compiler value to the depth
   getContext: function(depth) {
-    this.lastContext = depth;
+    this.lastContext = sanitizeDepth(depth);
   },
 
   // [pushContext]
@@ -513,7 +513,13 @@ JavaScriptCompiler.prototype = {
   lookupBlockParam: function(blockParamId, parts) {
     this.useBlockParams = true;
 
-    this.push(['blockParams[', blockParamId[0], '][', blockParamId[1], ']']);
+    this.push([
+      'blockParams[',
+      Number(blockParamId[0]),
+      '][',
+      Number(blockParamId[1]),
+      ']'
+    ]);
     this.resolvePath('context', parts, 1);
   },
 
@@ -527,7 +533,9 @@ JavaScriptCompiler.prototype = {
     if (!depth) {
       this.pushStackLiteral('data');
     } else {
-      this.pushStackLiteral('container.data(data, ' + depth + ')');
+      this.pushStackLiteral(
+        'container.data(data, ' + sanitizeDepth(depth) + ')'
+      );
     }
 
     this.resolvePath('data', parts, 0, true, strict);
@@ -659,6 +667,28 @@ JavaScriptCompiler.prototype = {
     this.pushStackLiteral(value);
   },
 
+  // [pushNumber]
+  //
+  // On stack, before: ...
+  // On stack, after: number, ...
+  //
+  // Pushes a numeric value onto the stack, coercing via Number()
+  // to prevent code injection through type-confused AST nodes.
+  pushNumber: function(value) {
+    this.pushStackLiteral(Number(value));
+  },
+
+  // [pushBoolean]
+  //
+  // On stack, before: ...
+  // On stack, after: boolean, ...
+  //
+  // Pushes a boolean value onto the stack, strictly coercing
+  // to prevent code injection through type-confused AST nodes.
+  pushBoolean: function(value) {
+    this.pushStackLiteral(value === true ? 'true' : 'false');
+  },
+
   // [pushProgram]
   //
   // On stack, before: ...

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) : [];
+}