Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript: Add flow tracking through nested properties. #90

Closed
wants to merge 3 commits into from
Closed

JavaScript: Add flow tracking through nested properties. #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3812d8
JavaScript: Add flow tracking through nested properties.
Aug 22, 2018
d308888
JavaScript: Factor out some duplicated logic.
Aug 22, 2018
8fe07f1
JavaScript: Suppress unhelpful magic.
Aug 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion change-notes/1.18/analysis-javascript.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
104 changes: 97 additions & 7 deletions javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, _)
}

/**
Expand Down Expand Up @@ -429,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)
)
}

Expand Down Expand Up @@ -477,14 +491,90 @@ 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
extendingFlowStep(mid, cfg, oldSummary, nd, summary)
)
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;
* ```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... such that the path from rhs to succ is summarized by summary.

This part was a bit confusing (in the otherwise light and enjoyable prose).

If I understand it correctly now, this predicate "defines" that there is an edge from rhs to succ, is that right? The "such that" seems to imply a condition that must be satisfied for the predicate to hold, but it's really the other way around; the edge is there because this predicate says so.

I think it would clarify things to talk separately about the edge defined by the predicate, and those it uses to generate it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right; this needs to be documented better.

*/
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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was quite confusing for the same reason as the above, because in the example nothing appears to be reachable from load.

*
* 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 |
reachableFromLoadBase(prop, read, mid, cfg, oldSummary) and
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)
)
}

/**
* Holds if the value of `pred` is written to a property of some base object, and that base
* object may flow into the base of property read `succ` under configuration `cfg` along
Expand Down
86 changes: 82 additions & 4 deletions javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, _)
}
Expand Down Expand Up @@ -104,6 +105,8 @@ private module NodeTracking {
basicStoreStep(mid, nd, _)
or
loadStep(mid, nd, _)
or
reverseLoadStep(mid, nd, _)
)
}

Expand Down Expand Up @@ -142,8 +145,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)
)
}

Expand Down Expand Up @@ -184,13 +199,76 @@ 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
extendingFlowStep(mid, oldSummary, nd, summary)
)
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 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`.
*
* 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
extendingFlowStep(mid, oldSummary, nd, newSummary)
)
}

/**
* Holds if the value of `pred` is written to a property of some base object, and that base
* object may flow into the base of property read `succ` along a path summarized by `summary`.
Expand Down
18 changes: 18 additions & 0 deletions javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
1 change: 1 addition & 0 deletions javascript/ql/src/semmle/javascript/frameworks/Express.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -326,6 +327,7 @@ module HTTP {

StandardRequestExpr() { src.flowsTo(DataFlow::valueNode(this)) }

pragma[nomagic]
override RouteHandler getRouteHandler() {
result = src.getRouteHandler()
}
Expand Down Expand Up @@ -369,6 +371,7 @@ module HTTP {
/**
* Gets a route handler that is defined by this setup.
*/
pragma[nomagic]
abstract DataFlow::SourceNode getARouteHandler();

/**
Expand Down
2 changes: 2 additions & 0 deletions javascript/ql/src/semmle/javascript/frameworks/Koa.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
1 change: 1 addition & 0 deletions javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Expand Down
1 change: 1 addition & 0 deletions javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Expand Down
1 change: 1 addition & 0 deletions javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Expand Down
9 changes: 9 additions & 0 deletions javascript/ql/test/library-tests/InterProceduralFlow/properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});