Merge rc/1.19 into next.

javascript/config/suites/javascript/security

@@ -7,6 +7,7 @@
 + semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079
 + semmlecode-javascript-queries/Security/CWE-089/SqlInjection.ql: /Security/CWE/CWE-089
 + semmlecode-javascript-queries/Security/CWE-094/CodeInjection.ql: /Security/CWE/CWE-094
++ semmlecode-javascript-queries/Security/CWE-094/UnsafeDynamicMethodAccess.ql: /Security/CWE/CWE-094
 + semmlecode-javascript-queries/Security/CWE-116/IncompleteSanitization.ql: /Security/CWE/CWE-116
 + semmlecode-javascript-queries/Security/CWE-134/TaintedFormatString.ql: /Security/CWE/CWE-134
 + semmlecode-javascript-queries/Security/CWE-209/StackTraceExposure.ql: /Security/CWE/CWE-209

javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp

@@ -0,0 +1,53 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Calling a user-controlled method on certain objects can lead to invocation of unsafe functions,
+such as <code>eval</code> or the <code>Function</code> constructor. In particular, the global object
+contains the <code>eval</code> function, and any function object contains the <code>Function</code> constructor
+in its <code>constructor</code> property.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Avoid invoking user-controlled methods on the global object or on any function object.
+Whitelist the permitted method names or change the type of object the methods are stored on.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, a message from the document's parent frame can invoke the <code>play</code>
+or <code>pause</code> method. However, it can also invoke <code>eval</code>.
+A malicious website could embed the page in an iframe and execute arbitrary code by sending a message
+with the name <code>eval</code>.
+</p>
+
+<sample src="examples/UnsafeDynamicMethodAccess.js" />
+
+<p>
+Instead of storing the API methods in the global scope, put them in an API object or Map. It is also good
+practice to prevent invocation of inherited methods like <code>toString</code> and <code>valueOf</code>.
+</p>
+
+<sample src="examples/UnsafeDynamicMethodAccessGood.js" />
+
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
+</li>
+<li>
+MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects#Function_properties">Global functions</a>.
+</li>
+<li>
+MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function">Function constructor</a>.
+</li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql

@@ -0,0 +1,17 @@
+/**
+ * @name Unsafe dynamic method access
+ * @description Invoking user-controlled methods on certain objects can lead to remote code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/unsafe-dynamic-method-access
+ * @tags security
+ *       external/cwe/cwe-094
+ */
+import javascript
+import semmle.javascript.security.dataflow.UnsafeDynamicMethodAccess::UnsafeDynamicMethodAccess
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink, source, sink, "Invocation of method derived from $@ may lead to remote code execution.", source.getNode(), "user-controlled value"

javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccess.js

@@ -0,0 +1,14 @@
+// API methods
+function play(data) {
+  // ...
+}
+function pause(data) {
+  // ...
+}
+
+window.addEventListener("message", (ev) => {
+    let message = JSON.parse(ev.data);
+
+    // Let the parent frame call the 'play' or 'pause' function 
+    window[message.name](message.payload);
+});

javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccessGood.js

@@ -0,0 +1,19 @@
+// API methods
+let api = {
+  play: function(data) {
+    // ...
+  },
+  pause: function(data) {
+    // ...
+  }
+};
+
+window.addEventListener("message", (ev) => {
+    let message = JSON.parse(ev.data);
+
+    // Let the parent frame call the 'play' or 'pause' function
+    if (!api.hasOwnProperty(message.name)) {
+      return;
+    }
+    api[message.name](message.payload);
+});

javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll

@@ -0,0 +1,39 @@
+/**
+ * Provides predicates for reasoning about flow of user-controlled values that are used
+ * as property names.
+ */
+import javascript
+
+module PropertyInjection {
+  /**
+   * A data-flow node that sanitizes user-controlled property names that flow through it.
+   */
+  abstract class Sanitizer extends DataFlow::Node {
+  }
+
+  /**
+   * Concatenation with a constant, acting as a sanitizer.
+   */
+  private class ConcatSanitizer extends Sanitizer {
+    ConcatSanitizer() {
+      StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString
+    }
+  }
+
+  /**
+   * Holds if the methods of the given value are unsafe, such as `eval`.
+   */
+  predicate hasUnsafeMethods(DataFlow::SourceNode node) {
+    // eval and friends can be accessed from the global object.
+    node = DataFlow::globalObjectRef()
+    or
+    // document.write can be accessed
+    isDocument(node.asExpr())
+    or
+    // 'constructor' property leads to the Function constructor.
+    node.analyze().getAValue() instanceof AbstractCallable
+    or
+    // Assume that a value that is invoked can refer to a function.
+    exists (node.getAnInvocation())
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll

@@ -6,6 +6,7 @@
 
 import javascript
 import semmle.javascript.frameworks.Express
+import PropertyInjectionShared
 
 module RemotePropertyInjection {
   /**
@@ -45,7 +46,8 @@ module RemotePropertyInjection {
 
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node) or
-      node instanceof Sanitizer
+      node instanceof Sanitizer or
+      node instanceof PropertyInjection::Sanitizer
     }
   }
 
@@ -76,9 +78,12 @@ module RemotePropertyInjection {
    */
   class MethodCallSink extends Sink, DataFlow::ValueNode  {
     MethodCallSink() {
-      exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() |           
-        exists (pr.getAnInvocation())
-      )          
+      exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() |
+        exists (pr.getAnInvocation()) and
+
+        // Omit sinks covered by the UnsafeDynamicMethodAccess query
+        not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource())
+      )
     }
 
     override string getMessage() {
@@ -105,18 +110,4 @@ module RemotePropertyInjection {
       result = " a header name."
     }
   }
-  
-  /** 
-   * A binary expression that sanitzes a value for remote property injection. That 
-   * is, if a string is prepended or appended to the remote input, an attacker 
-   * cannot access arbitrary properties.  
-   */
-  class ConcatSanitizer extends Sanitizer, DataFlow::ValueNode {
-
-    override BinaryExpr astNode;
-
-    ConcatSanitizer() {
-      astNode.getAnOperand() instanceof ConstantString      
-    }
-  }     
 }

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll

@@ -0,0 +1,129 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about method invocations
+ * with a user-controlled method name on objects with unsafe methods.
+ */
+
+import javascript
+import semmle.javascript.frameworks.Express
+import PropertyInjectionShared
+
+module UnsafeDynamicMethodAccess {
+  private import DataFlow::FlowLabel
+  
+  /**
+   * A data flow source for unsafe dynamic method access.
+   */
+  abstract class Source extends DataFlow::Node {
+    /**
+     * Gets the flow label relevant for this source.
+     */
+    DataFlow::FlowLabel getFlowLabel() {
+      result = data()
+    }
+  }
+
+  /**
+   * A data flow sink for unsafe dynamic method access.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the flow label relevant for this sink
+     */
+    abstract DataFlow::FlowLabel getFlowLabel();
+  }
+
+  /**
+   * A sanitizer for unsafe dynamic method access.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * Gets the flow label describing values that may refer to an unsafe
+   * function as a result of an attacker-controlled property name.
+   */
+  UnsafeFunction unsafeFunction() { any() }
+  private class UnsafeFunction extends DataFlow::FlowLabel {
+    UnsafeFunction() { this = "UnsafeFunction" }
+  }
+
+  /**
+   * A taint-tracking configuration for reasoning about unsafe dynamic method access.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "UnsafeDynamicMethodAccess" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      source.(Source).getFlowLabel() = label
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink.(Sink).getFlowLabel() = label
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node) or
+      node instanceof Sanitizer or
+      node instanceof PropertyInjection::Sanitizer
+    }
+
+    /**
+     * Holds if a property of the given object is an unsafe function.
+     */
+    predicate hasUnsafeMethods(DataFlow::SourceNode node) {
+      PropertyInjection::hasUnsafeMethods(node) // Redefined here so custom queries can override it
+    }
+
+    /**
+     * Holds if the `node` is of form `Object.create(null)` and so it has no prototype.
+     */
+    predicate isPrototypeLessObject(DataFlow::MethodCallNode node) {
+      node = DataFlow::globalVarRef("Object").getAMethodCall("create") and
+      node.getArgument(0).asExpr() instanceof NullLiteral
+    }
+
+    override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) {
+      // Reading a property of the global object or of a function
+      exists (DataFlow::PropRead read |
+        hasUnsafeMethods(read.getBase().getALocalSource()) and
+        src = read.getPropertyNameExpr().flow() and
+        dst = read and
+        (srclabel = data() or srclabel = taint()) and
+        dstlabel = unsafeFunction())
+      or
+      // Reading a chain of properties from any object with a prototype can lead to Function
+      exists (PropertyProjection proj |
+        not isPrototypeLessObject(proj.getObject().getALocalSource()) and
+        src = proj.getASelector() and
+        dst = proj and
+        (srclabel = data() or srclabel = taint()) and
+        dstlabel = unsafeFunction())
+    }
+  }
+
+  /**
+   * A source of remote user input, considered as a source for unsafe dynamic method access.
+   */
+  class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * The page URL considered as a flow source for unsafe dynamic method access.
+   */
+  class DocumentUrlAsSource extends Source {
+    DocumentUrlAsSource() { isDocumentURL(asExpr()) }
+  }
+
+  /**
+   * A function invocation of an unsafe function, as a sink for remote unsafe dynamic method access.
+   */
+  class CalleeAsSink extends Sink {
+    CalleeAsSink() {
+      this = any(DataFlow::InvokeNode node).getCalleeNode()
+    }
+
+    override DataFlow::FlowLabel getFlowLabel() {
+      result = unsafeFunction()
+    }
+  }
+}

javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected

@@ -0,0 +1,54 @@
+nodes
+| example.js:9:37:9:38 | ev |
+| example.js:10:9:10:37 | message |
+| example.js:10:19:10:37 | JSON.parse(ev.data) |
+| example.js:10:30:10:31 | ev |
+| example.js:10:30:10:36 | ev.data |
+| example.js:13:5:13:24 | window[message.name] |
+| example.js:13:12:13:18 | message |
+| example.js:13:12:13:23 | message.name |
+| tst.js:3:37:3:38 | ev |
+| tst.js:4:9:4:37 | message |
+| tst.js:4:19:4:37 | JSON.parse(ev.data) |
+| tst.js:4:30:4:31 | ev |
+| tst.js:4:30:4:36 | ev.data |
+| tst.js:5:5:5:24 | window[message.name] |
+| tst.js:5:12:5:18 | message |
+| tst.js:5:12:5:23 | message.name |
+| tst.js:6:9:6:28 | window[message.name] |
+| tst.js:6:16:6:22 | message |
+| tst.js:6:16:6:27 | message.name |
+| tst.js:11:5:11:19 | f[message.name] |
+| tst.js:11:7:11:13 | message |
+| tst.js:11:7:11:18 | message.name |
+| tst.js:15:5:15:14 | window[ev] |
+| tst.js:15:12:15:13 | ev |
+edges
+| example.js:9:37:9:38 | ev | example.js:10:30:10:31 | ev |
+| example.js:10:9:10:37 | message | example.js:13:12:13:18 | message |
+| example.js:10:19:10:37 | JSON.parse(ev.data) | example.js:10:9:10:37 | message |
+| example.js:10:30:10:31 | ev | example.js:10:30:10:36 | ev.data |
+| example.js:10:30:10:36 | ev.data | example.js:10:19:10:37 | JSON.parse(ev.data) |
+| example.js:13:12:13:18 | message | example.js:13:12:13:23 | message.name |
+| example.js:13:12:13:23 | message.name | example.js:13:5:13:24 | window[message.name] |
+| tst.js:3:37:3:38 | ev | tst.js:4:30:4:31 | ev |
+| tst.js:3:37:3:38 | ev | tst.js:15:12:15:13 | ev |
+| tst.js:4:9:4:37 | message | tst.js:5:12:5:18 | message |
+| tst.js:4:9:4:37 | message | tst.js:6:16:6:22 | message |
+| tst.js:4:9:4:37 | message | tst.js:11:7:11:13 | message |
+| tst.js:4:19:4:37 | JSON.parse(ev.data) | tst.js:4:9:4:37 | message |
+| tst.js:4:30:4:31 | ev | tst.js:4:30:4:36 | ev.data |
+| tst.js:4:30:4:36 | ev.data | tst.js:4:19:4:37 | JSON.parse(ev.data) |
+| tst.js:5:12:5:18 | message | tst.js:5:12:5:23 | message.name |
+| tst.js:5:12:5:23 | message.name | tst.js:5:5:5:24 | window[message.name] |
+| tst.js:6:16:6:22 | message | tst.js:6:16:6:27 | message.name |
+| tst.js:6:16:6:27 | message.name | tst.js:6:9:6:28 | window[message.name] |
+| tst.js:11:7:11:13 | message | tst.js:11:7:11:18 | message.name |
+| tst.js:11:7:11:18 | message.name | tst.js:11:5:11:19 | f[message.name] |
+| tst.js:15:12:15:13 | ev | tst.js:15:5:15:14 | window[ev] |
+#select
+| example.js:13:5:13:24 | window[message.name] | example.js:9:37:9:38 | ev | example.js:13:5:13:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | example.js:9:37:9:38 | ev | user-controlled value |
+| tst.js:5:5:5:24 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:5:5:5:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
+| tst.js:6:9:6:28 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:6:9:6:28 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
+| tst.js:11:5:11:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:11:5:11:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
+| tst.js:15:5:15:14 | window[ev] | tst.js:3:37:3:38 | ev | tst.js:15:5:15:14 | window[ev] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |

javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.qlref

@@ -0,0 +1 @@
+Security/CWE-094/UnsafeDynamicMethodAccess.ql

javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/example.js

@@ -0,0 +1,14 @@
+// API methods
+function play(data) {
+  // ...
+}
+function pause(data) {
+  // ...
+}
+
+window.addEventListener("message", (ev) => {
+    let message = JSON.parse(ev.data);
+
+    // Let the parent frame call the 'play' or 'pause' function 
+    window[message.name](message.payload); // NOT OK
+});

javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js

@@ -0,0 +1,16 @@
+let obj = {};
+
+window.addEventListener('message', (ev) => {
+    let message = JSON.parse(ev.data);
+    window[message.name](message.payload); // NOT OK - may invoke eval
+    new window[message.name](message.payload); // NOT OK - may invoke jQuery $ function or similar
+    window["HTMLElement" + message.name](message.payload); // OK - concatenation restricts choice of methods
+    window[`HTMLElement${message.name}`](message.payload); // OK - concatenation restricts choice of methods
+    
+    function f() {}
+    f[message.name](message.payload)(); // NOT OK - may acccess Function constructor
+    
+    obj[message.name](message.payload); // OK - may crash, but no code execution involved
+
+    window[ev](ev); // NOT OK
+});

javascript/ql/test/query-tests/Security/CWE-400/tst.js

@@ -14,7 +14,8 @@ app.get('/user/:id', function(req, res) {
 	Object.defineProperty(myObj, prop, {value: 24}); // NOT OK
 	var headers = {};	
 	headers[prop] = 42; // NOT OK
-	res.set(headers);	
+	res.set(headers);
+	myCoolLocalFct[req.query.x](); // OK - flagged by method name injection
 });
 
 function myCoolLocalFct(x) {