Merge pull request #503 from asger-semmle/unsafe-global-object-access

JS: add method name injection query

Author: Max Schaefer

change-notes/1.19/analysis-javascript.md

@@ -24,6 +24,7 @@
 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
 | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. |
 | Host header poisoning in email generation |  security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default.  |
+| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. Results are shown on LGTM by default. |
 | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
 | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |

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()
+    }
+  }
+}