Merge pull request github#5920 from erik-krogh/clone

codeql-ci · web-flow · commit 1d120824acff · 2021-05-18T05:13:57.000-07:00
Approved by esbena
diff --git a/javascript/change-notes/2021-05-18-clone.md b/javascript/change-notes/2021-05-18-clone.md
@@ -0,0 +1,4 @@
+lgtm,codescanning
+* The dataflow libraries now model dataflow in the `clone` library.
+  Affected packages are
+    [clone](https://npmjs.com/package/clone)
diff --git a/javascript/ql/src/semmle/javascript/Extend.qll b/javascript/ql/src/semmle/javascript/Extend.qll
@@ -174,3 +174,17 @@ private class ExtendCallTaintStep extends TaintTracking::SharedTaintStep {
     )
   }
 }
+
+private import semmle.javascript.dataflow.internal.PreCallGraphStep
+
+/**
+ * A step for the `clone` package.
+ */
+private class CloneStep extends PreCallGraphStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(DataFlow::CallNode call | call = DataFlow::moduleImport("clone").getACall() |
+      pred = call.getArgument(0) and
+      succ = call
+    )
+  }
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected
@@ -174,6 +174,14 @@ nodes
 | tst2.js:18:12:18:12 | p |
 | tst2.js:21:14:21:14 | p |
 | tst2.js:21:14:21:14 | p |
+| tst2.js:30:7:30:24 | p |
+| tst2.js:30:9:30:9 | p |
+| tst2.js:30:9:30:9 | p |
+| tst2.js:33:11:33:11 | p |
+| tst2.js:36:12:36:12 | p |
+| tst2.js:36:12:36:12 | p |
+| tst2.js:37:12:37:18 | other.p |
+| tst2.js:37:12:37:18 | other.p |
 edges
 | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
 | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
@@ -318,6 +326,13 @@ edges
 | tst2.js:14:7:14:24 | p | tst2.js:21:14:21:14 | p |
 | tst2.js:14:9:14:9 | p | tst2.js:14:7:14:24 | p |
 | tst2.js:14:9:14:9 | p | tst2.js:14:7:14:24 | p |
+| tst2.js:30:7:30:24 | p | tst2.js:33:11:33:11 | p |
+| tst2.js:30:7:30:24 | p | tst2.js:36:12:36:12 | p |
+| tst2.js:30:7:30:24 | p | tst2.js:36:12:36:12 | p |
+| tst2.js:30:9:30:9 | p | tst2.js:30:7:30:24 | p |
+| tst2.js:30:9:30:9 | p | tst2.js:30:7:30:24 | p |
+| tst2.js:33:11:33:11 | p | tst2.js:37:12:37:18 | other.p |
+| tst2.js:33:11:33:11 | p | tst2.js:37:12:37:18 | other.p |
 #select
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
 | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:17:31:17:39 | params.id | user-provided value |
@@ -359,3 +374,5 @@ edges
 | tst2.js:8:12:8:12 | r | tst2.js:6:12:6:15 | q: r | tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |
 | tst2.js:18:12:18:12 | p | tst2.js:14:9:14:9 | p | tst2.js:18:12:18:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
 | tst2.js:21:14:21:14 | p | tst2.js:14:9:14:9 | p | tst2.js:21:14:21:14 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
+| tst2.js:36:12:36:12 | p | tst2.js:30:9:30:9 | p | tst2.js:36:12:36:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
+| tst2.js:37:12:37:18 | other.p | tst2.js:30:9:30:9 | p | tst2.js:37:12:37:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected
@@ -37,3 +37,5 @@
 | tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |
 | tst2.js:18:12:18:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
 | tst2.js:21:14:21:14 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
+| tst2.js:36:12:36:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
+| tst2.js:37:12:37:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js
@@ -22,3 +22,17 @@ app.get('/bar', function(req, res) {
   else
     res.send(p); // OK
 });
+
+
+const clone = require('clone');
+
+app.get('/baz', function(req, res) {
+  let { p } = req.params;
+
+  var obj = {};
+  obj.p = p;
+  var other = clone(obj);
+
+  res.send(p); // NOT OK
+  res.send(other.p); // NOT OK
+});

Original file line number	Diff line number	Diff line change
`@@ -174,3 +174,17 @@ private class ExtendCallTaintStep extends TaintTracking::SharedTaintStep {`
`174`	`174`	`)`
`175`	`175`	`}`
`176`	`176`	`}`
	`177`	`+`
	`178`	`+private import semmle.javascript.dataflow.internal.PreCallGraphStep`
	`179`	`+`
	`180`	`+/**`
	`181`	+ * A step for the `clone` package.
	`182`	`+ */`
	`183`	`+private class CloneStep extends PreCallGraphStep {`
	`184`	`+ override predicate step(DataFlow::Node pred, DataFlow::Node succ) {`
	`185`	`+ exists(DataFlow::CallNode call \| call = DataFlow::moduleImport("clone").getACall() \|`
	`186`	`+ pred = call.getArgument(0) and`
	`187`	`+ succ = call`
	`188`	`+ )`
	`189`	`+ }`
	`190`	`+}`