Skip to content

Commit 8c4dbca

Browse files
committed
Improve data flow in the async library
1 parent d83cbde commit 8c4dbca

File tree

5 files changed

+158
-46
lines changed

5 files changed

+158
-46
lines changed

javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,8 @@ private string encodeContentAux(ContentSet cs, string arg) {
9494
cs = ContentSet::iteratorElement() and result = "IteratorElement"
9595
or
9696
cs = ContentSet::iteratorError() and result = "IteratorError"
97+
or
98+
cs = ContentSet::anyProperty() and result = "AnyMember"
9799
)
98100
or
99101
cs = getPromiseContent(arg) and

javascript/ql/lib/semmle/javascript/frameworks/AsyncPackage.qll

Lines changed: 112 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,15 @@ module AsyncPackage {
1515
}
1616

1717
/**
18-
* Gets a reference to the given member or one of its `Limit` or `Series` variants.
18+
* Gets `Limit` or `Series` name variants for a given member name.
1919
*
20-
* For example, `memberVariant("map")` finds references to `map`, `mapLimit`, and `mapSeries`.
20+
* For example, `memberNameVariant("map")` returns `map`, `mapLimit`, and `mapSeries`.
2121
*/
22-
DataFlow::SourceNode memberVariant(string name) {
23-
result = member(name) or
24-
result = member(name + "Limit") or
25-
result = member(name + "Series")
22+
bindingset[name]
23+
string memberNameVariant(string name) {
24+
result = name or
25+
result = name + "Limit" or
26+
result = name + "Series"
2627
}
2728

2829
/**
@@ -101,22 +102,47 @@ module AsyncPackage {
101102
*/
102103
class IterationCall extends DataFlow::InvokeNode {
103104
string name;
105+
int iteratorCallbackIndex;
106+
int finalCallbackIndex;
104107

105108
IterationCall() {
106-
this = memberVariant(name).getACall() and
107-
name =
108-
[
109-
"concat", "detect", "each", "eachOf", "forEach", "forEachOf", "every", "filter",
110-
"groupBy", "map", "mapValues", "reduce", "reduceRight", "reject", "some", "sortBy",
111-
"transform"
112-
]
109+
(
110+
(
111+
name =
112+
memberNameVariant([
113+
"concat", "detect", "each", "eachOf", "forEach", "forEachOf", "every", "filter",
114+
"groupBy", "map", "mapValues", "reject", "some", "sortBy",
115+
]) and
116+
if name.matches("%Limit")
117+
then (
118+
iteratorCallbackIndex = 2 and finalCallbackIndex = 3
119+
) else (
120+
iteratorCallbackIndex = 1 and finalCallbackIndex = 2
121+
)
122+
)
123+
or
124+
name = ["reduce", "reduceRight", "transform"] and
125+
iteratorCallbackIndex = 2 and
126+
finalCallbackIndex = 3
127+
) and
128+
this = member(name).getACall()
113129
}
114130

115131
/**
116-
* Gets the name of the iteration call, without the `Limit` or `Series` suffix.
132+
* Gets the name of the iteration call
117133
*/
118134
string getName() { result = name }
119135

136+
/**
137+
* Gets the iterator callback index
138+
*/
139+
int getIteratorCallbackIndex() { result = iteratorCallbackIndex }
140+
141+
/**
142+
* Gets the final callback index
143+
*/
144+
int getFinalCallbackIndex() { result = finalCallbackIndex }
145+
120146
/**
121147
* Gets the node holding the collection being iterated over.
122148
*/
@@ -125,41 +151,88 @@ module AsyncPackage {
125151
/**
126152
* Gets the node holding the function being called for each element in the collection.
127153
*/
128-
DataFlow::Node getIteratorCallback() { result = this.getArgument(this.getNumArgument() - 2) }
154+
DataFlow::FunctionNode getIteratorCallback() {
155+
result = this.getCallback(iteratorCallbackIndex)
156+
}
129157

130158
/**
131-
* Gets the node holding the function being invoked after iteration is complete.
159+
* Gets the node holding the function being invoked after iteration is complete. (may not exist)
132160
*/
133-
DataFlow::Node getFinalCallback() { result = this.getArgument(this.getNumArgument() - 1) }
161+
DataFlow::FunctionNode getFinalCallback() { result = this.getCallback(finalCallbackIndex) }
162+
}
163+
164+
/**
165+
* An IterationCall with its iterator callback at index 1
166+
*/
167+
private class IterationCallCallbacksFirstArg extends IterationCall {
168+
IterationCallCallbacksFirstArg() { this.getIteratorCallbackIndex() = 1 }
169+
}
170+
171+
/**
172+
* An IterationCall with its iterator callback at index 2
173+
*/
174+
private class IterationCallCallbacksSecondArg extends IterationCall {
175+
IterationCallCallbacksSecondArg() { this.getIteratorCallbackIndex() = 2 }
176+
}
177+
178+
/**
179+
* The model with the iteratorCallbackIndex abstracted
180+
*/
181+
bindingset[iteratorCallbackIndex]
182+
private predicate iterationCallPropagatesFlow(
183+
string input, string output, boolean preservesValue, int iteratorCallbackIndex
184+
) {
185+
preservesValue = true and
186+
input = "Argument[0]." + ["ArrayElement", "SetElement", "IteratorElement", "AnyMember"] and
187+
output = "Argument[" + iteratorCallbackIndex + "].Parameter[0]"
134188
}
135189

136190
/**
137-
* A taint step from the collection into the iterator callback of an iteration call.
191+
* A taint step from the collection into the iterator callback (at index 1) of an iteration call.
138192
*
139193
* For example: `data -> item` in `async.each(data, (item, cb) => {})`.
140194
*/
141-
private class IterationInputTaintStep extends TaintTracking::SharedTaintStep {
142-
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
143-
exists(DataFlow::FunctionNode iteratee, IterationCall call |
144-
iteratee = call.getIteratorCallback() and // Require a closure to avoid spurious call/return mismatch.
145-
pred = call.getCollection() and // TODO: needs a flow summary to ensure ArrayElement content is unfolded
146-
succ = iteratee.getParameter(0)
147-
)
195+
class IterationCallCallbacksFirstArgFlowSummary extends DataFlow::SummarizedCallable {
196+
IterationCallCallbacksFirstArgFlowSummary() { this = "async.[IterationCallCallbacksFirstArg]" }
197+
198+
override DataFlow::InvokeNode getACallSimple() {
199+
result instanceof IterationCallCallbacksFirstArg
200+
}
201+
202+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
203+
iterationCallPropagatesFlow(input, output, preservesValue, 1)
204+
}
205+
}
206+
207+
/**
208+
* A taint step from the collection into the iterator callback (at index 2) of an iteration call.
209+
*
210+
* For example: `data -> item` in `async.eachLimit(data, 1, (item, cb) => {})`.
211+
*/
212+
class IterationCallCallbacksSecondArgFlowSummary extends DataFlow::SummarizedCallable {
213+
IterationCallCallbacksSecondArgFlowSummary() { this = "async.[IterationCallCallbackSecondArg]" }
214+
215+
override DataFlow::InvokeNode getACallSimple() {
216+
result instanceof IterationCallCallbacksSecondArg
217+
}
218+
219+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
220+
iterationCallPropagatesFlow(input, output, preservesValue, 2)
148221
}
149222
}
150223

151224
/**
152225
* A taint step from the return value of an iterator callback to the result of the iteration
153226
* call.
154227
*
155-
* For example: `item + taint()` -> result` in `async.map(data, (item, cb) => cb(null, item + taint()), (err, result) => {})`.
228+
* For example: `item + taint() -> result` in `async.map(data, (item, cb) => cb(null, item + taint()), (err, result) => {})`.
156229
*/
157230
private class IterationOutputTaintStep extends TaintTracking::SharedTaintStep {
158231
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
159232
exists(
160233
DataFlow::FunctionNode iteratee, DataFlow::FunctionNode final, int i, IterationCall call
161234
|
162-
iteratee = call.getIteratorCallback().getALocalSource() and
235+
iteratee = call.getIteratorCallback() and
163236
final = call.getFinalCallback() and // Require a closure to avoid spurious call/return mismatch.
164237
pred = getLastParameter(iteratee).getACall().getArgument(i) and
165238
succ = final.getParameter(i) and
@@ -175,14 +248,18 @@ module AsyncPackage {
175248
*
176249
* For example: `data -> result` in `async.sortBy(data, orderingFn, (err, result) => {})`.
177250
*/
178-
private class IterationPreserveTaintStep extends TaintTracking::SharedTaintStep {
179-
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
180-
exists(DataFlow::FunctionNode final, IterationCall call |
181-
final = call.getFinalCallback() and // Require a closure to avoid spurious call/return mismatch.
182-
pred = call.getCollection() and
183-
succ = final.getParameter(1) and
184-
call.getName() = "sortBy"
185-
)
251+
class IterationPreserveTaintStepFlowSummary extends DataFlow::SummarizedCallable {
252+
IterationPreserveTaintStepFlowSummary() { this = "async.sortBy" }
253+
254+
override DataFlow::InvokeNode getACallSimple() {
255+
result instanceof IterationCall and
256+
result.(IterationCall).getName() = "sortBy"
257+
}
258+
259+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
260+
preservesValue = false and
261+
input = "Argument[0]." + ["ArrayElement", "SetElement", "IteratorElement", "AnyMember"] and
262+
output = "Argument[2].Parameter[1]"
186263
}
187264
}
188265
}
Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,24 @@
11
legacyDataFlowDifference
2-
| each.js:11:9:11:16 | source() | each.js:13:12:13:15 | item | only flow with OLD data flow library |
3-
| map.js:10:13:10:20 | source() | map.js:12:14:12:17 | item | only flow with OLD data flow library |
4-
| map.js:26:13:26:20 | source() | map.js:28:27:28:32 | result | only flow with OLD data flow library |
5-
| sortBy.js:10:22:10:29 | source() | sortBy.js:12:27:12:32 | result | only flow with OLD data flow library |
2+
| each.js:11:9:11:16 | source() | each.js:13:12:13:15 | item | only flow with NEW data flow library |
3+
| map.js:14:13:14:20 | source() | map.js:16:14:16:17 | item | only flow with NEW data flow library |
4+
| map.js:30:13:30:20 | source() | map.js:32:27:32:32 | result | only flow with NEW data flow library |
5+
| map.js:40:13:40:20 | source() | map.js:11:10:11:10 | x | only flow with NEW data flow library |
6+
| map.js:42:12:42:19 | source() | map.js:11:10:11:10 | x | only flow with NEW data flow library |
7+
| map.js:44:16:44:23 | source() | map.js:11:10:11:10 | x | only flow with NEW data flow library |
8+
| map.js:46:18:46:25 | source() | map.js:11:10:11:10 | x | only flow with NEW data flow library |
9+
| sortBy.js:10:22:10:29 | source() | sortBy.js:12:27:12:32 | result | only flow with NEW data flow library |
610
#select
7-
| map.js:20:19:20:26 | source() | map.js:23:27:23:32 | result |
8-
| waterfall.js:8:30:8:37 | source() | waterfall.js:11:12:11:16 | taint |
9-
| waterfall.js:8:30:8:37 | source() | waterfall.js:20:10:20:14 | taint |
10-
| waterfall.js:28:18:28:25 | source() | waterfall.js:39:10:39:12 | err |
11-
| waterfall.js:46:22:46:29 | source() | waterfall.js:49:12:49:16 | taint |
12-
| waterfall.js:46:22:46:29 | source() | waterfall.js:55:10:55:14 | taint |
11+
| each.js:11:9:11:16 | source() | each.js:13:12:13:15 | item |
12+
| map.js:14:13:14:20 | source() | map.js:16:14:16:17 | item |
13+
| map.js:24:19:24:26 | source() | map.js:27:27:27:32 | result |
14+
| map.js:30:13:30:20 | source() | map.js:32:27:32:32 | result |
15+
| map.js:40:13:40:20 | source() | map.js:11:10:11:10 | x |
16+
| map.js:42:12:42:19 | source() | map.js:11:10:11:10 | x |
17+
| map.js:44:16:44:23 | source() | map.js:11:10:11:10 | x |
18+
| map.js:46:18:46:25 | source() | map.js:11:10:11:10 | x |
19+
| sortBy.js:10:22:10:29 | source() | sortBy.js:12:27:12:32 | result |
20+
| waterfall.js:16:30:16:37 | source() | waterfall.js:19:12:19:16 | taint |
21+
| waterfall.js:16:30:16:37 | source() | waterfall.js:28:10:28:14 | taint |
22+
| waterfall.js:36:18:36:25 | source() | waterfall.js:47:10:47:12 | err |
23+
| waterfall.js:54:22:54:29 | source() | waterfall.js:57:12:57:16 | taint |
24+
| waterfall.js:54:22:54:29 | source() | waterfall.js:63:10:63:14 | taint |

javascript/ql/test/library-tests/frameworks/AsyncPackage/map.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ function sink(x) {
77
console.log(x)
88
}
99

10+
function call_sink(x) {
11+
sink(x)
12+
}
13+
1014
async_.map([source()],
1115
(item, cb) => {
1216
sink(item), // NOT OK
@@ -32,3 +36,12 @@ async_.map(['safe'],
3236
(item, cb) => cb(null, item),
3337
(err, result) => sink(result) // OK
3438
);
39+
40+
async_.map([source()], call_sink) // NOT OK
41+
42+
async_.map(source().prop, call_sink) // NOT OK
43+
44+
async_.map({a: source()}, call_sink) // NOT OK
45+
46+
async_.mapLimit([source()], 1, call_sink) // NOT OK
47+

javascript/ql/test/library-tests/frameworks/AsyncPackage/waterfall.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,15 @@
11
let async_ = require('async');
22
let waterfall = require('a-sync-waterfall');
33

4-
var source, sink, somethingWrong;
4+
function source() {
5+
return 'TAINT'
6+
}
7+
8+
function sink(x) {
9+
console.log(x)
10+
}
11+
12+
var somethingWrong;
513

614
async_.waterfall([
715
function(callback) {

0 commit comments

Comments
 (0)