Merge pull request github#5369 from erik-krogh/tempObjInj

Approved by asgerf

Author: codeql-ci

javascript/change-notes/2021-03-09-template-object-injection.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `js/template-object-injection` query has been added. It highlights places where an attacker can pass special parameters to a template engine.

javascript/ql/src/Security/CWE-073/TemplateObjectInjection.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Directly using user-controlled objects as arguments to template engines might allow an attacker to do 
+local file reads or even remote code execution. 
+</p>
+</overview>
+
+<recommendation>
+<p>
+Avoid using user-controlled objects as arguments to a template engine. Instead, construct the object explicitly with 
+the specific properties needed by the template.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the example below a server uses the user-controlled <code>profile</code> object to 
+render the <code>index</code> template.  
+</p>
+<sample src="examples/TemplateObjectInjection.js" />
+<p>
+However, if an attacker adds a <code>layout</code> property to the <code>profile</code> object then 
+the server will load the file specified by the <code>layout</code> property, thereby allowing an attacker
+to do local file reads.
+</p>
+<p>
+The fix is to have the server construct the object, and only add the properties that are needed by the template. 
+</p>
+<sample src="examples/TemplateObjectInjection_fixed.js" />
+</example>
+
+<references>
+<li>
+blog.shoebpatel.com: <a href="https://blog.shoebpatel.com/2021/01/23/The-Secret-Parameter-LFR-and-Potential-RCE-in-NodeJS-Apps/">The Secret Parameter, LFR, and Potential RCE in NodeJS Apps</a>.
+</li>
+<li>
+cwe.mitre.org: <a href="https://cwe.mitre.org/data/definitions/73.html">CWE-73: External Control of File Name or Path</a>
+</li>
+
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-073/TemplateObjectInjection.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Template Object Injection
+ * @description Instantiating a template using a user-controlled object is vulnerable to local file read and potential remote code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/template-object-injection
+ * @tags security
+ *       external/cwe/cwe-073
+ *       external/cwe/cwe-094
+ */
+
+import javascript
+import DataFlow::PathGraph
+import semmle.javascript.security.dataflow.TemplateObjectInjection::TemplateObjectInjection
+
+from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Template object injection due to $@.", source.getNode(),
+  "user-provided value"

javascript/ql/src/Security/CWE-073/examples/TemplateObjectInjection.js

@@ -0,0 +1,7 @@
+var app = require('express')();
+app.set('view engine', 'hbs');
+
+app.post('/', function (req, res, next) {
+    var profile = req.body.profile;
+    res.render('index', profile);
+});

javascript/ql/src/Security/CWE-073/examples/TemplateObjectInjection_fixed.js

@@ -0,0 +1,10 @@
+var app = require('express')();
+app.set('view engine', 'hbs');
+
+app.post('/', function (req, res, next) {
+    var profile = req.body.profile;
+    res.render('index', {
+        name: profile.name,
+        location: profile.location
+    });
+});

javascript/ql/src/experimental/Security/CWE-073/TemplateObjectInjection.ql

@@ -738,16 +738,32 @@ module Express {
    * as the value of a template variable.
    */
   private class TemplateInput extends HTTP::ResponseBody {
-    RouteHandler rh;
+    TemplateObjectInput obj;
 
     TemplateInput() {
+      obj.getALocalSource().(DataFlow::ObjectLiteralNode).hasPropertyWrite(_, this.flow())
+    }
+
+    override RouteHandler getRouteHandler() { result = obj.getRouteHandler() }
+  }
+
+  /**
+   * An object passed to the `render` method of an HTTP response object.
+   */
+  class TemplateObjectInput extends DataFlow::Node {
+    RouteHandler rh;
+
+    TemplateObjectInput() {
       exists(DataFlow::MethodCallNode render |
         render.calls(rh.getAResponseExpr().flow(), "render") and
-        this = render.getOptionArgument(1, _).asExpr()
+        this = render.getArgument(1)
       )
     }
 
-    override RouteHandler getRouteHandler() { result = rh }
+    /**
+     * Gets the route handler that uses this object.
+     */
+    RouteHandler getRouteHandler() { result = rh }
   }
 
   /**

javascript/ql/src/experimental/Security/CWE-073/documentation-examples/TemplateObjectInjection.js

@@ -0,0 +1,45 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about
+ * template object injection vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `TemplateObjectInjection::Configuration` is needed, otherwise
+ * `TemplateObjectInjectionCustomizations` should be imported instead.
+ */
+
+import javascript
+
+/**
+ * Provides a taint tracking configuration for reasoning template object injection vulnerabilities.
+ */
+module TemplateObjectInjection {
+  import TemplateObjectInjectionCustomizations::TemplateObjectInjection
+  private import semmle.javascript.security.TaintedObject
+
+  /**
+   * A taint tracking configuration for reasoning about template object injection vulnerabilities.
+   */
+  class TemplateObjInjectionConfig extends TaintTracking::Configuration {
+    TemplateObjInjectionConfig() { this = "TemplateObjInjectionConfig" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      source.(Source).getAFlowLabel() = label
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink instanceof Sink and label = TaintedObject::label()
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintedObject::SanitizerGuard
+    }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
+    ) {
+      TaintedObject::step(src, trg, inlbl, outlbl)
+    }
+  }
+}