moving jsdom sink to js/xss

Author: erik-krogh

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

@@ -138,17 +138,4 @@ module CodeInjection {
         API::moduleImport("module").getInstance().getMember("_compile").getACall().getArgument(0)
     }
   }
-
-  /**
-   * A construction of a JSDOM object (server side DOM), where scripts are allowed.
-   */
-  class JSDomWithRunScripts extends Sink {
-    JSDomWithRunScripts() {
-      exists(DataFlow::NewNode instance |
-        instance = API::moduleImport("jsdom").getMember("JSDOM").getInstance().getAnImmediateUse() and
-        this = instance.getArgument(0) and
-        instance.getOptionArgument(1, "runScripts").mayHaveStringValue("dangerously")
-      )
-    }
-  }
 }

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

@@ -151,7 +151,7 @@ module DomBasedXss {
    * An expression whose value is interpreted as HTML
    * and may be inserted into the DOM through a library.
    */
-  class LibrarySink extends Sink, DataFlow::ValueNode {
+  class LibrarySink extends Sink {
     LibrarySink() {
       // call to a jQuery method that interprets its argument as HTML
       exists(JQuery::MethodCall call |
@@ -175,6 +175,13 @@ module DomBasedXss {
       this = any(Handlebars::SafeString s).getAnArgument()
       or
       this = any(JQuery::MethodCall call | call.getMethodName() = "jGrowl").getArgument(0)
+      or
+      // A construction of a JSDOM object (server side DOM), where scripts are allowed.
+      exists(DataFlow::NewNode instance |
+        instance = API::moduleImport("jsdom").getMember("JSDOM").getInstance().getAnImmediateUse() and
+        this = instance.getArgument(0) and
+        instance.getOptionArgument(1, "runScripts").mayHaveStringValue("dangerously")
+      )
     }
   }
 

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected

@@ -89,6 +89,9 @@ nodes
 | classnames.js:15:47:15:63 | clsx(window.name) |
 | classnames.js:15:52:15:62 | window.name |
 | classnames.js:15:52:15:62 | window.name |
+| express.js:7:15:7:33 | req.param("wobble") |
+| express.js:7:15:7:33 | req.param("wobble") |
+| express.js:7:15:7:33 | req.param("wobble") |
 | jquery.js:2:7:2:40 | tainted |
 | jquery.js:2:7:2:40 | tainted |
 | jquery.js:2:17:2:33 | document.location |
@@ -685,6 +688,7 @@ edges
 | classnames.js:15:47:15:63 | clsx(window.name) | classnames.js:15:31:15:78 | `<span  ... <span>` |
 | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
 | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
+| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
 | jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
 | jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
 | jquery.js:2:17:2:33 | document.location | jquery.js:2:17:2:40 | documen ... .search |
@@ -1175,6 +1179,7 @@ edges
 | classnames.js:11:31:11:79 | `<span  ... <span>` | classnames.js:10:45:10:55 | window.name | classnames.js:11:31:11:79 | `<span  ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:10:45:10:55 | window.name | user-provided value |
 | classnames.js:13:31:13:83 | `<span  ... <span>` | classnames.js:13:57:13:67 | window.name | classnames.js:13:31:13:83 | `<span  ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:13:57:13:67 | window.name | user-provided value |
 | classnames.js:15:31:15:78 | `<span  ... <span>` | classnames.js:15:52:15:62 | window.name | classnames.js:15:31:15:78 | `<span  ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:15:52:15:62 | window.name | user-provided value |
+| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") | Cross-site scripting vulnerability due to $@. | express.js:7:15:7:33 | req.param("wobble") | user-provided value |
 | jquery.js:7:5:7:34 | "<div i ... + "\\">" | jquery.js:2:17:2:40 | documen ... .search | jquery.js:7:5:7:34 | "<div i ... + "\\">" | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:40 | documen ... .search | user-provided value |
 | jquery.js:8:18:8:34 | "XSS: " + tainted | jquery.js:2:17:2:33 | document.location | jquery.js:8:18:8:34 | "XSS: " + tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |
 | jquery.js:10:5:10:40 | "<b>" + ...  "</b>" | jquery.js:10:13:10:20 | location | jquery.js:10:5:10:40 | "<b>" + ...  "</b>" | Cross-site scripting vulnerability due to $@. | jquery.js:10:13:10:20 | location | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected

@@ -89,6 +89,9 @@ nodes
 | classnames.js:15:47:15:63 | clsx(window.name) |
 | classnames.js:15:52:15:62 | window.name |
 | classnames.js:15:52:15:62 | window.name |
+| express.js:7:15:7:33 | req.param("wobble") |
+| express.js:7:15:7:33 | req.param("wobble") |
+| express.js:7:15:7:33 | req.param("wobble") |
 | jquery.js:2:7:2:40 | tainted |
 | jquery.js:2:7:2:40 | tainted |
 | jquery.js:2:17:2:33 | document.location |
@@ -689,6 +692,7 @@ edges
 | classnames.js:15:47:15:63 | clsx(window.name) | classnames.js:15:31:15:78 | `<span  ... <span>` |
 | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
 | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
+| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
 | jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
 | jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
 | jquery.js:2:17:2:33 | document.location | jquery.js:2:17:2:40 | documen ... .search |

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

@@ -0,0 +1,11 @@
+var express = require('express');
+var app = express();
+
+import { JSDOM } from "jsdom";
+app.get('/some/path', function (req, res) {
+    // NOT OK
+    new JSDOM(req.param("wobble"), { runScripts: "dangerously" });
+
+    // OK
+    new JSDOM(req.param("wobble"), { runScripts: "outside-only" });
+});

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected

@@ -108,9 +108,6 @@ nodes
 | express.js:21:19:21:48 | req.par ... ntext") |
 | express.js:21:19:21:48 | req.par ... ntext") |
 | express.js:21:19:21:48 | req.par ... ntext") |
-| express.js:28:13:28:31 | req.param("wobble") |
-| express.js:28:13:28:31 | req.param("wobble") |
-| express.js:28:13:28:31 | req.param("wobble") |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
@@ -252,7 +249,6 @@ edges
 | express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") |
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
-| express.js:28:13:28:31 | req.param("wobble") | express.js:28:13:28:31 | req.param("wobble") |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
@@ -316,7 +312,6 @@ edges
 | express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") | $@ flows to here and is interpreted as code. | express.js:17:30:17:53 | req.par ... cript") | User-provided value |
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | $@ flows to here and is interpreted as code. | express.js:19:37:19:70 | req.par ... odule") | User-provided value |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | $@ flows to here and is interpreted as code. | express.js:21:19:21:48 | req.par ... ntext") | User-provided value |
-| express.js:28:13:28:31 | req.param("wobble") | express.js:28:13:28:31 | req.param("wobble") | express.js:28:13:28:31 | req.param("wobble") | $@ flows to here and is interpreted as code. | express.js:28:13:28:31 | req.param("wobble") | User-provided value |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | $@ flows to here and is interpreted as code. | module.js:9:16:9:29 | req.query.code | User-provided value |
 | react-native.js:8:32:8:38 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:32:8:38 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
 | react-native.js:10:23:10:29 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:10:23:10:29 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected

@@ -112,9 +112,6 @@ nodes
 | express.js:21:19:21:48 | req.par ... ntext") |
 | express.js:21:19:21:48 | req.par ... ntext") |
 | express.js:21:19:21:48 | req.par ... ntext") |
-| express.js:28:13:28:31 | req.param("wobble") |
-| express.js:28:13:28:31 | req.param("wobble") |
-| express.js:28:13:28:31 | req.param("wobble") |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
@@ -260,7 +257,6 @@ edges
 | express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") |
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
-| express.js:28:13:28:31 | req.param("wobble") | express.js:28:13:28:31 | req.param("wobble") |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/express.js

@@ -20,13 +20,3 @@ app.get('/some/path', function(req, res) {
   // NOT OK
   vm.runInContext(req.param("code_runInContext"), vm.createContext());
 });
-
-import {JSDOM} from "jsdom";
-
-app.get('/some/path', function(req, res) {
-  // NOT OK
-  new JSDOM(req.param("wobble"), {runScripts: "dangerously"});
-
-  // OK
-  new JSDOM(req.param("wobble"), {runScripts: "outside-only"});
-});