Merge pull request #8724 from erik-krogh/postMessage

JS: promote the `js/missing-origin-verification` query

javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

@@ -1225,19 +1225,25 @@ module TaintTracking {
    * An equality test on `e.origin` or `e.source` where `e` is a `postMessage` event object,
    * considered as a sanitizer for `e`.
    */
-  private class PostMessageEventSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
+  private class PostMessageEventSanitizer extends AdditionalSanitizerGuardNode {
     VarAccess event;
-    override EqualityTest astNode;
+    boolean polarity;
 
     PostMessageEventSanitizer() {
-      exists(string prop | prop = "origin" or prop = "source" |
-        astNode.getAnOperand().(PropAccess).accesses(event, prop) and
-        event.mayReferToParameter(any(PostMessageEventHandler h).getEventParameter())
+      event.mayReferToParameter(any(PostMessageEventHandler h).getEventParameter()) and
+      exists(DataFlow::PropRead read | read.accesses(event.flow(), ["origin", "source"]) |
+        exists(EqualityTest test | polarity = test.getPolarity() and this.getAstNode() = test |
+          test.getAnOperand().flow() = read
+        )
+        or
+        exists(InclusionTest test | polarity = test.getPolarity() and this = test |
+          test.getContainedNode() = read
+        )
       )
     }
 
     override predicate sanitizes(boolean outcome, Expr e) {
-      outcome = astNode.getPolarity() and
+      outcome = polarity and
       e = event
     }
 

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

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+
+<p>
+The <code>"message"</code> event is used to send messages between windows. 
+An untrusted window can send a message to a trusted window, and it is up to the receiver to verify the legitimacy of the message. One way of performing that verification is to check the <code>origin</code> of the message ensure that it originates from a trusted window.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Always verify the origin of incoming messages.
+</p>
+</recommendation>
+
+<example>
+<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/MissingOriginCheckBad.js" />
+
+<p>
+The example is fixed below, where the origin is checked to be trusted. 
+It is therefore not possible for a malicious user to perform an attack using an untrusted origin.
+</p>
+<sample src="examples/MissingOriginCheckGood.js" />
+
+</example>
+
+<references>
+
+<li><a href="https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage">Window.postMessage()</a>.</li>
+<li><a href="https://portswigger.net/web-security/dom-based/web-message-manipulation">Web message manipulation</a>.</li>
+<li><a href="https://labs.detectify.com/2016/12/08/the-pitfalls-of-postmessage/">The pitfalls of postMessage</a>.</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,83 @@
+/**
+ * @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
+ * @security-severity 5
+ * @precision medium
+ * @id js/missing-origin-check
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-020
+ */
+
+import javascript
+
+/** A function that handles "message" events. */
+class PostMessageHandler extends DataFlow::FunctionNode {
+  override PostMessageEventHandler astNode;
+
+  /** Gets the parameter that contains the event. */
+  DataFlow::ParameterNode getEventParameter() {
+    result = DataFlow::parameterNode(astNode.getEventParameter())
+  }
+}
+
+/** 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()))
+  or
+  // "safeOrigin".startsWith(event.origin)
+  exists(StringOps::StartsWith starts |
+    origin(DataFlow::TypeTracker::end(), handler).flowsTo(starts.getSubstring())
+  )
+  or
+  // "safeOrigin".endsWith(event.origin)
+  exists(StringOps::EndsWith ends |
+    origin(DataFlow::TypeTracker::end(), handler).flowsTo(ends.getSubstring())
+  )
+}
+
+from PostMessageHandler handler
+where not hasOriginCheck(handler)
+select handler.getEventParameter(), "Postmessage handler has no origin check."

javascript/ql/src/Security/CWE-020/examples/MissingOriginCheckGood.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
     }
 }

javascript/ql/src/change-notes/2022-04-12-postmessage-origin-verification.md

@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+* The `js/missing-origin-check` query has been added. It highlights "message" event handlers that do not check the origin of the event.  
+  The query previously existed as the experimental `js/missing-postmessageorigin-verification` query.

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

@@ -1,40 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-
-<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>
-</overview>
-
-<recommendation>
-<p>
-Always verify the sender's identity 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>
-<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>
-<sample src="examples/postMessageWithOriginCheck.js" />
-
-</example>
-
-<references>
-
-<li><a href="https://cwe.mitre.org/data/definitions/20.html">CWE-020: Improper Input Validation</a></li>
-<li><a href="https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage">Window.postMessage()</a></li>
-<li><a href="https://portswigger.net/web-security/dom-based/web-message-manipulation">Web-message manipulation</a></li>
-<li><a href="https://labs.detectify.com/2016/12/08/the-pitfalls-of-postmessage/">The pitfalls of postMessage</a></li>
-
-</references>
-</qhelp>

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

@@ -1,62 +0,0 @@
-/**
- * @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.
- * @kind problem
- * @problem.severity warning
- * @precision high
- * @id js/missing-postmessageorigin-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`.
- */
-class PostMessageHandler extends DataFlow::FunctionNode {
-  PostMessageHandler() { this.getFunction() instanceof PostMessageEventHandler }
-}
-
-/**
- * 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
-  }
-}
-
-from PostMessageEvent event
-where not event.hasOriginChecked() or event.hasOriginInsufficientlyChecked()
-select event, "Missing or unsafe origin verification."

javascript/ql/src/experimental/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/test/query-tests/Security/CWE-020/MissingOriginCheck/MissingOriginCheck.expected

@@ -0,0 +1,3 @@
+| tst.js:11:20:11:24 | event | Postmessage handler has no origin check. |
+| tst.js:24:27:24:27 | e | Postmessage handler has no origin check. |
+| tst.js:40:27:40:27 | e | Postmessage handler has no origin check. |

javascript/ql/test/query-tests/Security/CWE-020/MissingOriginCheck/MissingOriginCheck.qlref

@@ -0,0 +1 @@
+Security/CWE-020/MissingOriginCheck.ql

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

@@ -0,0 +1,70 @@
+window.onmessage = event => { // OK - good origin check
+    let origin = event.origin.toLowerCase();
+
+    if (origin !== window.location.origin) {
+        return;
+    }
+  
+    eval(event.data);
+}
+
+window.onmessage = event => { // NOT OK - no origin check
+    let origin = event.origin.toLowerCase();
+
+    console.log(origin);
+    eval(event.data);
+}
+
+window.onmessage = event => { // OK - there is an origin check
+    if (event.origin === "https://www.example.com") {
+      // do something
+    }
+}
+
+self.onmessage = function(e) { // NOT OK
+    Commands[e.data.cmd].apply(null, e.data.args);
+};
+
+window.onmessage = event => { // OK - there is an origin check
+    if (mySet.includes(event.origin)) {
+        // do something
+    }
+}
+
+window.onmessage = event => { // OK - there is an origin check
+    if (mySet.includes(event.source)) {
+        // do something
+    }
+}
+
+self.onmessage = function(e) { // NOT OK
+    Commands[e.data.cmd].apply(null, e.data.args);
+};
+
+window.addEventListener('message', function(e) { // OK - has a good origin check
+    if (is_sysend_post_message(e) && is_valid_origin(e.origin)) {
+        var payload = JSON.parse(e.data);
+        if (payload && payload.name === uniq_prefix) {
+            var data = unserialize(payload.data);
+            sysend.broadcast(payload.key, data);
+        }
+    }
+});
+
+function is_valid_origin(origin) {
+    if (!domains) {
+        warn("no domains configured");
+        return true;
+    }
+    var valid = domains.includes(origin);
+    if (!valid) {
+        warn("invalid origin: " + origin);
+    }
+    return valid;
+}
+
+window.onmessage = event => { // OK - the check is OK
+    if ("https://www.example.com".startsWith(event.origin)) {
+      // do something
+    }
+}

javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected

@@ -1,13 +1,3 @@
-| tst-IncompleteHostnameRegExp.js:2:2:2:24 | /^http: ... le.com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:3:2:3:29 | /^http: ... le.com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:5:2:5:29 | /^http: ... le.net/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:6:2:6:43 | /^http: ... b).com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:11:13:11:38 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:12:14:12:39 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:15:22:15:47 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:19:17:19:35 | '^test.example.com' | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:40:2:40:30 | /^https ... le.com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
-| tst-IncompleteHostnameRegExp.js:55:13:55:39 | '^http: ... le.com' | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
 | tst-SemiAnchoredRegExp.js:3:2:3:7 | /^a\|b/ | Misleading operator precedence. The subexpression '^a' is anchored at the beginning, but the other parts of this regular expression are not |
 | tst-SemiAnchoredRegExp.js:6:2:6:9 | /^a\|b\|c/ | Misleading operator precedence. The subexpression '^a' is anchored at the beginning, but the other parts of this regular expression are not |
 | tst-SemiAnchoredRegExp.js:12:2:12:9 | /^a\|(b)/ | Misleading operator precedence. The subexpression '^a' is anchored at the beginning, but the other parts of this regular expression are not |

javascript/ql/test/query-tests/Security/CWE-020/UselessCharacterEscape/UselessCharacterEscape.expected

@@ -1,6 +1,3 @@
-| tst-IncompleteHostnameRegExp.js:42:13:42:65 | '^http[ ... \\/(.+)' | The escape sequence '\\/' is equivalent to just '/'. |
-| tst-SemiAnchoredRegExp.js:72:13:72:40 | '^good\\ ... \\\\.com' | The escape sequence '\\.' is equivalent to just '.'. |
-| tst-SemiAnchoredRegExp.js:109:2:109:45 | /^((\\+\| ... ?\\d\\d)/ | The escape sequence '\\:' is equivalent to just ':'. |
 | tst-escapes.js:19:8:19:11 | "\\ " | The escape sequence '\\ ' is equivalent to just ' '. |
 | tst-escapes.js:20:1:20:54 | /\\a\\b\\c ... x\\y\\z"/ | The escape sequence '\\a' is equivalent to just 'a'. |
 | tst-escapes.js:20:1:20:54 | /\\a\\b\\c ... x\\y\\z"/ | The escape sequence '\\e' is equivalent to just 'e'. |

javascript/ql/test/query-tests/Security/CWE-020/UselessCharacterEscape/UselessRegExpCharacterEscape.expected

@@ -1,6 +1,3 @@
-| tst-IncompleteHostnameRegExp.js:55:26:55:27 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-IncompleteHostnameRegExp.js:55:13:55:39 | '^http: ... le.com' | regular expression |
-| tst-SemiAnchoredRegExp.js:70:19:70:20 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-SemiAnchoredRegExp.js:70:13:70:36 | '^good\\ ... r\\.com' | regular expression |
-| tst-SemiAnchoredRegExp.js:70:31:70:32 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-SemiAnchoredRegExp.js:70:13:70:36 | '^good\\ ... r\\.com' | regular expression |
 | tst-escapes.js:13:11:13:12 | '\\b' is a backspace, and not a word-boundary assertion | The escape sequence '\\b' is a backspace, and not a word-boundary assertion when it is used in a $@. | tst-escapes.js:13:8:13:61 | "\\a\\b\\c ... \\x\\y\\z" | regular expression |
 | tst-escapes.js:13:13:13:14 | '\\c' is equivalent to just 'c', so the sequence is not a character class | The escape sequence '\\c' is equivalent to just 'c', so the sequence is not a character class when it is used in a $@. | tst-escapes.js:13:8:13:61 | "\\a\\b\\c ... \\x\\y\\z" | regular expression |
 | tst-escapes.js:13:15:13:16 | '\\d' is equivalent to just 'd', so the sequence is not a character class | The escape sequence '\\d' is equivalent to just 'd', so the sequence is not a character class when it is used in a $@. | tst-escapes.js:13:8:13:61 | "\\a\\b\\c ... \\x\\y\\z" | regular expression |

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/addEventListener.js

@@ -14,4 +14,17 @@ function test() {
     }
 
     window.addEventListener("message", foo.bind(null, {data: 'items'}));
+
+    window.onmessage = e => {
+        if (e.origin !== "https://foobar.com") {
+            return;
+        }
+        document.write(e.data); // OK - there is an origin check
+    }
+
+    window.onmessage = e => {
+        if (mySet.includes(e.origin)) {
+            document.write(e.data); // OK - there is an origin check
+        }
+    }
 }