From 2dedfb302abd3dc5799e4f8889bd73fde37fb708 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Wed, 8 Sep 2021 13:13:00 +0200
Subject: [PATCH] remove paths without unmatched returns from
 `js/prototype-polluting-assignment`

---
 .../PrototypePollutingAssignmentQuery.qll     |  8 +++++
 .../PrototypePollutingAssignment.expected     | 35 +++++++++++++++++++
 .../PrototypePollutingAssignment/lib.js       | 33 ++++++++++++++++-
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll
index 90aa77cd2fd..571135f978c 100644
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll
@@ -69,6 +69,14 @@ class Configuration extends TaintTracking::Configuration {
       inlbl.isTaint() and
       outlbl instanceof ObjectPrototype
     )
+    or
+    DataFlow::localFieldStep(pred, succ) and inlbl = outlbl
+  }
+
+  // override to require that there is a path without unmatched return steps
+  override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+    super.hasFlowPath(source, sink) and
+    DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
   }
 
   override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected
index 9f29c850275..6e2ca7874be 100644
--- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected
@@ -52,6 +52,24 @@ nodes
 | lib.js:42:3:42:14 | obj[path[0]] |
 | lib.js:42:7:42:10 | path |
 | lib.js:42:7:42:13 | path[0] |
+| lib.js:45:13:45:13 | s |
+| lib.js:45:13:45:13 | s |
+| lib.js:46:10:46:10 | s |
+| lib.js:52:9:52:22 | path |
+| lib.js:52:16:52:22 | id("x") |
+| lib.js:55:11:55:22 | obj[path[0]] |
+| lib.js:55:11:55:22 | obj[path[0]] |
+| lib.js:55:15:55:18 | path |
+| lib.js:55:15:55:21 | path[0] |
+| lib.js:59:18:59:18 | s |
+| lib.js:59:18:59:18 | s |
+| lib.js:61:17:61:17 | s |
+| lib.js:68:11:68:26 | path |
+| lib.js:68:18:68:26 | this.path |
+| lib.js:70:13:70:24 | obj[path[0]] |
+| lib.js:70:13:70:24 | obj[path[0]] |
+| lib.js:70:17:70:20 | path |
+| lib.js:70:17:70:23 | path[0] |
 | tst.js:5:9:5:38 | taint |
 | tst.js:5:17:5:38 | String( ... y.data) |
 | tst.js:5:24:5:37 | req.query.data |
@@ -141,6 +159,22 @@ edges
 | lib.js:42:7:42:10 | path | lib.js:42:7:42:13 | path[0] |
 | lib.js:42:7:42:13 | path[0] | lib.js:42:3:42:14 | obj[path[0]] |
 | lib.js:42:7:42:13 | path[0] | lib.js:42:3:42:14 | obj[path[0]] |
+| lib.js:45:13:45:13 | s | lib.js:46:10:46:10 | s |
+| lib.js:45:13:45:13 | s | lib.js:46:10:46:10 | s |
+| lib.js:46:10:46:10 | s | lib.js:52:16:52:22 | id("x") |
+| lib.js:52:9:52:22 | path | lib.js:55:15:55:18 | path |
+| lib.js:52:16:52:22 | id("x") | lib.js:52:9:52:22 | path |
+| lib.js:55:15:55:18 | path | lib.js:55:15:55:21 | path[0] |
+| lib.js:55:15:55:21 | path[0] | lib.js:55:11:55:22 | obj[path[0]] |
+| lib.js:55:15:55:21 | path[0] | lib.js:55:11:55:22 | obj[path[0]] |
+| lib.js:59:18:59:18 | s | lib.js:61:17:61:17 | s |
+| lib.js:59:18:59:18 | s | lib.js:61:17:61:17 | s |
+| lib.js:61:17:61:17 | s | lib.js:68:18:68:26 | this.path |
+| lib.js:68:11:68:26 | path | lib.js:70:17:70:20 | path |
+| lib.js:68:18:68:26 | this.path | lib.js:68:11:68:26 | path |
+| lib.js:70:17:70:20 | path | lib.js:70:17:70:23 | path[0] |
+| lib.js:70:17:70:23 | path[0] | lib.js:70:13:70:24 | obj[path[0]] |
+| lib.js:70:17:70:23 | path[0] | lib.js:70:13:70:24 | obj[path[0]] |
 | tst.js:5:9:5:38 | taint | tst.js:8:12:8:16 | taint |
 | tst.js:5:9:5:38 | taint | tst.js:9:12:9:16 | taint |
 | tst.js:5:9:5:38 | taint | tst.js:12:25:12:29 | taint |
@@ -184,6 +218,7 @@ edges
 | lib.js:26:10:26:21 | obj[path[0]] | lib.js:25:44:25:47 | path | lib.js:26:10:26:21 | obj[path[0]] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:25:44:25:47 | path | library input |
 | lib.js:34:3:34:14 | obj[path[0]] | lib.js:32:14:32:20 | args[1] | lib.js:34:3:34:14 | obj[path[0]] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:32:14:32:20 | args[1] | library input |
 | lib.js:42:3:42:14 | obj[path[0]] | lib.js:40:14:40:20 | args[1] | lib.js:42:3:42:14 | obj[path[0]] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:40:14:40:20 | args[1] | library input |
+| lib.js:70:13:70:24 | obj[path[0]] | lib.js:59:18:59:18 | s | lib.js:70:13:70:24 | obj[path[0]] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:59:18:59:18 | s | library input |
 | tst.js:8:5:8:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:8:5:8:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | user controlled input |
 | tst.js:9:5:9:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:9:5:9:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | user controlled input |
 | tst.js:14:5:14:32 | unsafeG ...  taint) | tst.js:5:24:5:37 | req.query.data | tst.js:14:5:14:32 | unsafeG ...  taint) | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | user controlled input |
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js
index 023ef1fb9a6..16f95ea609a 100644
--- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js
+++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js
@@ -40,4 +40,35 @@ module.exports.setWithArgs3 = function() {
   var path = args[1];
   var value = args[2];
   obj[path[0]][path[1]] = value; // NOT OK
-}
\ No newline at end of file
+}
+
+function id(s) {
+  return s;
+}
+
+module.exports.id = id;
+
+module.exports.notVulnerable = function () {
+  const path = id("x");
+  const value = id("y");
+  const obj = id("z");
+  return (obj[path[0]][path[1]] = value); // OK
+}
+
+class Foo {
+  constructor(o, s, v) {
+    this.obj = o;
+    this.path = s;
+    this.value = v;
+  }
+
+  doXss() {
+    // not called here, but still bad.
+    const obj = this.obj;
+    const path = this.path;
+    const value = this.value;
+    return (obj[path[0]][path[1]] = value); // NOT OK
+  }
+}
+
+module.exports.Foo = Foo;