From c3812d8501f42ec42de7fb9e4667a74970970dcf Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 22 Aug 2018 14:47:02 +0100 Subject: [PATCH 1/3] JavaScript: Add flow tracking through nested properties. --- change-notes/1.18/analysis-javascript.md | 2 +- .../javascript/dataflow/Configuration.qll | 69 ++++++++++++++++++- .../javascript/dataflow/TrackedNodes.qll | 68 ++++++++++++++++++ .../dataflow/internal/FlowSteps.qll | 18 +++++ .../InterProceduralFlow/DataFlow.expected | 1 + .../InterProceduralFlow/GermanFlow.expected | 1 + .../TaintTracking.expected | 1 + .../InterProceduralFlow/properties.js | 9 +++ 8 files changed, 167 insertions(+), 2 deletions(-) diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index 9d8876be1475..1cc9cdf5e3a4 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/change-notes/1.18/analysis-javascript.md @@ -12,7 +12,7 @@ * Modelling of taint flow through the array operations `map` and `join` has been improved. This may give additional results for the security queries. -* The taint tracking library recognizes more ways in which taint propagates. In particular, some flow through string formatters is now recognized. This may give additional results for the security queries. +* The taint tracking library recognizes more ways in which taint propagates. In particular, some flow through nested properties and through string formatters is now recognized. This may give additional results for the security queries. * The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries. diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 09dff7b01204..1bf5e2a5fc10 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -344,7 +344,8 @@ private predicate exploratoryFlowStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg) { basicFlowStep(pred, succ, _, cfg) or basicStoreStep(pred, succ, _) or - loadStep(pred, succ, _) + loadStep(pred, succ, _) or + reverseLoadStep(pred, succ, _) } /** @@ -483,6 +484,72 @@ private predicate reachableFromStoreBase(string prop, DataFlow::Node rhs, DataFl newSummary.valuePreserving() = true and summary = oldSummary.append(newSummary) ) + or + nestedPropFlow(prop, rhs, nd, cfg, summary) +} + +/** + * Holds if `rhs` is the right-hand side of a write to property `outerProp` and some read of + * another property `innerProp` is reachable from the base of that write under configuration `cfg`, + * and from the base of that inner read we can reach a read `succ` of the same property `innerProp`, + * such that the path from `rhs` to `succ` is summarized by `summary`. + * + * Example: + * + * ``` + * let root = new A(); + * let base = root.innerProp; + * base.outerProp = rhs; + * let succ = root.innerProp; + * ``` + */ +private predicate nestedPropFlow(string outerProp, DataFlow::Node rhs, DataFlow::Node succ, + DataFlow::Configuration cfg, PathSummary summary) { + exists (DataFlow::PropRead nestedRead, PathSummary oldSummary | + reachableFromStoreBase(outerProp, rhs, nestedRead, cfg, oldSummary) and + loadLoadPair(nestedRead, succ, cfg, oldSummary, summary) + ) +} + +/** + * Holds if `load` is a read of some property `innerProp` from which we can reach a read `succ` of the same + * property `innerProp` under configuration `cfg`, and the concatenation of `oldSummary` with the summary + * of that path is `summary`. + * + * Example: + * + * ``` + * let root = A(); + * let load = root.innerProp; + * let succ = root.innerProp; + * ``` + */ +pragma[noinline] +private predicate loadLoadPair(DataFlow::PropRead load, DataFlow::Node succ, + DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary summary) { + exists (string innerProp, DataFlow::Node nd, PathSummary newSummary | + reachableFromLoadBase(innerProp, load, nd, cfg, newSummary) and + loadStep(nd, succ, innerProp) and + summary = oldSummary.append(newSummary) + ) +} + +/** + * Holds if `read` is a read of property `prop`, and `nd` is reachable from the base of that read + * under configuration `cfg` (possibly through callees) along a path summarized by `summary`. + */ +private predicate reachableFromLoadBase(string prop, DataFlow::Node read, DataFlow::Node nd, + DataFlow::Configuration cfg, PathSummary summary) { + reachableFromStoreBase(_, _, read, cfg, _) and + reverseLoadStep(read, nd, prop) and + summary = PathSummary::empty() + or + exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | + reachableFromLoadBase(prop, read, mid, cfg, oldSummary) and + flowStep(mid, cfg, nd, newSummary) and + newSummary.valuePreserving() = true and + summary = oldSummary.append(newSummary) + ) } /** diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll index 92a6b8c463ac..73fafdfe448f 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll @@ -104,6 +104,8 @@ private module NodeTracking { basicStoreStep(mid, nd, _) or loadStep(mid, nd, _) + or + reverseLoadStep(mid, nd, _) ) } @@ -189,6 +191,72 @@ private module NodeTracking { flowStep(mid, nd, newSummary) and summary = oldSummary.append(newSummary) ) + or + nestedPropFlow(prop, rhs, nd, summary) + } + + /** + * Holds if `rhs` is the right-hand side of a write to property `outerProp` and some read of + * another property `innerProp` is reachable from the base of that, + * and from the base of that inner read we can reach a read `succ` of the same property `innerProp`, + * such that the path from `rhs` to `succ` is summarized by `summary`. + * + * Example: + * + * ``` + * let root = new A(); + * let base = root.innerProp; + * base.outerProp = rhs; + * let succ = root.innerProp; + * ``` + */ + private predicate nestedPropFlow(string outerProp, DataFlow::Node rhs, DataFlow::Node succ, + PathSummary summary) { + exists (DataFlow::PropRead nestedRead, PathSummary oldSummary | + reachableFromStoreBase(outerProp, rhs, nestedRead, oldSummary) and + loadLoadPair(nestedRead, succ, oldSummary, summary) + ) + } + + /** + * Holds if `load` is a read of some property `innerProp` from which we can reach a read `succ` of the same + * property `innerProp`, and the concatenation of `oldSummary` with the summary + * of that path is `summary`. + * + * Example: + * + * ``` + * let root = A(); + * let load = root.innerProp; + * let succ = root.innerProp; + * ``` + */ + pragma[noinline] + private predicate loadLoadPair(DataFlow::PropRead load, DataFlow::Node succ, + PathSummary oldSummary, PathSummary summary) { + exists (string innerProp, DataFlow::Node nd, PathSummary newSummary | + reachableFromLoadBase(innerProp, load, nd, newSummary) and + loadStep(nd, succ, innerProp) and + summary = oldSummary.append(newSummary) + ) + } + + /** + * Holds if `read` is a read of property `prop`, and `nd` is reachable from the base of that read + * (possibly through callees) along a path summarized by `summary`. + */ + private predicate reachableFromLoadBase(string prop, DataFlow::Node read, DataFlow::Node nd, + PathSummary summary) { + reachableFromStoreBase(_, _, read, _) and + reverseLoadStep(read, nd, prop) and + summary = PathSummary::empty() + or + exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | + reachableFromLoadBase(prop, read, mid, oldSummary) and + flowStep(mid, nd, newSummary) and + newSummary.valuePreserving() = true and + summary = oldSummary.append(newSummary) + ) } /** diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll index 1251766cef66..8a39bd9fa2ad 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -208,6 +208,24 @@ predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) { succ.accesses(pred, prop) } +/** + * Holds if there is a "reverse load" step from `pred` to `succ` under property `prop`, + * that is, `pred` is a read of property `prop` from (a data flow node reachable from) + * `succ`. + * + * For example, for this code snippet: + * + * ``` + * var a = new A(); + * a.p; + * ``` + * + * there is a reverse load step from `a.p` to `new A()` under property `prop`. + */ +predicate reverseLoadStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { + pred = succ.getAPropertyRead(prop) +} + /** * A utility class that is equivalent to `boolean` but does not require type joining. */ diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected index 0bd663e2038a..03f50751dd19 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected @@ -27,6 +27,7 @@ | properties.js:2:16:2:24 | "tainted" | properties.js:12:15:12:24 | x.someProp | | properties.js:2:16:2:24 | "tainted" | properties.js:14:15:14:27 | tmp1.someProp | | properties.js:18:26:18:42 | "tainted as well" | properties.js:20:24:20:33 | window.foo | +| properties.js:23:22:23:35 | "also tainted" | properties.js:28:15:28:19 | b.p.q | | tst2.js:2:17:2:26 | "tainted1" | tst2.js:10:15:10:24 | g(source1) | | tst2.js:3:17:3:26 | "tainted2" | tst2.js:11:15:11:24 | g(source2) | | tst2.js:6:24:6:37 | "also tainted" | tst2.js:10:15:10:24 | g(source1) | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected index 21ec35e6dd1b..4d66e109865c 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected @@ -28,6 +28,7 @@ | properties.js:2:16:2:24 | "tainted" | properties.js:12:15:12:24 | x.someProp | | properties.js:2:16:2:24 | "tainted" | properties.js:14:15:14:27 | tmp1.someProp | | properties.js:18:26:18:42 | "tainted as well" | properties.js:20:24:20:33 | window.foo | +| properties.js:23:22:23:35 | "also tainted" | properties.js:28:15:28:19 | b.p.q | | tst2.js:2:17:2:26 | "tainted1" | tst2.js:10:15:10:24 | g(source1) | | tst2.js:3:17:3:26 | "tainted2" | tst2.js:11:15:11:24 | g(source2) | | tst2.js:6:24:6:37 | "also tainted" | tst2.js:10:15:10:24 | g(source1) | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected index c826108c46b3..4659cb54e449 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected @@ -32,6 +32,7 @@ | properties.js:2:16:2:24 | "tainted" | properties.js:12:15:12:24 | x.someProp | | properties.js:2:16:2:24 | "tainted" | properties.js:14:15:14:27 | tmp1.someProp | | properties.js:18:26:18:42 | "tainted as well" | properties.js:20:24:20:33 | window.foo | +| properties.js:23:22:23:35 | "also tainted" | properties.js:28:15:28:19 | b.p.q | | tst2.js:2:17:2:26 | "tainted1" | tst2.js:10:15:10:24 | g(source1) | | tst2.js:3:17:3:26 | "tainted2" | tst2.js:11:15:11:24 | g(source2) | | tst2.js:6:24:6:37 | "also tainted" | tst2.js:10:15:10:24 | g(source1) | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/properties.js b/javascript/ql/test/library-tests/InterProceduralFlow/properties.js index bb6379366c8c..a573d68d776d 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/properties.js +++ b/javascript/ql/test/library-tests/InterProceduralFlow/properties.js @@ -18,3 +18,12 @@ function f(x) { var yet_another_source = "tainted as well"; window.foo = yet_another_source; var yet_another_sink = window.foo; + +(function() { + var other_source = "also tainted"; + var b = new B(); + b.p.q = other_source; + var sink3 = b; + var sink4 = b.p; + var sink5 = b.p.q; +}); From d30888895b29667974a13d42274f124c830059b8 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 22 Aug 2018 14:48:34 +0100 Subject: [PATCH 2/3] JavaScript: Factor out some duplicated logic. --- .../javascript/dataflow/Configuration.qll | 45 ++++++++++++++----- .../javascript/dataflow/TrackedNodes.qll | 27 +++++++---- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 1bf5e2a5fc10..115636b419d4 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -430,10 +430,23 @@ private predicate reachableFromInput(Function f, DataFlow::Node invk, callInputStep(f, invk, input, nd, cfg) and summary = PathSummary::empty() or - exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | + exists (DataFlow::Node mid, PathSummary oldSummary | reachableFromInput(f, invk, input, mid, cfg, oldSummary) and - flowStep(mid, cfg, nd, newSummary) and - summary = oldSummary.append(newSummary) + extendingTaintStep(mid, cfg, oldSummary, nd, summary) + ) +} + +/** + * Holds if there is a (not necessarily value-preserving) flow step from `pred` to `succ` under + * `cfg`, and the path summary of that step can be appended to `oldSummary` to yield `newSummary`. + */ +pragma[noinline] +private predicate extendingTaintStep(DataFlow::Node pred, DataFlow::Configuration cfg, + PathSummary oldSummary, DataFlow::Node succ, + PathSummary newSummary) { + exists (PathSummary stepSummary | + flowStep(pred, cfg, succ, stepSummary) and + newSummary = oldSummary.append(stepSummary) ) } @@ -478,11 +491,9 @@ private predicate reachableFromStoreBase(string prop, DataFlow::Node rhs, DataFl isRelevant(rhs, cfg) and storeStep(rhs, nd, prop, cfg, summary) or - exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | + exists (DataFlow::Node mid, PathSummary oldSummary | reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and - flowStep(mid, cfg, nd, newSummary) and - newSummary.valuePreserving() = true and - summary = oldSummary.append(newSummary) + extendingFlowStep(mid, cfg, oldSummary, nd, summary) ) or nestedPropFlow(prop, rhs, nd, cfg, summary) @@ -544,11 +555,23 @@ private predicate reachableFromLoadBase(string prop, DataFlow::Node read, DataFl reverseLoadStep(read, nd, prop) and summary = PathSummary::empty() or - exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | + exists (DataFlow::Node mid, PathSummary oldSummary | reachableFromLoadBase(prop, read, mid, cfg, oldSummary) and - flowStep(mid, cfg, nd, newSummary) and - newSummary.valuePreserving() = true and - summary = oldSummary.append(newSummary) + extendingFlowStep(mid, cfg, oldSummary, nd, summary) + ) +} + +/** + * Holds if there is a (value-preserving) flow step from `pred` to `succ` under `cfg`, and + * the path summary of that step can be appended to `oldSummary` to yield `newSummary`. + */ +pragma[noinline] +private predicate extendingFlowStep(DataFlow::Node pred, DataFlow::Configuration cfg, + PathSummary oldSummary, DataFlow::Node succ, PathSummary newSummary) { + exists (PathSummary stepSummary | + flowStep(pred, cfg, succ, stepSummary) and + stepSummary.valuePreserving() = true and + newSummary = oldSummary.append(stepSummary) ) } diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll index 73fafdfe448f..932460d9fa15 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll @@ -144,8 +144,20 @@ private module NodeTracking { or exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | reachableFromInput(f, invk, input, mid, oldSummary) and - flowStep(mid, nd, newSummary) and - summary = oldSummary.append(newSummary) + extendingFlowStep(mid, oldSummary, nd, newSummary) + ) + } + + /** + * Holds if there is a (value-preserving) flow step from `pred` to `succ` under `cfg`, and + * the path summary of that step can be appended to `oldSummary` to yield `newSummary`. + */ + pragma[noinline] + private predicate extendingFlowStep(DataFlow::Node pred, + PathSummary oldSummary, DataFlow::Node succ, PathSummary newSummary) { + exists (PathSummary stepSummary | + flowStep(pred, succ, stepSummary) and + newSummary = oldSummary.append(stepSummary) ) } @@ -186,10 +198,9 @@ private module NodeTracking { PathSummary summary) { storeStep(rhs, nd, prop, summary) or - exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | + exists (DataFlow::Node mid, PathSummary oldSummary | reachableFromStoreBase(prop, rhs, mid, oldSummary) and - flowStep(mid, nd, newSummary) and - summary = oldSummary.append(newSummary) + extendingFlowStep(mid, oldSummary, nd, summary) ) or nestedPropFlow(prop, rhs, nd, summary) @@ -197,7 +208,7 @@ private module NodeTracking { /** * Holds if `rhs` is the right-hand side of a write to property `outerProp` and some read of - * another property `innerProp` is reachable from the base of that, + * another property `innerProp` is reachable from the base of that write, * and from the base of that inner read we can reach a read `succ` of the same property `innerProp`, * such that the path from `rhs` to `succ` is summarized by `summary`. * @@ -253,9 +264,7 @@ private module NodeTracking { or exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | reachableFromLoadBase(prop, read, mid, oldSummary) and - flowStep(mid, nd, newSummary) and - newSummary.valuePreserving() = true and - summary = oldSummary.append(newSummary) + extendingFlowStep(mid, oldSummary, nd, newSummary) ) } From 8fe07f1c1096e9d4a165c0b0bfa43750f2b72103 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 17 Aug 2018 15:25:23 +0100 Subject: [PATCH 3/3] JavaScript: Suppress unhelpful magic. This eliminates a nest of `pos#` predicates resulting from ill-advised applications of magic. --- javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll | 1 + javascript/ql/src/semmle/javascript/frameworks/Express.qll | 1 + javascript/ql/src/semmle/javascript/frameworks/HTTP.qll | 3 +++ javascript/ql/src/semmle/javascript/frameworks/Koa.qll | 2 ++ 4 files changed, 7 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll index 932460d9fa15..927473c53ab9 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll @@ -16,6 +16,7 @@ abstract class TrackedNode extends DataFlow::Node { * Holds if this node flows into `sink` in zero or more (possibly * inter-procedural) steps. */ + pragma[nomagic] predicate flowsTo(DataFlow::Node sink) { NodeTracking::flowsTo(this, sink, _) } diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll index 9e9b52d050ee..ce10448f05fe 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll @@ -62,6 +62,7 @@ module Express { /** * Holds if `e` may refer to the given `router` object. */ + pragma[nomagic] private predicate isRouter(Expr e, RouterDefinition router) { router.flowsTo(e) or diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll index 43882d2fa27b..9900e18f3f32 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll @@ -218,6 +218,7 @@ module HTTP { * Gets an expression that contains a request object handled * by this handler. */ + pragma[nomagic] RequestExpr getARequestExpr() { result.getRouteHandler() = this } @@ -326,6 +327,7 @@ module HTTP { StandardRequestExpr() { src.flowsTo(DataFlow::valueNode(this)) } + pragma[nomagic] override RouteHandler getRouteHandler() { result = src.getRouteHandler() } @@ -369,6 +371,7 @@ module HTTP { /** * Gets a route handler that is defined by this setup. */ + pragma[nomagic] abstract DataFlow::SourceNode getARouteHandler(); /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll index e2d31586e6b3..5b2977fca916 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll @@ -130,11 +130,13 @@ module Koa { class ContextExpr extends Expr { ContextSource src; + pragma[nomagic] ContextExpr() { src.flowsTo(DataFlow::valueNode(this)) } /** * Gets the route handler that provides this response. */ + pragma[nomagic] RouteHandler getRouteHandler() { result = src.getRouteHandler() }