JS: Fix flow through +=

Author: asger-semmle

javascript/ql/src/semmle/javascript/StringConcatenation.qll

@@ -10,7 +10,7 @@ module StringConcatenation {
     result = expr.flow()
     or
     exists(SsaExplicitDefinition def | def.getDef() = expr |
-      result = DataFlow::valueNode(def.getVariable().getAUse())
+      result = DataFlow::ssaDefinitionNode(def)
     )
   }
 

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -366,7 +366,9 @@ module TaintTracking {
    * Note that since we cannot easily distinguish string append from addition,
    * we consider any `+` operation to propagate taint.
    */
-  class StringConcatenationTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
+  class StringConcatenationTaintStep extends AdditionalTaintStep {
+    StringConcatenationTaintStep() { StringConcatenation::taintStep(_, this) }
+
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       succ = this and
       StringConcatenation::taintStep(pred, succ)

javascript/ql/test/library-tests/StringConcatenation/ClassContainsTwo.expected

@@ -9,9 +9,13 @@
 | tst.js:5:3:5:3 | x |
 | tst.js:5:3:5:13 | x += "four" |
 | tst.js:6:10:6:10 | x |
+| tst.js:12:5:12:5 | x |
 | tst.js:12:5:12:26 | x += "o ... + "two" |
 | tst.js:12:10:12:26 | "one" + y + "two" |
 | tst.js:12:22:12:26 | "two" |
+| tst.js:14:3:14:3 | x |
+| tst.js:14:3:14:13 | x += "last" |
+| tst.js:15:10:15:10 | x |
 | tst.js:19:11:19:23 | "one" + "two" |
 | tst.js:19:19:19:23 | "two" |
 | tst.js:20:3:20:3 | x |
@@ -33,3 +37,8 @@
 | tst.js:53:19:53:23 |  two  |
 | tst.js:71:14:71:18 | "two" |
 | tst.js:77:23:77:27 | "two" |
+| tst.js:87:5:87:14 | x += 'two' |
+| tst.js:87:10:87:14 | 'two' |
+| tst.js:89:3:89:3 | x |
+| tst.js:89:3:89:14 | x += 'three' |
+| tst.js:90:10:90:10 | x |

javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected

@@ -9,9 +9,13 @@
 | tst.js:5:3:5:3 | x |
 | tst.js:5:3:5:13 | x += "four" |
 | tst.js:6:10:6:10 | x |
+| tst.js:12:5:12:5 | x |
 | tst.js:12:5:12:26 | x += "o ... + "two" |
 | tst.js:12:10:12:26 | "one" + y + "two" |
 | tst.js:12:22:12:26 | "two" |
+| tst.js:14:3:14:3 | x |
+| tst.js:14:3:14:13 | x += "last" |
+| tst.js:15:10:15:10 | x |
 | tst.js:19:11:19:23 | "one" + "two" |
 | tst.js:19:19:19:23 | "two" |
 | tst.js:20:3:20:3 | x |
@@ -33,3 +37,8 @@
 | tst.js:53:19:53:23 |  two  |
 | tst.js:71:14:71:18 | "two" |
 | tst.js:77:23:77:27 | "two" |
+| tst.js:87:5:87:14 | x += 'two' |
+| tst.js:87:10:87:14 | 'two' |
+| tst.js:89:3:89:3 | x |
+| tst.js:89:3:89:14 | x += 'three' |
+| tst.js:90:10:90:10 | x |

javascript/ql/test/library-tests/StringConcatenation/tst.js

@@ -80,3 +80,12 @@ function joinInClosure() {
   }
   return f();
 }
+
+function addExprPhi(b) {
+  let x = 'one';
+  if (b) {
+    x += 'two';
+  }
+  x += 'three';
+  return x;
+}

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -1,3 +1,5 @@
+| addexpr.js:4:10:4:17 | source() | addexpr.js:7:8:7:8 | x |
+| addexpr.js:11:15:11:22 | source() | addexpr.js:21:8:21:12 | value |
 | advanced-callgraph.js:2:13:2:20 | source() | advanced-callgraph.js:6:22:6:22 | v |
 | callbacks.js:4:6:4:13 | source() | callbacks.js:34:27:34:27 | x |
 | callbacks.js:4:6:4:13 | source() | callbacks.js:35:27:35:27 | x |

javascript/ql/test/library-tests/TaintTracking/addexpr.js

@@ -0,0 +1,22 @@
+function test1(b) {
+  let x = 'one';
+  if (b) {
+    x += source();
+  }
+  x += 'three';
+  sink(x); // NOT OK
+}
+
+function test2(x, foo) {
+  let taint = source();
+  let value = '';
+
+  sink(value); // OK
+
+  if (x) {
+    value += taint;
+  }
+  value += foo;
+
+  sink(value); // NOT OK
+}