Skip to content

Commit 2bbc1c2

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3478 from erik-krogh/PromiseAll
Approved by asgerf, esbena
2 parents 29b8a0d + aa396a3 commit 2bbc1c2

File tree

8 files changed

+119
-11
lines changed

8 files changed

+119
-11
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
## General improvements
44

55
* Support for the following frameworks and libraries has been improved:
6+
- [bluebird](http://bluebirdjs.com/)
67
- [express](https://www.npmjs.com/package/express)
78
- [fstream](https://www.npmjs.com/package/fstream)
89
- [jGrowl](https://github.com/stanlemon/jGrowl)
@@ -12,6 +13,7 @@
1213
- [mssql](https://www.npmjs.com/package/mssql)
1314
- [mysql](https://www.npmjs.com/package/mysql)
1415
- [pg](https://www.npmjs.com/package/pg)
16+
- [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
1517
- [sequelize](https://www.npmjs.com/package/sequelize)
1618
- [spanner](https://www.npmjs.com/package/spanner)
1719
- [sqlite](https://www.npmjs.com/package/sqlite)

‎javascript/ql/src/semmle/javascript/Arrays.qll

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -251,13 +251,15 @@ private module ArrayDataFlow {
251251
/**
252252
* A step for creating an array and storing the elements in the array.
253253
*/
254-
private class ArrayCreationStep extends DataFlow::AdditionalFlowStep, DataFlow::Node {
255-
ArrayCreationStep() { this instanceof DataFlow::ArrayCreationNode }
256-
254+
private class ArrayCreationStep extends DataFlow::AdditionalFlowStep, DataFlow::ArrayCreationNode {
257255
override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
258-
prop = arrayElement() and
259-
element = this.(DataFlow::ArrayCreationNode).getAnElement() and
260-
obj = this
256+
exists(int i |
257+
element = this.getElement(i) and
258+
obj = this and
259+
if this = any(PromiseAllCreation c).getArrayNode()
260+
then prop = arrayElement(i)
261+
else prop = arrayElement()
262+
)
261263
}
262264
}
263265

‎javascript/ql/src/semmle/javascript/Promises.qll

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ private class ES2015PromiseDefinition extends PromiseDefinition, DataFlow::NewNo
8585
*/
8686
abstract class PromiseCreationCall extends DataFlow::CallNode {
8787
/**
88-
* Gets the value this promise is resolved with.
88+
* Gets a value this promise is resolved with.
8989
*/
9090
abstract DataFlow::Node getValue();
9191
}
@@ -95,6 +95,16 @@ abstract class PromiseCreationCall extends DataFlow::CallNode {
9595
*/
9696
abstract class ResolvedPromiseDefinition extends PromiseCreationCall { }
9797

98+
/**
99+
* A promise that is created using a `Promise.all(array)` call.
100+
*/
101+
abstract class PromiseAllCreation extends PromiseCreationCall {
102+
/**
103+
* Gets a node for the array of values given to the `Promise.all(array)` call.
104+
*/
105+
abstract DataFlow::Node getArrayNode();
106+
}
107+
98108
/**
99109
* A resolved promise created by the standard ECMAScript 2015 `Promise.resolve` function.
100110
*/
@@ -121,6 +131,15 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall {
121131
}
122132
}
123133

134+
/**
135+
* An aggregated promise created using `Promise.all()`.
136+
*/
137+
class ES2015PromiseAllDefinition extends AggregateES2015PromiseDefinition, PromiseAllCreation {
138+
ES2015PromiseAllDefinition() { this.getCalleeName() = "all" }
139+
140+
override DataFlow::Node getArrayNode() { result = getArgument(0) }
141+
}
142+
124143
/**
125144
* Common predicates shared between type-tracking and data-flow for promises.
126145
*/
@@ -303,16 +322,27 @@ private module PromiseFlow {
303322
CreationStep() { this = promise }
304323

305324
override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
325+
not promise instanceof PromiseAllCreation and
306326
prop = valueProp() and
307327
pred = promise.getValue() and
308328
succ = this
329+
or
330+
prop = valueProp() and
331+
pred = promise.(PromiseAllCreation).getArrayNode() and
332+
succ = this
309333
}
310334

311335
override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
312336
// Copy the value of a resolved promise to the value of this promise.
337+
not promise instanceof PromiseAllCreation and
313338
prop = valueProp() and
314339
pred = promise.getValue() and
315340
succ = this
341+
or
342+
promise instanceof PromiseAllCreation and
343+
prop = valueProp() and
344+
pred = promise.(PromiseAllCreation).getArrayNode() and
345+
succ = this
316346
}
317347
}
318348

@@ -446,7 +476,11 @@ predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
446476
pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
447477
or
448478
// from `x` to `Promise.resolve(x)`
449-
pred = succ.(PromiseCreationCall).getValue()
479+
pred = succ.(PromiseCreationCall).getValue() and
480+
not succ instanceof PromiseAllCreation
481+
or
482+
// from `arr` to `Promise.all(arr)`
483+
pred = succ.(PromiseAllCreation).getArrayNode()
450484
or
451485
exists(DataFlow::MethodCallNode thn | thn.getMethodName() = "then" |
452486
// from `p` to `x` in `p.then(x => ...)`
@@ -533,6 +567,15 @@ module Bluebird {
533567
result = getArgument(0).getALocalSource().(DataFlow::ArrayCreationNode).getAnElement()
534568
}
535569
}
570+
571+
/**
572+
* A promise created using `Promise.all`:
573+
*/
574+
class BluebirdPromiseAllDefinition extends AggregateBluebirdPromiseDefinition, PromiseAllCreation {
575+
BluebirdPromiseAllDefinition() { this.getCalleeName() = "all" }
576+
577+
override DataFlow::Node getArrayNode() { result = getArgument(0) }
578+
}
536579
}
537580

538581
/**

‎javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,16 @@ module PseudoProperties {
607607
*/
608608
string arrayElement() { result = pseudoProperty("arrayElement") }
609609

610+
/**
611+
* Gets a pseudo-property for the location of the `i`th element in an `Array`.
612+
*/
613+
bindingset[i]
614+
string arrayElement(int i) {
615+
i < 5 and result = i.toString()
616+
or
617+
result = arrayElement()
618+
}
619+
610620
/**
611621
* Gets a pseudo-property for the location of elements in some array-like object. (Set, Array, or Iterator).
612622
*/

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,9 +241,6 @@ module TaintTracking {
241241
*/
242242
private predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
243243
exists(Expr e, Expr f | e = succ.asExpr() and f = pred.asExpr() |
244-
// arrays with tainted elements and objects with tainted property names are tainted
245-
e.(ArrayExpr).getAnElement() = f
246-
or
247244
exists(Property prop | e.(ObjectExpr).getAProperty() = prop |
248245
prop.isComputed() and f = prop.getNameExpr()
249246
)
@@ -258,6 +255,10 @@ module TaintTracking {
258255
e.(ArrayExpr).getAnElement().(SpreadElement).getOperand() = f
259256
)
260257
or
258+
// arrays with tainted elements and objects with tainted property names are tainted
259+
succ.(DataFlow::ArrayCreationNode).getAnElement() = pred and
260+
not any(PromiseAllCreation call).getArrayNode() = succ
261+
or
261262
// reading from a tainted object yields a tainted result
262263
succ.(DataFlow::PropRead).getBase() = pred
263264
or

‎javascript/ql/test/library-tests/Promises/AdditionalPromises.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
| additional-promises.js:2:13:2:57 | new Pin ... ct) {}) |
2+
| flow2.js:4:2:4:31 | Promise ... lean"]) |
23
| flow.js:7:11:7:59 | new Pro ... ource)) |
34
| flow.js:10:11:10:58 | new Pro ... ource)) |
45
| flow.js:13:11:13:58 | new Pro ... ource)) |

‎javascript/ql/test/library-tests/Promises/flow2.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
(async function () {
2+
var source = "source";
3+
4+
Promise.all([source, "clean"]).then((arr) => {
5+
sink(arr); // OK
6+
sink(arr[0]); // NOT OK
7+
sink(arr[1]); // OK
8+
})
9+
10+
var [clean, tainted] = await Promise.all(["clean", source]);
11+
sink(clean); // OK
12+
sink(tainted); // NOT OK
13+
14+
var [clean2, tainted2] = await Promise.resolve(Promise.all(["clean", source]));
15+
sink(clean2); // OK
16+
sink(tainted2); // NOT OK
17+
18+
var [clean3, tainted3] = await Promise.all(["clean", Promise.resolve(source)]);
19+
sink(clean3); // OK
20+
sink(tainted3); // NOT OK - but only flagged by taint-tracking
21+
});

‎javascript/ql/test/library-tests/Promises/tests.expected

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,14 @@
11
test_ResolvedPromiseDefinition
2+
| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:15:4:20 | source |
3+
| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:23:4:29 | "clean" |
4+
| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:44:10:50 | "clean" |
5+
| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:53:10:58 | source |
6+
| flow2.js:14:33:14:79 | Promise ... urce])) | flow2.js:14:49:14:78 | Promise ... ource]) |
7+
| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:62:14:68 | "clean" |
8+
| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:71:14:76 | source |
9+
| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:46:18:52 | "clean" |
10+
| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:55:18:77 | Promise ... source) |
11+
| flow2.js:18:55:18:77 | Promise ... source) | flow2.js:18:71:18:76 | source |
212
| flow.js:4:11:4:33 | Promise ... source) | flow.js:4:27:4:32 | source |
313
| flow.js:20:2:20:24 | Promise ... source) | flow.js:20:18:20:23 | source |
414
| flow.js:22:2:22:24 | Promise ... source) | flow.js:22:18:22:23 | source |
@@ -188,6 +198,9 @@ test_PromiseDefinition_getACatchHandler
188198
| flow.js:119:2:119:48 | new Pro ... "BLA")) | flow.js:119:56:119:68 | x => resolved |
189199
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:23:18:25:3 | (v) => ... v;\\n } |
190200
flow
201+
| flow2.js:2:15:2:22 | "source" | flow2.js:6:8:6:13 | arr[0] |
202+
| flow2.js:2:15:2:22 | "source" | flow2.js:12:7:12:13 | tainted |
203+
| flow2.js:2:15:2:22 | "source" | flow2.js:16:7:16:14 | tainted2 |
191204
| flow.js:2:15:2:22 | "source" | flow.js:5:7:5:14 | await p1 |
192205
| flow.js:2:15:2:22 | "source" | flow.js:8:7:8:14 | await p2 |
193206
| flow.js:2:15:2:22 | "source" | flow.js:17:8:17:8 | e |
@@ -220,8 +233,23 @@ flow
220233
| flow.js:2:15:2:22 | "source" | flow.js:129:69:129:69 | x |
221234
| flow.js:2:15:2:22 | "source" | flow.js:131:43:131:43 | x |
222235
exclusiveTaintFlow
236+
| flow2.js:2:15:2:22 | "source" | flow2.js:20:7:20:14 | tainted3 |
223237
| interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |
224238
typetrack
239+
| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:14:4:30 | [source, "clean"] | copy $PromiseResolveField$ |
240+
| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:14:4:30 | [source, "clean"] | store $PromiseResolveField$ |
241+
| flow2.js:4:39:4:41 | arr | flow2.js:4:2:4:31 | Promise ... lean"]) | load $PromiseResolveField$ |
242+
| flow2.js:10:25:10:60 | await P ... ource]) | flow2.js:10:31:10:60 | Promise ... ource]) | load $PromiseResolveField$ |
243+
| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:43:10:59 | ["clean", source] | copy $PromiseResolveField$ |
244+
| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:43:10:59 | ["clean", source] | store $PromiseResolveField$ |
245+
| flow2.js:14:27:14:79 | await P ... urce])) | flow2.js:14:33:14:79 | Promise ... urce])) | load $PromiseResolveField$ |
246+
| flow2.js:14:33:14:79 | Promise ... urce])) | flow2.js:14:49:14:78 | Promise ... ource]) | copy $PromiseResolveField$ |
247+
| flow2.js:14:33:14:79 | Promise ... urce])) | flow2.js:14:49:14:78 | Promise ... ource]) | store $PromiseResolveField$ |
248+
| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:61:14:77 | ["clean", source] | copy $PromiseResolveField$ |
249+
| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:61:14:77 | ["clean", source] | store $PromiseResolveField$ |
250+
| flow2.js:18:27:18:79 | await P ... urce)]) | flow2.js:18:33:18:79 | Promise ... urce)]) | load $PromiseResolveField$ |
251+
| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:45:18:78 | ["clean ... ource)] | copy $PromiseResolveField$ |
252+
| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:45:18:78 | ["clean ... ource)] | store $PromiseResolveField$ |
225253
| flow.js:20:2:20:43 | Promise ... ink(x)) | flow.js:20:36:20:42 | sink(x) | copy $PromiseResolveField$ |
226254
| flow.js:20:2:20:43 | Promise ... ink(x)) | flow.js:20:36:20:42 | sink(x) | store $PromiseResolveField$ |
227255
| flow.js:20:31:20:31 | x | flow.js:20:2:20:24 | Promise ... source) | load $PromiseResolveField$ |

0 commit comments

Comments
 (0)