From 6d1eab8a4b7f6f86d079459499213bbd8f3def22 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 11 Jan 2019 12:50:59 +0000 Subject: [PATCH 1/3] JS: support flow out of "this" in constructor call --- .../javascript/dataflow/Configuration.qll | 16 ++++++++-- .../semmle/javascript/dataflow/DataFlow.qll | 7 +++++ .../dataflow/internal/FlowSteps.qll | 8 +++-- .../TaintTracking/BasicTaintTracking.expected | 6 ++++ .../TaintTracking/constructor-calls.js | 31 +++++++++++++++++++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 javascript/ql/test/library-tests/TaintTracking/constructor-calls.js diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index d31295b3e66d..81df66ddf1c1 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -606,11 +606,16 @@ private predicate storeStep( basicStoreStep(pred, succ, prop) and summary = PathSummary::level() or - exists(Function f, DataFlow::Node mid, DataFlow::Node base | + exists(Function f, DataFlow::Node mid | // `f` stores its parameter `pred` in property `prop` of a value that it returns, // and `succ` is an invocation of `f` reachableFromInput(f, succ, pred, mid, cfg, summary) and - returnedPropWrite(f, base, prop, mid) + ( + returnedPropWrite(f, _, prop, mid) + or + succ instanceof DataFlow::NewNode and + receiverPropWrite(f, prop, mid) + ) ) } @@ -622,6 +627,13 @@ predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, base.flowsToExpr(f.getAReturnedExpr()) } +/** + * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`. + */ +predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) { + DataFlow::thisNode(f).hasPropertyWrite(prop, rhs) +} + /** * Holds if `rhs` is the right-hand side of a write to property `prop`, and `nd` is reachable * from the base of that write under configuration `cfg` (possibly through callees) along a diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index d678e5774dd4..09a5ac19fc8c 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll @@ -802,6 +802,13 @@ module DataFlow { */ predicate thisNode(DataFlow::Node node, StmtContainer container) { node = TThisNode(container) } + /** + * Gets the node representing the receiver of the given function, or `this` in the given top-level. + * + * Has no result if `container` is an arrow function. + */ + DataFlow::ThisNode thisNode(StmtContainer container) { result = TThisNode(container) } + /** * A classification of flows that are not modeled, or only modeled incompletely, by * `DataFlowNode`: diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll index 2681f4c88289..e6d092d3ebc1 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -118,9 +118,11 @@ predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { * from a function call. */ predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) { - exists(Function f | - returnExpr(f, pred, _) and - calls(succ, f) + exists(Function f | calls(succ, f) | + returnExpr(f, pred, _) + or + succ instanceof DataFlow::NewNode and + DataFlow::thisNode(pred, f) ) } diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index d24e4148210e..22e824c4c545 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -1,4 +1,10 @@ | advanced-callgraph.js:2:13:2:20 | source() | advanced-callgraph.js:6:22:6:22 | v | +| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:18:8:18:14 | c.taint | +| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:22:8:22:19 | c_safe.taint | +| constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:26:8:26:14 | d.taint | +| constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:30:8:30:19 | d_safe.taint | +| constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:17:8:17:14 | c.param | +| constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:25:8:25:14 | d.param | | partialCalls.js:4:17:4:24 | source() | partialCalls.js:17:14:17:14 | x | | partialCalls.js:4:17:4:24 | source() | partialCalls.js:20:14:20:14 | y | | partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value | diff --git a/javascript/ql/test/library-tests/TaintTracking/constructor-calls.js b/javascript/ql/test/library-tests/TaintTracking/constructor-calls.js new file mode 100644 index 000000000000..c59915527874 --- /dev/null +++ b/javascript/ql/test/library-tests/TaintTracking/constructor-calls.js @@ -0,0 +1,31 @@ +class EcmaClass { + constructor(param) { + this.param = param; + this.taint = source(); + } +} + +function JsClass(param) { + this.param = param; + this.taint = source(); +} + +function test() { + let taint = source(); + + let c = new EcmaClass(taint); + sink(c.param); // NOT OK + sink(c.taint); // NOT OK + + let c_safe = new EcmaClass("safe"); + sink(c_safe.param); // OK + sink(c_safe.taint); // NOT OK + + let d = new JsClass(taint); + sink(d.param); // NOT OK + sink(d.taint); // NOT OK + + let d_safe = new JsClass("safe"); + sink(d_safe.param); // OK + sink(d_safe.taint); // NOT OK +} From 9aaea40719faf167c1631514eddfa114a795d0b5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 16 Jan 2019 10:59:43 +0000 Subject: [PATCH 2/3] JS: address comments and support TrackedNode --- .../javascript/dataflow/Configuration.qll | 17 +---------------- .../semmle/javascript/dataflow/TrackedNodes.qll | 12 ++++++++---- .../javascript/dataflow/internal/FlowSteps.qll | 17 ++++++++++++++++- .../InterProceduralFlow/constructor-calls.js | 12 ++++++++++++ 4 files changed, 37 insertions(+), 21 deletions(-) create mode 100644 javascript/ql/test/library-tests/InterProceduralFlow/constructor-calls.js diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 81df66ddf1c1..61d9f6bc4d97 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -607,7 +607,7 @@ private predicate storeStep( summary = PathSummary::level() or exists(Function f, DataFlow::Node mid | - // `f` stores its parameter `pred` in property `prop` of a value that it returns, + // `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller, // and `succ` is an invocation of `f` reachableFromInput(f, succ, pred, mid, cfg, summary) and ( @@ -619,21 +619,6 @@ private predicate storeStep( ) } -/** - * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`. - */ -predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) { - base.hasPropertyWrite(prop, rhs) and - base.flowsToExpr(f.getAReturnedExpr()) -} - -/** - * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`. - */ -predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) { - DataFlow::thisNode(f).hasPropertyWrite(prop, rhs) -} - /** * Holds if `rhs` is the right-hand side of a write to property `prop`, and `nd` is reachable * from the base of that write under configuration `cfg` (possibly through callees) along a diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll index 563638336736..2a936efa5e3c 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll @@ -167,12 +167,16 @@ private module NodeTracking { basicStoreStep(pred, succ, prop) and summary = PathSummary::level() or - exists(Function f, DataFlow::Node mid, DataFlow::SourceNode base | - // `f` stores its parameter `pred` in property `prop` of a value that it returns, + exists(Function f, DataFlow::Node mid | + // `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller, // and `succ` is an invocation of `f` reachableFromInput(f, succ, pred, mid, summary) and - base.hasPropertyWrite(prop, mid) and - base.flowsToExpr(f.getAReturnedExpr()) + ( + returnedPropWrite(f, _, prop, mid) + or + succ instanceof DataFlow::NewNode and + receiverPropWrite(f, prop, mid) + ) ) } diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll index e6d092d3ebc1..647406b20e9c 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -115,7 +115,7 @@ predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { /** * Holds if there is a flow step from `pred` to `succ` through returning - * from a function call. + * from a function call or the receiver flowing out of a constructor call. */ predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) { exists(Function f | calls(succ, f) | @@ -266,6 +266,21 @@ predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) { ) } +/** + * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`. + */ +predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) { + base.hasPropertyWrite(prop, rhs) and + base.flowsToExpr(f.getAReturnedExpr()) +} + +/** + * Holds if `f` may assign `rhs` to `this.prop`. + */ +predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) { + DataFlow::thisNode(f).hasPropertyWrite(prop, rhs) +} + /** * A utility class that is equivalent to `boolean` but does not require type joining. */ diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/constructor-calls.js b/javascript/ql/test/library-tests/InterProceduralFlow/constructor-calls.js new file mode 100644 index 000000000000..97af5b480856 --- /dev/null +++ b/javascript/ql/test/library-tests/InterProceduralFlow/constructor-calls.js @@ -0,0 +1,12 @@ +import * as dummy from 'dummy'; + +class C { + constructor() { + this.field = new Object(); + } +} + +function test() { + let x = new C().field; + foo(x); +} From a1c7f32fb6b77449022db62fdb15ccc4b9706add Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 16 Jan 2019 11:08:47 +0000 Subject: [PATCH 3/3] JS: change note --- change-notes/1.20/analysis-javascript.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index 5b58bc304946..30874ce9c517 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -8,7 +8,7 @@ - server-side code, for example [hapi](https://hapijs.com/) * File classification has been improved to recognize additional generated files, for example files from [HTML Tidy](html-tidy.org). -* The taint tracking library now recognizes flow through persistent storage and callbacks in certain cases. This may give more results for the security queries. +* The taint tracking library now recognizes flow through persistent storage, class fields, and callbacks in certain cases. This may give more results for the security queries. ## New queries @@ -41,3 +41,4 @@ * `DataFlow::SourceNode` is no longer an abstract class; to add new source nodes, extend `DataFlow::SourceNode::Range` instead. * Subclasses of `DataFlow::PropRead` are no longer automatically made source nodes; you now need to additionally define a corresponding subclass of `DataFlow::SourceNode::Range` to achieve this. * The deprecated libraries `semmle.javascript.DataFlow` and `semmle.javascript.dataflow.CallGraph` have been removed; they are both superseded by `semmle.javascript.dataflow.DataFlow`. +* The predicate `DataFlow::returnedPropWrite` was intended for internal use only and is no longer available.