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** | diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 671729b9b2f3..55f8ec8b9e7e 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -185,38 +185,15 @@ 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 - // `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 + // awaiting a tainted expression gives a tainted result + e.(AwaitExpr).getOperand() = f ) or // reading from a tainted object yields a tainted result @@ -233,6 +210,61 @@ module TaintTracking { } } + /** + * A taint propagating data flow edge caused by the builtin array functions. + */ + private class ArrayFunctionTaintStep extends AdditionalTaintStep { + DataFlow::CallNode call; + + ArrayFunctionTaintStep() { + this = call + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + // `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)`, `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 + } + + } + /** * A taint propagating data flow edge for assignments of the form `o[k] = v`, where * `k` is not a constant and `o` refers to some object literal; in this case, we consider 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 + }