Merge pull request #851 from xiemaisi/js/post-message-star

Approved by esben-semmle

javascript/ql/src/Security/CWE-201/PostMessageStar.qhelp

@@ -0,0 +1,56 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+The <code>window.postMessage</code> method allows different windows or iframes
+to communicate directly, even if they were loaded from different origins, circumventing
+the usual same-origin policy.
+</p>
+<p>
+The sender of the message can restrict the origin of the receiver by specifying
+a target origin. If the receiver window does not come from this origin, the
+message is not sent.
+</p>
+<p>
+Alternatively, the sender can specify a target origin of <code>'*'</code>, which means
+that any origin is acceptable and the message is always sent.
+</p>
+<p>
+This feature should not be used if the message being sent contains sensitive data such
+as user credentials: the target window may have been loaded from a malicious site,
+to which the data would then become available.
+</p>
+</overview>
+
+<recommendation>
+<p>
+If possible, specify a target origin when using <code>window.postMessage</code>.
+Alternatively, encrypt the sensitive data before sending it to prevent an unauthorized
+receiver from accessing it.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example code sends user credentials (in this case, their user
+name) to <code>window.parent</code> without checking its origin. If a malicious site
+loads the page containing this code into an iframe it would be able to gain access
+to the user name.
+</p>
+<sample src="examples/PostMessageStar.js"/>
+<p>
+To prevent this from happening, the origin of the target window should be restricted,
+as in this example:
+</p>
+<sample src="examples/PostMessageStarGood.js"/>
+</example>
+
+
+<references>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage">Window.postMessage</a>.</li>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy">Same-origin policy</a>.</li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-201/PostMessageStar.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Cross-window communication with unrestricted target origin
+ * @description When sending sensitive information to another window using `postMessage`,
+ *              the origin of the target window should be restricted to avoid unintentional
+ *              information leaks.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/cross-window-information-leak
+ * @tags security
+ *       external/cwe/cwe-201
+ *       external/cwe/cwe-359
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.PostMessageStar::PostMessageStar
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Sensitive data returned from $@ is sent to another window without origin restriction.",
+  source.getNode(), "here"

javascript/ql/src/Security/CWE-201/examples/PostMessageStar.js

@@ -0,0 +1 @@
+window.parent.postMessage(userName, '*');

javascript/ql/src/Security/CWE-201/examples/PostMessageStarGood.js

@@ -0,0 +1 @@
+window.parent.postMessage(userName, 'https://lgtm.com');

javascript/ql/src/semmle/javascript/security/SensitiveActions.qll

@@ -17,22 +17,32 @@ import javascript
  * INTERNAL: Do not use directly.
  */
 module HeuristicNames {
-  /** A regular expression that identifies strings that look like they represent secret data that are not passwords. */
+  /** Gets a regular expression that identifies strings that look like they represent secret data that are not passwords. */
   string suspiciousNonPassword() { result = "(?is).*(secret|account|accnt|(?<!un)trusted).*" }
 
-  /** A regular expression that identifies strings that look like they represent secret data that are passwords. */
+  /** Gets a regular expression that identifies strings that look like they represent secret data that are passwords. */
   string suspiciousPassword() { result = "(?is).*(password|passwd).*" }
 
-  /** A regular expression that identifies strings that look like they represent secret data. */
+  /** Gets a regular expression that identifies strings that look like they represent secret data. */
   string suspicious() { result = suspiciousPassword() or result = suspiciousNonPassword() }
 
   /**
-   * A regular expression that identifies strings that look like they represent data that is
+   * Gets a regular expression that identifies strings that look like they represent data that is
    * hashed or encrypted.
    */
   string nonSuspicious() {
     result = "(?is).*(redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
   }
+
+  /**
+   * Gets a regular expression that identifies names that look like they represent credential information.
+   */
+  string suspiciousCredentials() {
+    result = "(?i).*pass(wd|word|code|phrase)(?!.*question).*" or
+    result = "(?i).*(puid|username|userid).*" or
+    result = "(?i).*(cert)(?!.*(format|name)).*" or
+    result = "(?i).*(auth(entication|ori[sz]ation)?)key.*"
+  }
 }
 private import HeuristicNames
 
@@ -144,6 +154,15 @@ class AuthorizationCall extends SensitiveAction, DataFlow::CallNode {
   }
 }
 
+/** A call to a function whose name suggests that it encodes or encrypts its arguments. */
+class ProtectCall extends DataFlow::CallNode {
+  ProtectCall() {
+    exists(string s | getCalleeName().regexpMatch("(?i).*" + s + ".*") |
+      s = "protect" or s = "encode" or s = "encrypt"
+    )
+  }
+}
+
 /**
  * Classes for expressions containing cleartext passwords.
  */

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

@@ -53,14 +53,8 @@ module CleartextStorage {
     override string describe() { result = astNode.describe() }
   }
 
-  /** A call to any method whose name suggests that it encodes or encrypts the parameter. */
-  class ProtectSanitizer extends Sanitizer, DataFlow::ValueNode {
-    ProtectSanitizer() {
-      exists(string s | astNode.(CallExpr).getCalleeName().regexpMatch("(?i).*" + s + ".*") |
-        s = "protect" or s = "encode" or s = "encrypt"
-      )
-    }
-  }
+  /** A call to any function whose name suggests that it encodes or encrypts its arguments. */
+  class ProtectSanitizer extends Sanitizer { ProtectSanitizer() { this instanceof ProtectCall } }
 
   /**
    * An expression set as a value on a cookie instance.

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

@@ -0,0 +1,127 @@
+/**
+ * Provides a taint tracking configuration for reasoning about cross-window communication
+ * with unrestricted origin.
+ */
+
+import javascript
+private import semmle.javascript.security.SensitiveActions
+
+module PostMessageStar {
+  /**
+   * A data flow source for cross-window communication with unrestricted origin.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for cross-window communication with unrestricted origin.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for cross-window communication with unrestricted origin.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A flow label representing an object with at least one tainted property.
+   */
+  private class PartiallyTaintedObject extends DataFlow::FlowLabel {
+    PartiallyTaintedObject() { this = "partially tainted object" }
+  }
+
+  /**
+   * Gets either a standard flow label or the partial-taint label.
+   */
+  private DataFlow::FlowLabel anyLabel() {
+    result instanceof DataFlow::StandardFlowLabel or result instanceof PartiallyTaintedObject
+  }
+
+  /**
+   * A taint tracking configuration for cross-window communication with unrestricted origin.
+   *
+   * This configuration identifies flows from `Source`s, which are sources of
+   * sensitive data, to `Sink`s, which is an abstract class representing all
+   * the places sensitive data may be transmitted across window boundaries without restricting
+   * the origin.
+   *
+   * Additional sources or sinks can be added either by extending the relevant class, or by subclassing
+   * this configuration itself, and amending the sources and sinks.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "PostMessageStar" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel lbl) {
+      sink instanceof Sink and lbl = anyLabel()
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
+    ) {
+      // writing a tainted value to an object property makes the object partially tainted
+      exists(DataFlow::PropWrite write |
+        write.getRhs() = src and
+        inlbl = anyLabel() and
+        trg.(DataFlow::SourceNode).flowsTo(write.getBase()) and
+        outlbl instanceof PartiallyTaintedObject
+      )
+      or
+      // `toString` or `JSON.toString` on a partially tainted object gives a tainted value
+      exists (DataFlow::InvokeNode toString | toString = trg |
+        toString.(DataFlow::MethodCallNode).calls(src, "toString")
+        or
+        toString = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") and
+        src = toString.getArgument(0)
+      ) and
+      inlbl instanceof PartiallyTaintedObject and
+      outlbl = DataFlow::FlowLabel::taint()
+      or
+      // `valueOf` preserves partial taint
+      trg.(DataFlow::MethodCallNode).calls(src, "valueOf") and
+      inlbl instanceof PartiallyTaintedObject and
+      outlbl = inlbl
+    }
+  }
+
+  /**
+   * A sensitive expression, viewed as a data flow source for cross-window communication
+   * with unrestricted origin.
+   */
+  class SensitiveExprSource extends Source, DataFlow::ValueNode { override SensitiveExpr astNode; }
+
+  /**
+   * A variable/property access or function call whose name suggests that it may contain credentials,
+   * viewed as a data flow source for cross-window communication with unrestricted origin.
+   */
+  class CredentialsSource extends Source {
+    CredentialsSource() {
+      exists(string name |
+        name = this.(DataFlow::InvokeNode).getCalleeName() or
+        name = this.(DataFlow::PropRead).getPropertyName() or
+        name = this.asExpr().(VarUse).getVariable().getName()
+      |
+        name.regexpMatch(HeuristicNames::suspiciousCredentials()) and
+        not name.regexpMatch(HeuristicNames::nonSuspicious())
+      )
+    }
+  }
+
+  /** A call to any function whose name suggests that it encodes or encrypts its arguments. */
+  class ProtectSanitizer extends Sanitizer { ProtectSanitizer() { this instanceof ProtectCall } }
+
+  /**
+   * An expression sent using `postMessage` without restricting the target window origin.
+   */
+  class PostMessageStarSink extends Sink {
+    PostMessageStarSink() {
+      exists(DataFlow::MethodCallNode postMessage |
+        postMessage.getMethodName() = "postMessage" and
+        postMessage.getArgument(1).mayHaveStringValue("*") and
+        this = postMessage.getArgument(0)
+      )
+    }
+  }
+}

javascript/ql/test/query-tests/Security/CWE-201/PostMessageStar.expected

@@ -0,0 +1,20 @@
+nodes
+| PostMessageStar2.js:1:27:1:34 | password |
+| PostMessageStar2.js:4:7:4:15 | data |
+| PostMessageStar2.js:4:14:4:15 | {} |
+| PostMessageStar2.js:5:14:5:21 | password |
+| PostMessageStar2.js:8:29:8:32 | data |
+| PostMessageStar2.js:9:29:9:36 | data.foo |
+| PostMessageStar2.js:13:27:13:33 | authKey |
+| PostMessageStar.js:1:27:1:34 | userName |
+edges
+| PostMessageStar2.js:4:7:4:15 | data | PostMessageStar2.js:8:29:8:32 | data |
+| PostMessageStar2.js:4:14:4:15 | {} | PostMessageStar2.js:4:7:4:15 | data |
+| PostMessageStar2.js:5:14:5:21 | password | PostMessageStar2.js:4:14:4:15 | {} |
+| PostMessageStar2.js:5:14:5:21 | password | PostMessageStar2.js:9:29:9:36 | data.foo |
+#select
+| PostMessageStar2.js:1:27:1:34 | password | PostMessageStar2.js:1:27:1:34 | password | PostMessageStar2.js:1:27:1:34 | password | Sensitive data returned from $@ is sent to another window without origin restriction. | PostMessageStar2.js:1:27:1:34 | password | here |
+| PostMessageStar2.js:8:29:8:32 | data | PostMessageStar2.js:5:14:5:21 | password | PostMessageStar2.js:8:29:8:32 | data | Sensitive data returned from $@ is sent to another window without origin restriction. | PostMessageStar2.js:5:14:5:21 | password | here |
+| PostMessageStar2.js:9:29:9:36 | data.foo | PostMessageStar2.js:5:14:5:21 | password | PostMessageStar2.js:9:29:9:36 | data.foo | Sensitive data returned from $@ is sent to another window without origin restriction. | PostMessageStar2.js:5:14:5:21 | password | here |
+| PostMessageStar2.js:13:27:13:33 | authKey | PostMessageStar2.js:13:27:13:33 | authKey | PostMessageStar2.js:13:27:13:33 | authKey | Sensitive data returned from $@ is sent to another window without origin restriction. | PostMessageStar2.js:13:27:13:33 | authKey | here |
+| PostMessageStar.js:1:27:1:34 | userName | PostMessageStar.js:1:27:1:34 | userName | PostMessageStar.js:1:27:1:34 | userName | Sensitive data returned from $@ is sent to another window without origin restriction. | PostMessageStar.js:1:27:1:34 | userName | here |

javascript/ql/test/query-tests/Security/CWE-201/PostMessageStar.js

@@ -0,0 +1 @@
+window.parent.postMessage(userName, '*');

javascript/ql/test/query-tests/Security/CWE-201/PostMessageStar.qlref

@@ -0,0 +1 @@
+Security/CWE-201/PostMessageStar.ql

javascript/ql/test/query-tests/Security/CWE-201/PostMessageStar2.js

@@ -0,0 +1,13 @@
+window.parent.postMessage(password, '*'); // NOT OK
+
+(function() {
+  var data = {};
+  data.foo = password;
+  data.bar = "unproblematic";
+
+  window.parent.postMessage(data, '*');     // NOT OK
+  window.parent.postMessage(data.foo, '*'); // NOT OK
+  window.parent.postMessage(data.bar, '*'); // OK
+})();
+
+window.parent.postMessage(authKey, '*');

javascript/ql/test/query-tests/Security/CWE-201/PostMessageStarGood.js

@@ -0,0 +1 @@
+window.parent.postMessage(userName, 'https://lgtm.com');