add handlebars taint step

2026-05-03 04:39:29 +02:00 · 2022-03-11 10:26:54 +01:00
parent 5b974582e3
commit 0bd9e9f298
4 changed files with 144 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,69 @@ module Handlebars {
    SafeString() { this = any(Handlebars h).getAConstructorInvocation("SafeString") }
  }
 }
+
+/** Provides logic for taint steps for the handlebars library. */
+module TaintStep {
+  /**
+   * Gets a `SourceNode` tracked from a compilation of a Handlebars template.
+   */
+  private DataFlow::SourceNode compiledHandlebarsTemplate(DataFlow::Node originalCall) {
+    result = compiledHandlebarsTemplate(DataFlow::TypeTracker::end(), originalCall)
+  }
+
+  private DataFlow::SourceNode compiledHandlebarsTemplate(
+    DataFlow::TypeTracker t, DataFlow::Node originalCall
+  ) {
+    t.start() and
+    result = any(Handlebars::Handlebars hb).getAMethodCall(["compile", "template"]) and
+    result = originalCall
+    or
+    exists(DataFlow::TypeTracker t2 |
+      result = compiledHandlebarsTemplate(t2, originalCall).track(t2, t)
+    )
+  }
+
+  /**
+   * 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.
+   */
+  private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(string helperName |
+      exists(DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall |
+        templatingCall = compiledHandlebarsTemplate(compileCall).getACall() and
+        pred = templatingCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs() and
+        (
+          compileCall
+              .getArgument(0)
+              .mayHaveStringValue(any(string templateText |
+                  // an approximation: if the template contains the
+                  // helper name, it's probably a helper call
+                  templateText.matches("%" + helperName + "%")
+                ))
+          or
+          // When we don't have a string value, we can't be sure
+          // and will assume a step.
+          not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s))
+        )
+      ) and
+      exists(DataFlow::CallNode registerHelperCall, DataFlow::FunctionNode helperFunc |
+        registerHelperCall = any(Handlebars::Handlebars hb).getAMethodCall("registerHelper") and
+        registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
+        helperFunc = registerHelperCall.getArgument(1).getAFunctionValue() and
+        helperFunc.getAParameter() = succ
+      )
+    )
+  }
+
+  /**
+   * 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 node1, DataFlow::Node node2) {
+      isHandlebarsArgStep(node1, node2)
+    }
+  }
+}
--- 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,25 @@ 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:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
@@ -6414,6 +6433,30 @@ 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:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | 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 +9887,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:23:28:23:35 | filePath | handlebars.js:19:46:19:60 | req.params.path | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:19:46:19:60 | req.params.path | a user-provided value |
+| handlebars.js:23:28:23:35 | filePath | handlebars.js:31:43:31:57 | req.params.name | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:31:43:31:57 | req.params.name | 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,32 @@
+const express = require('express');
+const hb = require("handlebars");
+const fs = require("fs");
+
+const app = express();
+
+const data = {};
+
+function init() {
+    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"));
+    hb.registerHelper("catFile", catFile);
+}
+
+init();
+
+app.get('/some/path1', function (req, res) {
+    res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
+});
+
+function catFile(filePath) {
+    return fs.readFileSync(filePath); // SINK (reads file)
+}
+
+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 })); // NOT ALLOWED (could be using vulnerable catFile)
+});