various improvements to the js/missing-origin-verification query

javascript/ql/src/Security/CWE-020/PostMessageNoOriginCheck.qhelp

@@ -5,26 +5,31 @@
 
 <overview>
 
-<p>If you use cross-origin communication between Window objects and do expect to receive messages from other sites, always verify the sender's identity using the origin and possibly source properties of the recevied `MessageEvent`. </p>
-
-<p>Unexpected behaviours, like `DOM-based XSS` could occur, if the event handler for incoming data does not check the origin of the data received and handles the data in an unsafe way.</p>
+<p>
+The <code>"message"</code> event is used to send messages between windows. 
+An untrusted origin is allowed to send messages to a trusted window, and if the origin 
+is not checked that can lead to various security issues.
+</p>
 </overview>
 
 <recommendation>
 <p>
-Always verify the sender's identity of incoming messages.
+Always verify the origin of incoming messages.
 </p>
-
 </recommendation>
 
 <example>
-<p>In the first example, the `MessageEvent.data` is passed to the `eval` function withouth checking the origin. This means that any window can send arbitrary messages that will be executed in the window receiving the message</p>
+<p>
+The example below uses a received message to execute some code. However, the 
+origin of the message is not checked, so it might be possible for an attacker
+to execute arbitrary code.
+</p>
 <sample src="examples/postMessageNoOriginCheck.js" />
 
-<p> In the second example, the `MessageEvent.origin` is verified with an unsecure check. For example, using `event.origin.indexOf('www.example.com') > -1` can be bypassed because the string `www.example.com` could appear anywhere in `event.origin` (i.e. `www.example.com.mydomain.com`)</p>
-<sample src="examples/postMessageInsufficientCheck.js" />
-
-<p> In the third example, the `MessageEvent.origin` is properly checked against a trusted origin. </p>
+<p>
+The example is fixed below, where the origin is checked to be trusted. 
+It is therefore not possible for an attacker to attack using an untrusted origin.
+</p>
 <sample src="examples/postMessageWithOriginCheck.js" />
 
 </example>

javascript/ql/src/Security/CWE-020/PostMessageNoOriginCheck.ql

@@ -1,62 +1,73 @@
 /**
- * @name Missing `MessageEvent.origin` verification in `postMessage` handlers
- * @description Missing the `MessageEvent.origin` verification in `postMessage` handlers, allows any windows to send arbitrary data to the `MessageEvent` listener.
- * This could lead to unexpected behavior, especially when `MessageEvent.data` is used in an unsafe way.
+ * @name Missing origin verification in `postMessage` handler
+ * @description Missing origin verification in a `postMessage` handler allows any windows to send arbitrary data to the handler.
  * @kind problem
  * @problem.severity warning
- * @precision high
- * @id js/missing-postmessageorigin-verification
+ * @security-severity 5
+ * @precision medium
+ * @id js/missing-origin-verification
  * @tags correctness
  *       security
  *       external/cwe/cwe-020
  */
 
 import javascript
-import semmle.javascript.security.dataflow.DOM
 
-/**
- * A method call for the insecure functions used to verify the `MessageEvent.origin`.
- */
-class InsufficientOriginChecks extends DataFlow::Node {
-  InsufficientOriginChecks() {
-    exists(DataFlow::Node node |
-      this.(StringOps::StartsWith).getSubstring() = node or
-      this.(StringOps::Includes).getSubstring() = node or
-      this.(StringOps::EndsWith).getSubstring() = node
-    )
-  }
-}
-
-/**
- * A function handler for the `MessageEvent`.
- */
+/** A function that handles "message" events. */
 class PostMessageHandler extends DataFlow::FunctionNode {
-  PostMessageHandler() { this.getFunction() instanceof PostMessageEventHandler }
-}
+  override PostMessageEventHandler astNode;
 
-/**
- * The `MessageEvent` parameter received by the handler
- */
-class PostMessageEvent extends DataFlow::SourceNode {
-  PostMessageEvent() { exists(PostMessageHandler handler | this = handler.getParameter(0)) }
-
-  /**
-   * Holds if an access on `MessageEvent.origin` is in an `EqualityTest` and there is no call of an insufficient verification method on `MessageEvent.origin`
-   */
-  predicate hasOriginChecked() {
-    exists(EqualityTest test |
-      this.getAPropertyRead(["origin", "source"]).flowsToExpr(test.getAnOperand())
-    )
-  }
-
-  /**
-   * Holds if there is an insufficient method call (i.e indexOf) used to verify `MessageEvent.origin`
-   */
-  predicate hasOriginInsufficientlyChecked() {
-    this.getAPropertyRead("origin").getAMethodCall*() instanceof InsufficientOriginChecks
+  /** Gets the parameter that contains the event. */
+  DataFlow::ParameterNode getEventParameter() {
+    result = DataFlow::parameterNode(astNode.getEventParameter())
   }
 }
 
-from PostMessageEvent event
-where not event.hasOriginChecked() or event.hasOriginInsufficientlyChecked()
-select event, "Missing or unsafe origin verification."
+/** Gets a reference to the event from a postmessage `handler` */
+DataFlow::SourceNode event(DataFlow::TypeTracker t, PostMessageHandler handler) {
+  t.start() and
+  result = handler.getEventParameter()
+  or
+  exists(DataFlow::TypeTracker t2 | result = event(t2, handler).track(t2, t))
+}
+
+/** Gets a reference to the .origin from a postmessage event. */
+DataFlow::SourceNode origin(DataFlow::TypeTracker t, PostMessageHandler handler) {
+  t.start() and
+  result = event(DataFlow::TypeTracker::end(), handler).getAPropertyRead("origin")
+  or
+  result =
+    origin(t.continue(), handler)
+        .getAMethodCall([
+            "toString", "toLowerCase", "toUpperCase", "toLocaleLowerCase", "toLocaleUpperCase"
+          ])
+  or
+  exists(DataFlow::TypeTracker t2 | result = origin(t2, handler).track(t2, t))
+}
+
+/** Gets a reference to the .source from a postmessage event. */
+DataFlow::SourceNode source(DataFlow::TypeTracker t, PostMessageHandler handler) {
+  t.start() and
+  result = event(DataFlow::TypeTracker::end(), handler).getAPropertyRead("source")
+  or
+  exists(DataFlow::TypeTracker t2 | result = source(t2, handler).track(t2, t))
+}
+
+/** Gets a reference to the origin or the source of a postmessage event. */
+DataFlow::SourceNode sourceOrOrigin(PostMessageHandler handler) {
+  result = source(DataFlow::TypeTracker::end(), handler) or
+  result = origin(DataFlow::TypeTracker::end(), handler)
+}
+
+/** Holds if there exists a check of the .origin or .source of the postmessage `handler`. */
+predicate hasOriginCheck(PostMessageHandler handler) {
+  // event.origin === "constant"
+  exists(EqualityTest test | sourceOrOrigin(handler).flowsToExpr(test.getAnOperand()))
+  or
+  // set.includes(event.source)
+  exists(InclusionTest test | sourceOrOrigin(handler).flowsTo(test.getContainedNode()))
+}
+
+from PostMessageHandler handler
+where not hasOriginCheck(handler)
+select handler.getEventParameter(), "Postmessage handler has no origin check."

javascript/ql/src/Security/CWE-020/examples/postMessageInsufficientCheck.js

@@ -1,14 +0,0 @@
-function postMessageHandler(event) {
-    let origin = event.origin.toLowerCase();
-
-    let host = window.location.host;
-
-    // BAD
-    if (origin.indexOf(host) === -1)
-        return;
-
-
-    eval(event.data);
-}
-
-window.addEventListener('message', postMessageHandler, false);

javascript/ql/src/Security/CWE-020/examples/postMessageWithOriginCheck.js

@@ -1,7 +1,7 @@
 function postMessageHandler(event) {
     console.log(event.origin)
     // GOOD: the origin property is checked
-    if (event.origin === 'www.example.com') {
+    if (event.origin === 'https://www.example.com') {
         // do something
     }
 }