Merge pull request #17243 from asgerf/js/post-message-source-client-side

asgerf · web-flow · commit 3be219c79d8f · 2024-08-19T11:09:26.000+02:00
JS: Classify post-message events as client side taint sources
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll
@@ -237,6 +237,15 @@ module ClientSideUrlRedirect {
     override predicate isXssSink() { any() }
   }
 
+  /**
+   * A `templateUrl` member of an AngularJS directive.
+   */
+  private class AngularJSTemplateUrlSink extends Sink {
+    AngularJSTemplateUrlSink() { this = any(AngularJS::CustomDirective d).getMember("templateUrl") }
+
+    override predicate isXssSink() { any() }
+  }
+
   private class SinkFromModel extends Sink {
     SinkFromModel() { this = ModelOutput::getASinkNode("url-redirection").asSink() }
   }
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DOM.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DOM.qll
@@ -207,12 +207,14 @@ class PostMessageEventHandler extends Function {
  * An event parameter for a `postMessage` event handler, considered as an untrusted
  * source of data.
  */
-private class PostMessageEventParameter extends RemoteFlowSource {
+private class PostMessageEventParameter extends ClientSideRemoteFlowSource {
   PostMessageEventParameter() {
     this = DataFlow::parameterNode(any(PostMessageEventHandler pmeh).getEventParameter())
   }
 
   override string getSourceType() { result = "postMessage event" }
+
+  override ClientSideRemoteFlowKind getKind() { result.isMessageEvent() }
 }
 
 /**
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/RemoteFlowSources.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/RemoteFlowSources.qll
@@ -40,7 +40,9 @@ import Cached
  * A type of remote flow source that is specific to the browser environment.
  */
 class ClientSideRemoteFlowKind extends string {
-  ClientSideRemoteFlowKind() { this = ["query", "fragment", "path", "url", "name"] }
+  ClientSideRemoteFlowKind() {
+    this = ["query", "fragment", "path", "url", "name", "message-event"]
+  }
 
   /**
    * Holds if this is the `query` kind, describing sources derived from the query parameters of the browser URL,
@@ -77,6 +79,12 @@ class ClientSideRemoteFlowKind extends string {
 
   /** Holds if this is the `name` kind, describing sources derived from the window name, such as `window.name`. */
   predicate isWindowName() { this = "name" }
+
+  /**
+   * Holds if this is the `message-event` kind, describing sources derived from cross-window message passing,
+   * such as `event` in `window.onmessage = event => {...}`.
+   */
+  predicate isMessageEvent() { this = "message-event" }
 }
 
 /**
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -653,10 +653,11 @@ module TaintedPath {
   }
 
   /**
-   * A `templateUrl` member of an AngularJS directive.
+   * DEPRECATED. This is no longer seen as a path-injection sink. It is tentatively handled
+   * by the client-side URL redirection query for now.
    */
-  class AngularJSTemplateUrlSink extends Sink, DataFlow::ValueNode {
-    AngularJSTemplateUrlSink() { this = any(AngularJS::CustomDirective d).getMember("templateUrl") }
+  deprecated class AngularJSTemplateUrlSink extends DataFlow::ValueNode instanceof Sink {
+    AngularJSTemplateUrlSink() { none() }
   }
 
   /**
diff --git a/javascript/ql/src/change-notes/2024-08-16-post-message-source-client-side.md b/javascript/ql/src/change-notes/2024-08-16-post-message-source-client-side.md
@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* Message events in the browser are now properly classified as client-side taint sources. Previously they were
+  incorrectly classified as server-side taint sources, which resulted in some alerts being reported by
+  the wrong query, such as server-side URL redirection instead of client-side URL redirection.
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js
@@ -60,18 +60,6 @@ var server = http.createServer(function(req, res) {
   res.write(fs.readFileSync(pathModule.toNamespacedPath(path)));
 });
 
-angular.module('myApp', [])
-    .directive('myCustomer', function() {
-        return {
-            templateUrl: "SAFE" // OK
-        }
-    })
-    .directive('myCustomer', function() {
-        return {
-            templateUrl: Cookie.get("unsafe") // NOT OK
-        }
-    })
-
 var server = http.createServer(function(req, res) {
     // tests for a few uri-libraries
     res.write(fs.readFileSync(require("querystringify").parse(req.url).query)); // NOT OK
@@ -92,10 +80,6 @@ var server = http.createServer(function(req, res) {
 
 })();
 
-addEventListener('message', (ev) => {
-  Cookie.set("unsafe", ev.data);
-});
-
 var server = http.createServer(function(req, res) {
 	let path = url.parse(req.url, true).query.path;
 
@@ -110,25 +94,25 @@ var server = http.createServer(function(req, res) {
 
 var server = http.createServer(function(req, res) {
   let path = url.parse(req.url, true).query.path;
-  
+
   if (path) { // sanitization
     path = path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''); // remove all invalid characters from states plus slashes
     path = path.replace(/\.\./g, ''); // remove all ".."
   }
-  
+
   res.write(fs.readFileSync(path));  // OK. Is sanitized above.
 });
 
 var server = http.createServer(function(req, res) {
   let path = url.parse(req.url, true).query.path;
-  
+
   if (!path) {
-    
+
   } else { // sanitization
 	  path = path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''); // remove all invalid characters from states plus slashes
     path = path.replace(/\.\./g, ''); // remove all ".."
   }
-  
+
   res.write(fs.readFileSync(path));  // OK. Is sanitized above.
 });
 
@@ -142,29 +126,29 @@ var server = http.createServer(function(req, res) {
   let path = url.parse(req.url, true).query.path;
 
   fs.readFileSync(path); // NOT OK
-  
+
   var split = path.split("/");
-  
+
   fs.readFileSync(split.join("/")); // NOT OK
 
   fs.readFileSync(prefix + split[split.length - 1]) // OK
 
   fs.readFileSync(split[x]) // NOT OK
-  fs.readFileSync(prefix + split[x]) // NOT OK 
+  fs.readFileSync(prefix + split[x]) // NOT OK
 
   var concatted = prefix.concat(split);
   fs.readFileSync(concatted.join("/")); // NOT OK
 
   var concatted2 = split.concat(prefix);
   fs.readFileSync(concatted2.join("/")); // NOT OK
 
-  fs.readFileSync(split.pop()); // NOT OK 
+  fs.readFileSync(split.pop()); // NOT OK
 
 });
 
 var server = http.createServer(function(req, res) {
   let path = url.parse(req.url, true).query.path;
-  
+
   // Removal of forward-slash or dots.
   res.write(fs.readFileSync(path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''))); // OK.
   res.write(fs.readFileSync(path.replace(/[abcd]/g, ''))); // NOT OK
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -86,6 +86,12 @@ nodes
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
+| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
+| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
+| angular-tempate-url.js:13:30:13:31 | ev |
+| angular-tempate-url.js:13:30:13:31 | ev |
+| angular-tempate-url.js:14:26:14:27 | ev |
+| angular-tempate-url.js:14:26:14:32 | ev.data |
 | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:47:7:69 | classNa ... w.name) |
@@ -1275,6 +1281,11 @@ edges
 | angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url |
 | angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
+| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
+| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
+| angular-tempate-url.js:14:26:14:27 | ev | angular-tempate-url.js:14:26:14:32 | ev.data |
+| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
+| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
 | classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:58:7:68 | window.name | classnames.js:7:47:7:69 | classNa ... w.name) |
@@ -2407,6 +2418,7 @@ edges
 | angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url | Cross-site scripting vulnerability due to $@. | angular2-client.ts:38:44:38:58 | this.router.url | user-provided value |
 | angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url | Cross-site scripting vulnerability due to $@. | angular2-client.ts:40:45:40:59 | this.router.url | user-provided value |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | Cross-site scripting vulnerability due to $@. | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | user-provided value |
+| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") | angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") | Cross-site scripting vulnerability due to $@. | angular-tempate-url.js:13:30:13:31 | ev | user-provided value |
 | classnames.js:7:31:7:84 | `<span  ... <span>` | classnames.js:7:58:7:68 | window.name | classnames.js:7:31:7:84 | `<span  ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:7:58:7:68 | window.name | user-provided value |
 | classnames.js:8:31:8:85 | `<span  ... <span>` | classnames.js:8:59:8:69 | window.name | classnames.js:8:31:8:85 | `<span  ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:8:59:8:69 | window.name | user-provided value |
 | classnames.js:9:31:9:85 | `<span  ... <span>` | classnames.js:9:59:9:69 | window.name | classnames.js:9:31:9:85 | `<span  ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:9:59:9:69 | window.name | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
@@ -86,6 +86,12 @@ nodes
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
+| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
+| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
+| angular-tempate-url.js:13:30:13:31 | ev |
+| angular-tempate-url.js:13:30:13:31 | ev |
+| angular-tempate-url.js:14:26:14:27 | ev |
+| angular-tempate-url.js:14:26:14:32 | ev.data |
 | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:47:7:69 | classNa ... w.name) |
@@ -1325,6 +1331,11 @@ edges
 | angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url |
 | angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url |
 | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
+| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
+| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
+| angular-tempate-url.js:14:26:14:27 | ev | angular-tempate-url.js:14:26:14:32 | ev.data |
+| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
+| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
 | classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span  ... <span>` |
 | classnames.js:7:58:7:68 | window.name | classnames.js:7:47:7:69 | classNa ... w.name) |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/angular-tempate-url.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/angular-tempate-url.js
@@ -0,0 +1,15 @@
+angular.module('myApp', [])
+    .directive('myCustomer', function() {
+        return {
+            templateUrl: "SAFE" // OK
+        }
+    })
+    .directive('myCustomer', function() {
+        return {
+            templateUrl: Cookie.get("unsafe") // NOT OK
+        }
+    });
+
+addEventListener('message', (ev) => {
+    Cookie.set("unsafe", ev.data);
+});
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected b/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected
@@ -64,15 +64,13 @@ nodes
 | RegExpInjection.js:93:20:93:31 | process.argv |
 | RegExpInjection.js:93:20:93:31 | process.argv |
 | RegExpInjection.js:93:20:93:34 | process.argv[1] |
-| tst.js:1:46:1:46 | e |
-| tst.js:1:46:1:46 | e |
-| tst.js:2:9:2:21 | data |
-| tst.js:2:16:2:16 | e |
-| tst.js:2:16:2:21 | e.data |
-| tst.js:3:16:3:35 | "^"+ data.name + "$" |
-| tst.js:3:16:3:35 | "^"+ data.name + "$" |
-| tst.js:3:21:3:24 | data |
-| tst.js:3:21:3:29 | data.name |
+| tst.js:5:9:5:29 | data |
+| tst.js:5:16:5:29 | req.query.data |
+| tst.js:5:16:5:29 | req.query.data |
+| tst.js:6:16:6:35 | "^"+ data.name + "$" |
+| tst.js:6:16:6:35 | "^"+ data.name + "$" |
+| tst.js:6:21:6:24 | data |
+| tst.js:6:21:6:29 | data.name |
 edges
 | RegExpInjection.js:5:7:5:28 | key | RegExpInjection.js:8:31:8:33 | key |
 | RegExpInjection.js:5:7:5:28 | key | RegExpInjection.js:19:19:19:21 | key |
@@ -135,14 +133,12 @@ edges
 | RegExpInjection.js:93:20:93:31 | process.argv | RegExpInjection.js:93:20:93:34 | process.argv[1] |
 | RegExpInjection.js:93:20:93:34 | process.argv[1] | RegExpInjection.js:93:16:93:49 | `^${pro ... r.app$` |
 | RegExpInjection.js:93:20:93:34 | process.argv[1] | RegExpInjection.js:93:16:93:49 | `^${pro ... r.app$` |
-| tst.js:1:46:1:46 | e | tst.js:2:16:2:16 | e |
-| tst.js:1:46:1:46 | e | tst.js:2:16:2:16 | e |
-| tst.js:2:9:2:21 | data | tst.js:3:21:3:24 | data |
-| tst.js:2:16:2:16 | e | tst.js:2:16:2:21 | e.data |
-| tst.js:2:16:2:21 | e.data | tst.js:2:9:2:21 | data |
-| tst.js:3:21:3:24 | data | tst.js:3:21:3:29 | data.name |
-| tst.js:3:21:3:29 | data.name | tst.js:3:16:3:35 | "^"+ data.name + "$" |
-| tst.js:3:21:3:29 | data.name | tst.js:3:16:3:35 | "^"+ data.name + "$" |
+| tst.js:5:9:5:29 | data | tst.js:6:21:6:24 | data |
+| tst.js:5:16:5:29 | req.query.data | tst.js:5:9:5:29 | data |
+| tst.js:5:16:5:29 | req.query.data | tst.js:5:9:5:29 | data |
+| tst.js:6:21:6:24 | data | tst.js:6:21:6:29 | data.name |
+| tst.js:6:21:6:29 | data.name | tst.js:6:16:6:35 | "^"+ data.name + "$" |
+| tst.js:6:21:6:29 | data.name | tst.js:6:16:6:35 | "^"+ data.name + "$" |
 #select
 | RegExpInjection.js:8:23:8:45 | "\\\\b" + ... (.*)\\n" | RegExpInjection.js:5:13:5:28 | req.param("key") | RegExpInjection.js:8:23:8:45 | "\\\\b" + ... (.*)\\n" | This regular expression is constructed from a $@. | RegExpInjection.js:5:13:5:28 | req.param("key") | user-provided value |
 | RegExpInjection.js:19:14:19:22 | wrap(key) | RegExpInjection.js:5:13:5:28 | req.param("key") | RegExpInjection.js:19:14:19:22 | wrap(key) | This regular expression is constructed from a $@. | RegExpInjection.js:5:13:5:28 | req.param("key") | user-provided value |
@@ -161,4 +157,4 @@ edges
 | RegExpInjection.js:87:14:87:55 | "^.*\\.( ...  + ")$" | RegExpInjection.js:82:15:82:32 | req.param("input") | RegExpInjection.js:87:14:87:55 | "^.*\\.( ...  + ")$" | This regular expression is constructed from a $@. | RegExpInjection.js:82:15:82:32 | req.param("input") | user-provided value |
 | RegExpInjection.js:91:16:91:50 | `^${pro ... r.app$` | RegExpInjection.js:91:20:91:30 | process.env | RegExpInjection.js:91:16:91:50 | `^${pro ... r.app$` | This regular expression is constructed from a $@. | RegExpInjection.js:91:20:91:30 | process.env | environment variable |
 | RegExpInjection.js:93:16:93:49 | `^${pro ... r.app$` | RegExpInjection.js:93:20:93:31 | process.argv | RegExpInjection.js:93:16:93:49 | `^${pro ... r.app$` | This regular expression is constructed from a $@. | RegExpInjection.js:93:20:93:31 | process.argv | command-line argument |
-| tst.js:3:16:3:35 | "^"+ data.name + "$" | tst.js:1:46:1:46 | e | tst.js:3:16:3:35 | "^"+ data.name + "$" | This regular expression is constructed from a $@. | tst.js:1:46:1:46 | e | user-provided value |
+| tst.js:6:16:6:35 | "^"+ data.name + "$" | tst.js:5:16:5:29 | req.query.data | tst.js:6:16:6:35 | "^"+ data.name + "$" | This regular expression is constructed from a $@. | tst.js:5:16:5:29 | req.query.data | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/tst.js b/javascript/ql/test/query-tests/Security/CWE-730/tst.js
@@ -1,12 +1,7 @@
-window.addEventListener("message", function (e) {
-    let data = e.data;
-    new RegExp("^"+ data.name + "$", "i"); // NOT OK
-});
+const express = require('express');
+const app = express();
 
-const SOMEONE_I_TRUST = "myself";
-window.addEventListener("message", function (e) {
-    if (e.origin === SOMEONE_I_TRUST) {
-        let data = e.data;
-        new RegExp("^"+ data.name + "$", "i"); // OK
-    }
+app.get('/foo', (req, res) => {
+    let data = req.query.data;
+    new RegExp("^"+ data.name + "$", "i"); // NOT OK
 });

Original file line number	Diff line number	Diff line change
`@@ -207,12 +207,14 @@ class PostMessageEventHandler extends Function {`
`207`	`207`	* An event parameter for a `postMessage` event handler, considered as an untrusted
`208`	`208`	`* source of data.`
`209`	`209`	`*/`
`210`		`-private class PostMessageEventParameter extends RemoteFlowSource {`
	`210`	`+private class PostMessageEventParameter extends ClientSideRemoteFlowSource {`
`211`	`211`	`PostMessageEventParameter() {`
`212`	`212`	`this = DataFlow::parameterNode(any(PostMessageEventHandler pmeh).getEventParameter())`
`213`	`213`	`}`
`214`	`214`
`215`	`215`	`override string getSourceType() { result = "postMessage event" }`
	`216`	`+`
	`217`	`+ override ClientSideRemoteFlowKind getKind() { result.isMessageEvent() }`
`216`	`218`	`}`
`217`	`219`
`218`	`220`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -653,10 +653,11 @@ module TaintedPath {`
`653`	`653`	`}`
`654`	`654`
`655`	`655`	`/**`
`656`		- * A `templateUrl` member of an AngularJS directive.
	`656`	`+ * DEPRECATED. This is no longer seen as a path-injection sink. It is tentatively handled`
	`657`	`+ * by the client-side URL redirection query for now.`
`657`	`658`	`*/`
`658`		`- class AngularJSTemplateUrlSink extends Sink, DataFlow::ValueNode {`
`659`		`- AngularJSTemplateUrlSink() { this = any(AngularJS::CustomDirective d).getMember("templateUrl") }`
	`659`	`+ deprecated class AngularJSTemplateUrlSink extends DataFlow::ValueNode instanceof Sink {`
	`660`	`+ AngularJSTemplateUrlSink() { none() }`
`660`	`661`	`}`
`661`	`662`
`662`	`663`	`/**`