From e55330b820719b3d0c3fbbbbc1f2d3acb01df7e6 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 5 Apr 2019 13:55:48 +0100 Subject: [PATCH] JS: Fix flow through += --- .../semmle/javascript/StringConcatenation.qll | 2 +- .../javascript/dataflow/TaintTracking.qll | 4 +++- .../ClassContainsTwo.expected | 9 ++++++++ .../StringConcatenation/ContainsTwo.expected | 9 ++++++++ .../library-tests/StringConcatenation/tst.js | 9 ++++++++ .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../library-tests/TaintTracking/addexpr.js | 22 +++++++++++++++++++ 7 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 javascript/ql/test/library-tests/TaintTracking/addexpr.js diff --git a/javascript/ql/src/semmle/javascript/StringConcatenation.qll b/javascript/ql/src/semmle/javascript/StringConcatenation.qll index 7483665f926e..bfab7da2e103 100644 --- a/javascript/ql/src/semmle/javascript/StringConcatenation.qll +++ b/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) ) } diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 2b3b50604aef..4e019f6b1d87 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/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) diff --git a/javascript/ql/test/library-tests/StringConcatenation/ClassContainsTwo.expected b/javascript/ql/test/library-tests/StringConcatenation/ClassContainsTwo.expected index 8342040bd1eb..ffdaf7807118 100644 --- a/javascript/ql/test/library-tests/StringConcatenation/ClassContainsTwo.expected +++ b/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 | diff --git a/javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected b/javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected index 8342040bd1eb..ffdaf7807118 100644 --- a/javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected +++ b/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 | diff --git a/javascript/ql/test/library-tests/StringConcatenation/tst.js b/javascript/ql/test/library-tests/StringConcatenation/tst.js index f9f42c33f17b..e3f945a6fdde 100644 --- a/javascript/ql/test/library-tests/StringConcatenation/tst.js +++ b/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; +} diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 7f4709eafaac..bba8fae4a60b 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/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 | diff --git a/javascript/ql/test/library-tests/TaintTracking/addexpr.js b/javascript/ql/test/library-tests/TaintTracking/addexpr.js new file mode 100644 index 000000000000..16166d58107d --- /dev/null +++ b/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 +}