Merge pull request #8430 from kaeluka/js/CVE-2022-24718

JS: Add taint step for handlebars model
2026-04-30 03:05:15 +02:00 · 2022-04-19 15:57:58 +01:00
parent 91324d40b5 fb66ccff39
commit 2fb3147b7b
4 changed files with 245 additions and 1 deletions
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -27,3 +27,133 @@ module Handlebars {
    SafeString() { this = any(Handlebars h).getAConstructorInvocation("SafeString") }
  }
 }
+
+/** Provides logic for taint steps for the handlebars library. */
+private module HandlebarsTaintSteps {
+  /**
+   * Gets a reference to a compiled Handlebars template.
+   */
+  private DataFlow::SourceNode compiledTemplate(DataFlow::CallNode compileCall) {
+    result = compiledTemplate(DataFlow::TypeTracker::end(), compileCall)
+  }
+
+  private DataFlow::SourceNode compiledTemplate(
+    DataFlow::TypeTracker t, DataFlow::CallNode compileCall
+  ) {
+    t.start() and
+    result = any(Handlebars::Handlebars hb).getAMethodCall(["compile", "template"]) and
+    result = compileCall
+    or
+    exists(DataFlow::TypeTracker t2 | result = compiledTemplate(t2, compileCall).track(t2, t))
+  }
+
+  /**
+   * Gets a reference to a parameter of a registered Handlebars helper.
+   *
+   * ```javascript
+   * function loudHelper(text) {
+   *   return text.toUpperCase();
+   * }
+   *
+   * hb.registerHelper("loud", loudHelper);
+   * ```
+   * In this example, `getRegisteredHelperParameter("loud", func, 0)` will bind `func` to
+   * the `FunctionNode` representing `function loudHelper`, and return its parameter `text`.
+   */
+  private DataFlow::ParameterNode getRegisteredHelperParam(
+    string helperName, DataFlow::FunctionNode helperFunction, int paramIndex
+  ) {
+    exists(DataFlow::CallNode registerHelperCall |
+      registerHelperCall = any(Handlebars::Handlebars hb).getAMemberCall("registerHelper") and
+      registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
+      helperFunction = registerHelperCall.getArgument(1).getAFunctionValue() and
+      result = helperFunction.getParameter(paramIndex)
+    )
+  }
+
+  /**
+   * Gets a `call` (which is a block wrapped inside curly braces inside the template) from `templateText`.
+   *
+   * For example, `getAHelperCallFromTemplate("Hello {{loud customer}}")` will return `"loud customer"`.
+   */
+  bindingset[templateText]
+  private string getAHelperCallFromTemplate(string templateText) {
+    result = templateText.regexpFind("\\{\\{[^}]+\\}\\}", _, _).regexpReplaceAll("[{}]", "").trim() and
+    result.regexpMatch(".*\\s.*")
+  }
+
+  /**
+   * Holds for calls to helpers from handlebars templates.
+   *
+   * ```javascript
+   * hb.compile("contents of file {{path}} are: {{catFile path}} {{echo p1 p2}}");
+   * ```
+   *
+   * In the example, the predicate will hold for:
+   *
+   * * helperName="catFile", argIdx=1, arg="path"
+   * * helperName="echo", argIdx=1, arg="p1"
+   * * helperName="echo", argIdx=2, arg="p2"
+   *
+   * The initial `{{path}}` will not be considered, as it has no arguments.
+   */
+  bindingset[templateText]
+  private predicate isTemplateHelperCallArg(
+    string templateText, string helperName, int argIdx, string argVal
+  ) {
+    exists(string call | call = getAHelperCallFromTemplate(templateText) |
+      helperName = call.regexpFind("[^\\s]+", 0, _) and
+      argIdx >= 0 and
+      argVal = call.regexpFind("[^\\s]+", argIdx + 1, _)
+    )
+  }
+
+  /**
+   * Holds if there's a step from `pred` to `succ` due to templating data being
+   * passed from a templating call to a registered helper via a parameter.
+   *
+   * To establish the step, we look at the template passed to `compile`, and will
+   * only track steps from templates to helpers they actually reference.
+   *
+   * ```javascript
+   * function loudHelper(text) {
+   *   //                ^^^^ succ
+   *   return text.toUpperCase();
+   * }
+   *
+   * hb.registerHelper("loud", loudHelper);
+   *
+   * const template = hb.compile("Hello, {{loud name}}!");
+   *
+   * template({name: "user"});
+   * //              ^^^^^^ pred
+   * ```
+   */
+  private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(
+      string helperName, DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall,
+      DataFlow::FunctionNode helperFunction
+    |
+      templatingCall = compiledTemplate(compileCall).getACall() and
+      exists(string templateText, string paramName, int argIdx |
+        compileCall.getArgument(0).mayHaveStringValue(templateText)
+      |
+        pred = templatingCall.getArgument(0).getALocalSource().getAPropertyWrite(paramName).getRhs() and
+        isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
+        succ = getRegisteredHelperParam(helperName, helperFunction, argIdx)
+      )
+    )
+  }
+
+  /**
+   * A shared flow step from passing data to a handlebars template with
+   * helpers registered.
+   */
+  class HandlebarsStep extends DataFlow::SharedFlowStep {
+    DataFlow::CallNode templatingCall;
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      isHandlebarsArgStep(pred, succ)
+    }
+  }
+}
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -850,7 +850,7 @@ module TaintedPath {

  /**
   * Holds if we should include a step from `src -> dst` with labels `srclabel -> dstlabel`, and the
-   * standard taint step `src -> dst` should be suppresesd.
+   * standard taint step `src -> dst` should be suppressed.
   */
  private predicate isPosixPathStep(
    DataFlow::Node src, DataFlow::Node dst, Label::PosixPath srclabel, Label::PosixPath dstlabel
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1541,6 +1541,34 @@ nodes
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
@@ -6414,6 +6442,38 @@ edges
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -9844,6 +9904,8 @@ edges
 | TaintedPath.js:213:45:213:48 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:213:45:213:48 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on $@. | express.js:8:20:8:32 | req.query.bar | a user-provided value |
+| handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on $@. | handlebars.js:29:46:29:60 | req.params.path | a user-provided value |
+| handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on $@. | handlebars.js:43:15:43:29 | req.params.path | a user-provided value |
 | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
@@ -0,0 +1,52 @@
+const express = require('express');
+const hb = require("handlebars");
+const fs = require("fs");
+
+const app = express();
+
+const data = {};
+
+function init() {
+    hb.registerHelper("catFile", function catFile(filePath) {
+        return fs.readFileSync(filePath); // SINK (reads file)
+    });
+    hb.registerHelper("prependToLines", function prependToLines(prefix, filePath) {
+        return fs
+          .readFileSync(filePath)
+          .split("\n")
+          .map((line) => prefix + line)
+          .join("\n");
+    });
+    data.compiledFileAccess = hb.compile("contents of file {{path}} are: {{catFile path}}")
+    data.compiledBenign = hb.compile("hello, {{name}}");
+    data.compiledUnknown = hb.compile(fs.readFileSync("greeting.template"));
+    data.compiledMixed = hb.compile("helpers may have several args, like here: {{prependToLines prefix path}}");
+}
+
+init();
+
+app.get('/some/path1', function (req, res) {
+    res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
+});
+
+app.get('/some/path2', function (req, res) {
+    res.send(data.compiledBenign({ name: req.params.name })); // ALLOWED (this template does not use catFile)
+});
+
+app.get('/some/path3', function (req, res) {
+    res.send(data.compiledUnknown({ name: req.params.name })); // ALLOWED (could be using a vulnerable helper, but we'll assume it's ok)
+});
+
+app.get('/some/path4', function (req, res) {
+    res.send(data.compiledMixed({
+        prefix: ">>> ",
+        path: req.params.path // NOT ALLOWED (template uses vulnerable helper)
+    }));
+});
+
+app.get('/some/path5', function (req, res) {
+    res.send(data.compiledMixed({
+        prefix: req.params.prefix, // ALLOWED (this parameter is safe)
+        path: "data/path-5.txt"
+    }));
+});