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 1 commit
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
69 changes: 68 additions & 1 deletion 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 @@ -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;
* ```
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)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we have two PathSummary arguments. oldSummary is almost not bound at all; it's just anything that can be prefixed onto newSummary to yield summary.

It seems like the summary of this edge is a pair of input/output summaries, instead of just being a summary. I mean that's what PathSummary is for, right?

Apart from that, is it correctly understood that we would get N^2 load-load pairs for code like this?

elm.style.color = X;
elm.style.backgroundColor = X;
elm.style.border = X;
elm.style.padding = X;
...

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's a bad case with the current implementation.

Adding oldSummary as an argument to this predicate is mostly done to enable a better join order in nestedPropFlow.

}

/**
* 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)
)
}

/**
Expand Down
68 changes: 68 additions & 0 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 @@ -104,6 +104,8 @@ private module NodeTracking {
basicStoreStep(mid, nd, _)
or
loadStep(mid, nd, _)
or
reverseLoadStep(mid, nd, _)
)
}

Expand Down Expand Up @@ -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)
)
}

/**
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/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;
});