From ea37665ec69a93b83995349a49763b7a96955e6d Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Tue, 11 Sep 2018 14:27:08 +0200 Subject: [PATCH 1/4] JS: move array-specific taint steps to separate class --- .../javascript/dataflow/TaintTracking.qll | 59 ++++++++++++------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 671729b9b2f3..03b164172842 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -185,18 +185,44 @@ module TaintTracking { override predicate step(DataFlow::Node pred, DataFlow::Node succ) { succ = this and - ( - exists (Expr e, Expr f | e = this.asExpr() and f = pred.asExpr() | - // arrays with tainted elements and objects with tainted property names are tainted - e.(ArrayExpr).getAnElement() = f or - exists (Property prop | e.(ObjectExpr).getAProperty() = prop | - prop.isComputed() and f = prop.getNameExpr() - ) - or - // awaiting a tainted expression gives a tainted result - e.(AwaitExpr).getOperand() = f + exists (Expr e, Expr f | e = this.asExpr() and f = pred.asExpr() | + // arrays with tainted elements and objects with tainted property names are tainted + e.(ArrayExpr).getAnElement() = f or + exists (Property prop | e.(ObjectExpr).getAProperty() = prop | + prop.isComputed() and f = prop.getNameExpr() ) or + // awaiting a tainted expression gives a tainted result + e.(AwaitExpr).getOperand() = f + ) + or + // reading from a tainted object yields a tainted result + this = succ and + succ.(DataFlow::PropRead).getBase() = pred + or + // iterating over a tainted iterator taints the loop variable + exists (EnhancedForLoop efl, SsaExplicitDefinition ssa | + this = DataFlow::valueNode(efl.getIterationDomain()) and + pred = this and + ssa.getDef() = efl.getIteratorExpr() and + succ = DataFlow::ssaDefinitionNode(ssa) + ) + } + } + + /** + * A taint propagating data flow edge caused by the builtin array functions. + */ + private class ArrayFunctionTaintStep extends AdditionalTaintStep { + + ArrayFunctionTaintStep() { + this = DataFlow::valueNode(_) or + this = DataFlow::parameterNode(_) or + this instanceof DataFlow::PropRead + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + succ = this and ( // `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are // `elt` and `ary`; similar for `forEach` exists (MethodCallExpr m, Function f, int i, SimpleParameter p | @@ -218,19 +244,8 @@ module TaintTracking { // `array.push(e)`: if `e` is tainted, then so is `array` succ.(DataFlow::SourceNode).getAMethodCall("push").getAnArgument() = pred ) - or - // reading from a tainted object yields a tainted result - this = succ and - succ.(DataFlow::PropRead).getBase() = pred - or - // iterating over a tainted iterator taints the loop variable - exists (EnhancedForLoop efl, SsaExplicitDefinition ssa | - this = DataFlow::valueNode(efl.getIterationDomain()) and - pred = this and - ssa.getDef() = efl.getIteratorExpr() and - succ = DataFlow::ssaDefinitionNode(ssa) - ) } + } /** From 763da72ce5275890d0be82fd9adcbe47b2ef53b3 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Tue, 11 Sep 2018 15:09:35 +0200 Subject: [PATCH 2/4] JS: modernize old array taint steps --- .../javascript/dataflow/TaintTracking.qll | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 03b164172842..2ae0a5ae4330 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -214,36 +214,33 @@ module TaintTracking { * A taint propagating data flow edge caused by the builtin array functions. */ private class ArrayFunctionTaintStep extends AdditionalTaintStep { + DataFlow::CallNode call; ArrayFunctionTaintStep() { - this = DataFlow::valueNode(_) or - this = DataFlow::parameterNode(_) or - this instanceof DataFlow::PropRead + this = call } override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - succ = this and ( - // `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are - // `elt` and `ary`; similar for `forEach` - exists (MethodCallExpr m, Function f, int i, SimpleParameter p | - (m.getMethodName() = "map" or m.getMethodName() = "forEach") and - (i = 0 or i = 2) and - m.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and - p = f.getParameter(i) and - this = DataFlow::parameterNode(p) and - pred.asExpr() = m.getReceiver() - ) - or - // `array.map` with tainted return value in callback - exists (MethodCallExpr m, Function f | - this.asExpr() = m and - m.getMethodName() = "map" and - m.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow - pred = f.getAReturnedExpr().flow()) - or - // `array.push(e)`: if `e` is tainted, then so is `array` - succ.(DataFlow::SourceNode).getAMethodCall("push").getAnArgument() = pred + // `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are + // `elt` and `ary`; similar for `forEach` + exists (string name, Function f, int i | + (name = "map" or name = "forEach") and + (i = 0 or i = 2) and + call.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and + pred.(DataFlow::SourceNode).getAMethodCall(name) = call and + succ = DataFlow::parameterNode(f.getParameter(i)) ) + or + // `array.map` with tainted return value in callback + exists (DataFlow::FunctionNode f | + call.(DataFlow::MethodCallNode).getMethodName() = "map" and + call.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow + pred = f.getAReturn() and + succ = call + ) + or + // `array.push(e)`: if `e` is tainted, then so is `array` + succ.(DataFlow::SourceNode).getAMethodCall("push") = call } } From 4c13e6b46b182d733bba356479feeab5dbeec8bf Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Tue, 11 Sep 2018 15:10:59 +0200 Subject: [PATCH 3/4] JS: add additional array-specific taint steps --- .../javascript/dataflow/TaintTracking.qll | 24 +++++++++++++++-- .../TaintTracking/BasicTaintTracking.expected | 12 +++++++++ .../test/library-tests/TaintTracking/tst.js | 26 +++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 2ae0a5ae4330..55f8ec8b9e7e 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -239,8 +239,28 @@ module TaintTracking { succ = call ) or - // `array.push(e)`: if `e` is tainted, then so is `array` - succ.(DataFlow::SourceNode).getAMethodCall("push") = call + // `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`. + exists (string name | + name = "push" or + name = "unshift" | + pred = call.getAnArgument() and + succ.(DataFlow::SourceNode).getAMethodCall(name) = call + ) + or + // `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`. + exists (string name | + name = "pop" or + name = "shift" or + name = "slice" or + name = "splice" | + call.(DataFlow::MethodCallNode).calls(pred, name) and + succ = call + ) + or + // `e = Array.from(x)`: if `x` is tainted, then so is `e`. + call = DataFlow::globalVarRef("Array").getAPropertyRead("from").getACall() and + pred = call.getAnArgument() and + succ = call } } diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index a30f0b1bf2bb..6dcfbe315bfd 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -3,3 +3,15 @@ | tst.js:2:13:2:20 | source() | tst.js:14:10:14:17 | x.sort() | | tst.js:2:13:2:20 | source() | tst.js:17:10:17:10 | a | | tst.js:2:13:2:20 | source() | tst.js:19:10:19:10 | a | +| tst.js:2:13:2:20 | source() | tst.js:23:10:23:10 | b | +| tst.js:2:13:2:20 | source() | tst.js:25:10:25:16 | x.pop() | +| tst.js:2:13:2:20 | source() | tst.js:26:10:26:18 | x.shift() | +| tst.js:2:13:2:20 | source() | tst.js:27:10:27:18 | x.slice() | +| tst.js:2:13:2:20 | source() | tst.js:28:10:28:19 | x.splice() | +| tst.js:2:13:2:20 | source() | tst.js:30:10:30:22 | Array.from(x) | +| tst.js:2:13:2:20 | source() | tst.js:33:14:33:16 | elt | +| tst.js:2:13:2:20 | source() | tst.js:35:14:35:16 | ary | +| tst.js:2:13:2:20 | source() | tst.js:39:14:39:16 | elt | +| tst.js:2:13:2:20 | source() | tst.js:41:14:41:16 | ary | +| tst.js:2:13:2:20 | source() | tst.js:44:10:44:30 | innocen ... ) => x) | +| tst.js:2:13:2:20 | source() | tst.js:45:10:45:24 | x.map(x2 => x2) | diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index 69b5f17074ce..48c7ace4611a 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -18,4 +18,30 @@ function test() { a.push(x); sink(a); // NOT OK + var b = []; + b.unshift(x); + sink(b); // NOT OK + + sink(x.pop()); // NOT OK + sink(x.shift()); // NOT OK + sink(x.slice()); // NOT OK + sink(x.splice()); // NOT OK + + sink(Array.from(x)); // NOT OK + + x.map((elt, i, ary) => { + sink(elt); // NOT OK + sink(i); // OK + sink(ary); // NOT OK + }); + + x.forEach((elt, i, ary) => { + sink(elt); // NOT OK + sink(i); // OK + sink(ary); // NOT OK + }); + + sink(innocent.map(() => x)); // NOT OK + sink(x.map(x2 => x2)); // NOT OK + } From cb2bd9e0aef050188b38e596a5af5da94fe187b9 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 13 Sep 2018 15:52:40 +0200 Subject: [PATCH 4/4] JS: change notes for additional array taint steps --- change-notes/1.19/analysis-javascript.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 5fdf442d1289..0e9b224e26dc 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -2,6 +2,8 @@ ## General improvements +* Modelling of taint flow through array operations has been improved. This may give additional results for the security queries. + ## New queries | **Query** | **Tags** | **Purpose** |