filter away paths that start with libary inputs and end with a fixed-property write

Author: erik-krogh

javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll

@@ -70,10 +70,17 @@ class Configuration extends TaintTracking::Configuration {
     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)
+    // require that there is a path without unmatched return steps
+    DataFlow::hasPathWithoutUnmatchedReturn(source, sink) and
+    // filter away paths that start with library inputs and end with a write to a fixed property.
+    not exists(ExternalInputSource src, Sink snk, DataFlow::PropWrite write |
+      source.getNode() = src and sink.getNode() = snk
+    |
+      snk = write.getBase() and
+      exists(write.getPropertyName())
+    )
   }
 
   override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {

javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected

@@ -79,6 +79,13 @@ nodes
 | lib.js:86:19:86:25 | path[0] |
 | lib.js:87:10:87:14 | proto |
 | lib.js:87:10:87:14 | proto |
+| lib.js:90:43:90:46 | path |
+| lib.js:90:43:90:46 | path |
+| lib.js:91:7:91:28 | maybeProto |
+| lib.js:91:20:91:28 | obj[path] |
+| lib.js:91:24:91:27 | path |
+| lib.js:92:3:92:12 | maybeProto |
+| lib.js:92:3:92:12 | maybeProto |
 | 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 |
@@ -192,6 +199,12 @@ edges
 | lib.js:86:15:86:26 | obj[path[0]] | lib.js:86:7:86:26 | proto |
 | lib.js:86:19:86:22 | path | lib.js:86:19:86:25 | path[0] |
 | lib.js:86:19:86:25 | path[0] | lib.js:86:15:86:26 | obj[path[0]] |
+| lib.js:90:43:90:46 | path | lib.js:91:24:91:27 | path |
+| lib.js:90:43:90:46 | path | lib.js:91:24:91:27 | path |
+| lib.js:91:7:91:28 | maybeProto | lib.js:92:3:92:12 | maybeProto |
+| lib.js:91:7:91:28 | maybeProto | lib.js:92:3:92:12 | maybeProto |
+| lib.js:91:20:91:28 | obj[path] | lib.js:91:7:91:28 | maybeProto |
+| lib.js:91:24:91:27 | path | lib.js:91:20:91:28 | obj[path] |
 | 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 |

javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js

@@ -84,5 +84,10 @@ module.exports.delete = function() {
   delete obj[path[0]]; // OK
   var prop = arguments[2];
   var proto = obj[path[0]];
-  delete proto[prop]; // NOT
+  delete proto[prop]; // NOT OK
 }
+
+module.exports.fixedProp = function (obj, path, value) {
+  var maybeProto = obj[path];
+  maybeProto.foo = value; // OK - fixed properties from library inputs are OK.
+}