Skip to content

Commit 765c40e

Browse files
codeql-cicodeql-ci
authored
Merge pull request #4019 from erik-krogh/asyncCalls
Approved by asgerf
2 parents b8d6f76 + 65a1769 commit 765c40e

30 files changed

+398
-37
lines changed

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,49 @@ private class PromiseTaintStep extends TaintTracking::AdditionalTaintStep {
455455
}
456456
}
457457

458+
/**
459+
* Defines flow steps for return on async functions.
460+
*/
461+
private module AsyncReturnSteps {
462+
private predicate valueProp = Promises::valueProp/0;
463+
464+
private predicate errorProp = Promises::errorProp/0;
465+
466+
private import semmle.javascript.dataflow.internal.FlowSteps
467+
468+
/**
469+
* A data-flow step for ordinary and exceptional returns from async functions.
470+
*/
471+
private class AsyncReturn extends PreCallGraphStep {
472+
override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
473+
exists(DataFlow::FunctionNode f | f.getFunction().isAsync() |
474+
// ordinary return
475+
prop = valueProp() and
476+
pred = f.getAReturn() and
477+
succ = f.getReturnNode()
478+
or
479+
// exceptional return
480+
prop = errorProp() and
481+
localExceptionStepWithAsyncFlag(pred, succ, true)
482+
)
483+
}
484+
}
485+
486+
/**
487+
* A data-flow step for ordinary return from an async function in a taint configuration.
488+
*/
489+
private class AsyncTaintReturn extends TaintTracking::AdditionalTaintStep, DataFlow::FunctionNode {
490+
Function f;
491+
492+
AsyncTaintReturn() { this.getFunction() = f and f.isAsync() }
493+
494+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
495+
returnExpr(f, pred, _) and
496+
succ.(DataFlow::FunctionReturnNode).getFunction() = f
497+
}
498+
}
499+
}
500+
458501
/**
459502
* Provides classes for working with the `bluebird` library (http://bluebirdjs.com).
460503
*/

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

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -982,8 +982,8 @@ private predicate appendStep(
982982
private predicate flowThroughCall(
983983
DataFlow::Node input, DataFlow::Node output, DataFlow::Configuration cfg, PathSummary summary
984984
) {
985-
exists(Function f, DataFlow::ValueNode ret |
986-
ret.asExpr() = f.getAReturnedExpr() and
985+
exists(Function f, DataFlow::FunctionReturnNode ret |
986+
ret.getFunction() = f and
987987
(calls(output, f) or callsBound(output, f, _)) and // Do not consider partial calls
988988
reachableFromInput(f, output, input, ret, cfg, summary) and
989989
not isBarrierEdge(cfg, ret, output) and
@@ -1000,6 +1000,17 @@ private predicate flowThroughCall(
10001000
not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
10011001
not cfg.isLabeledBarrier(output, summary.getEndLabel())
10021002
)
1003+
or
1004+
// exception thrown inside an immediately awaited function call.
1005+
exists(DataFlow::FunctionNode f, DataFlow::Node invk, DataFlow::Node ret |
1006+
f.getFunction().isAsync()
1007+
|
1008+
(calls(invk, f.getFunction()) or callsBound(invk, f.getFunction(), _)) and
1009+
reachableFromInput(f.getFunction(), invk, input, ret, cfg, summary) and
1010+
output = invk.asExpr().getExceptionTarget() and
1011+
f.getExceptionalReturn() = getThrowTarget(ret) and
1012+
invk = getAwaitOperand(_)
1013+
)
10031014
}
10041015

10051016
/**
@@ -1019,23 +1030,39 @@ private predicate storeStep(
10191030
isAdditionalStoreStep(pred, succ, prop, cfg) and
10201031
summary = PathSummary::level()
10211032
or
1022-
exists(Function f, DataFlow::Node mid |
1033+
exists(Function f, DataFlow::Node mid, DataFlow::Node invk |
1034+
not f.isAsync() and invk = succ
1035+
or
1036+
// store in an immediately awaited function call
1037+
f.isAsync() and
1038+
invk = getAwaitOperand(succ)
1039+
|
10231040
// `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller,
10241041
// and `succ` is an invocation of `f`
1025-
reachableFromInput(f, succ, pred, mid, cfg, summary) and
1042+
reachableFromInput(f, invk, pred, mid, cfg, summary) and
10261043
(
10271044
returnedPropWrite(f, _, prop, mid)
10281045
or
10291046
exists(DataFlow::SourceNode base | base.flowsToExpr(f.getAReturnedExpr()) |
10301047
isAdditionalStoreStep(mid, base, prop, cfg)
10311048
)
10321049
or
1033-
succ instanceof DataFlow::NewNode and
1050+
invk instanceof DataFlow::NewNode and
10341051
receiverPropWrite(f, prop, mid)
10351052
)
10361053
)
10371054
}
10381055

1056+
/**
1057+
* Gets a dataflow-node for the operand of the await-expression `await`.
1058+
*/
1059+
private DataFlow::Node getAwaitOperand(DataFlow::Node await) {
1060+
exists(AwaitExpr awaitExpr |
1061+
result = awaitExpr.getOperand().getUnderlyingValue().flow() and
1062+
await.asExpr() = awaitExpr
1063+
)
1064+
}
1065+
10391066
/**
10401067
* Holds if `f` may `read` property `prop` of parameter `parm`.
10411068
*/
@@ -1128,8 +1155,14 @@ private predicate loadStep(
11281155
isAdditionalLoadStep(pred, succ, prop, cfg) and
11291156
summary = PathSummary::level()
11301157
or
1131-
exists(Function f, DataFlow::Node read |
1132-
parameterPropRead(f, succ, pred, prop, read, cfg) and
1158+
exists(Function f, DataFlow::Node read, DataFlow::Node invk |
1159+
not f.isAsync() and invk = succ
1160+
or
1161+
// load from an immediately awaited function call
1162+
f.isAsync() and
1163+
invk = getAwaitOperand(succ)
1164+
|
1165+
parameterPropRead(f, invk, pred, prop, read, cfg) and
11331166
reachesReturn(f, read, cfg, summary)
11341167
)
11351168
}
@@ -1624,6 +1657,9 @@ class MidPathNode extends PathNode, MkMidNode {
16241657
// Skip the exceptional return on functions, as this highlights the entire function.
16251658
nd = any(DataFlow::FunctionNode f).getExceptionalReturn()
16261659
or
1660+
// Skip the special return node for functions, as this highlights the entire function (and the returned expr is the previous node).
1661+
nd = any(DataFlow::FunctionNode f).getReturnNode()
1662+
or
16271663
// Skip the synthetic 'this' node, as a ThisExpr will be the next node anyway
16281664
nd = DataFlow::thisNode(_)
16291665
or

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -922,6 +922,32 @@ module DataFlow {
922922
override File getFile() { result = function.getFile() }
923923
}
924924

925+
/**
926+
* A data flow node representing the values returned by a function.
927+
*/
928+
class FunctionReturnNode extends DataFlow::Node, TFunctionReturnNode {
929+
Function function;
930+
931+
FunctionReturnNode() { this = TFunctionReturnNode(function) }
932+
933+
override string toString() { result = "return of " + function.describe() }
934+
935+
override predicate hasLocationInfo(
936+
string filepath, int startline, int startcolumn, int endline, int endcolumn
937+
) {
938+
function.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
939+
}
940+
941+
override BasicBlock getBasicBlock() { result = function.(ExprOrStmt).getBasicBlock() }
942+
943+
/**
944+
* Gets the function corresponding to this return node.
945+
*/
946+
Function getFunction() { result = function }
947+
948+
override File getFile() { result = function.getFile() }
949+
}
950+
925951
/**
926952
* A data flow node representing the exceptions thrown by the callee of an invocation.
927953
*/
@@ -1265,6 +1291,13 @@ module DataFlow {
12651291
nd = TExceptionalFunctionReturnNode(function)
12661292
}
12671293

1294+
/**
1295+
* INTERNAL: Use `FunctionReturnNode` instead.
1296+
*/
1297+
predicate functionReturnNode(DataFlow::Node nd, Function function) {
1298+
nd = TFunctionReturnNode(function)
1299+
}
1300+
12681301
/**
12691302
* Gets the data flow node corresponding the given l-value expression, if
12701303
* such a node exists.
@@ -1460,6 +1493,11 @@ module DataFlow {
14601493
localCall(succExpr, f)
14611494
)
14621495
)
1496+
or
1497+
// from returned expr to the FunctionReturnNode.
1498+
exists(Function f | not f.isAsync() |
1499+
DataFlow::functionReturnNode(succ, f) and pred = valueNode(f.getAReturnedExpr())
1500+
)
14631501
}
14641502

14651503
/**

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,16 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode {
491491
DataFlow::ExceptionalFunctionReturnNode getExceptionalReturn() {
492492
DataFlow::exceptionalFunctionReturnNode(result, astNode)
493493
}
494+
495+
/**
496+
* Gets the data flow node representing the value returned from this function.
497+
*
498+
* Note that this differs from `getAReturn()`, in that every function has exactly
499+
* one canonical return node, but may have multiple (or zero) returned expressions.
500+
* The result of `getAReturn()` is always a predecessor of `getReturnNode()`
501+
* in the data-flow graph.
502+
*/
503+
DataFlow::FunctionReturnNode getReturnNode() { DataFlow::functionReturnNode(result, astNode) }
494504
}
495505

496506
/**

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,9 @@ module SourceNode {
319319
this = DataFlow::destructuredModuleImportNode(_)
320320
or
321321
this = DataFlow::globalAccessPathRootPseudoNode()
322+
or
323+
// Include return nodes because they model the implicit Promise creation in async functions.
324+
DataFlow::functionReturnNode(this, _)
322325
}
323326
}
324327
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ private module NodeTracking {
243243
basicStoreStep(pred, succ, prop) and
244244
summary = PathSummary::level()
245245
or
246-
exists(Function f, DataFlow::Node mid |
246+
exists(Function f, DataFlow::Node mid | not f.isAsync() |
247247
// `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller,
248248
// and `succ` is an invocation of `f`
249249
reachableFromInput(f, succ, pred, mid, summary) and
@@ -266,7 +266,7 @@ private module NodeTracking {
266266
basicLoadStep(pred, succ, prop) and
267267
summary = PathSummary::level()
268268
or
269-
exists(Function f, DataFlow::SourceNode parm |
269+
exists(Function f, DataFlow::SourceNode parm | not f.isAsync() |
270270
argumentPassing(succ, pred, f, parm) and
271271
reachesReturn(f, parm.getAPropertyRead(prop), summary)
272272
)

‎javascript/ql/src/semmle/javascript/dataflow/internal/DataFlowNode.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ newtype TNode =
2727
exists(decl.getASpecifier().getImportedName())
2828
} or
2929
THtmlAttributeNode(HTML::Attribute attr) or
30+
TFunctionReturnNode(Function f) or
3031
TExceptionalFunctionReturnNode(Function f) or
3132
TExceptionalInvocationReturnNode(InvokeExpr e) or
3233
TGlobalAccessPathRoot()

‎javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll‎

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,40 @@ predicate localFlowStep(
6161
* Holds if an exception thrown from `pred` can propagate locally to `succ`.
6262
*/
6363
predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
64+
localExceptionStepWithAsyncFlag(pred, succ, false)
65+
}
66+
67+
/**
68+
* Holds if an exception thrown from `pred` can propagate locally to `succ`.
69+
*
70+
* The `async` flag is true if the step involves wrapping the exception in a rejected Promise.
71+
*/
72+
predicate localExceptionStepWithAsyncFlag(DataFlow::Node pred, DataFlow::Node succ, boolean async) {
73+
exists(DataFlow::Node target | target = getThrowTarget(pred) |
74+
async = false and
75+
succ = target and
76+
not succ = any(DataFlow::FunctionNode f | f.getFunction().isAsync()).getExceptionalReturn()
77+
or
78+
async = true and
79+
exists(DataFlow::FunctionNode f | f.getExceptionalReturn() = target |
80+
succ = f.getReturnNode() // returns a rejected promise - therefore using the ordinary return node.
81+
)
82+
)
83+
}
84+
85+
/**
86+
* Gets the dataflow-node that an exception thrown at `thrower` will flow to.
87+
*
88+
* The predicate that all functions are not async.
89+
*/
90+
DataFlow::Node getThrowTarget(DataFlow::Node thrower) {
6491
exists(Expr expr |
6592
expr = any(ThrowStmt throw).getExpr() and
66-
pred = expr.flow()
93+
thrower = expr.flow()
6794
or
68-
DataFlow::exceptionalInvocationReturnNode(pred, expr)
95+
DataFlow::exceptionalInvocationReturnNode(thrower, expr)
6996
|
70-
succ = expr.getExceptionTarget()
97+
result = expr.getExceptionTarget()
7198
)
7299
}
73100

@@ -213,14 +240,14 @@ private module CachedSteps {
213240

214241
/**
215242
* Holds if there is a flow step from `pred` to `succ` through:
216-
* - returning a value from a function call, or
243+
* - returning a value from a function call (from the special `FunctionReturnNode`), or
217244
* - throwing an exception out of a function call, or
218245
* - the receiver flowing out of a constructor call.
219246
*/
220247
cached
221248
predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
222249
exists(Function f | calls(succ, f) or callsBound(succ, f, _) |
223-
returnExpr(f, pred, _)
250+
DataFlow::functionReturnNode(pred, f)
224251
or
225252
succ instanceof DataFlow::NewNode and
226253
DataFlow::thisNode(pred, f)

‎javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ test_getAFunctionValue
6767
| es2015.js:2:14:4:3 | () {\\n ... ");\\n } | es2015.js:2:14:4:3 | () {\\n ... ");\\n } |
6868
| es2015.js:6:5:6:16 | ExampleClass | es2015.js:2:14:4:3 | () {\\n ... ");\\n } |
6969
| es2015.js:8:2:12:1 | functio ... \\n };\\n} | es2015.js:8:2:12:1 | functio ... \\n };\\n} |
70+
| es2015.js:8:2:12:1 | return of anonymous function | es2015.js:9:10:11:3 | () => { ... ();\\n } |
7071
| es2015.js:9:10:11:3 | () => { ... ();\\n } | es2015.js:9:10:11:3 | () => { ... ();\\n } |
7172
| es2015.js:10:5:10:20 | arguments.callee | es2015.js:8:2:12:1 | functio ... \\n };\\n} |
7273
| es2015.js:10:5:10:22 | arguments.callee() | es2015.js:9:10:11:3 | () => { ... ();\\n } |

‎javascript/ql/test/library-tests/DataFlow/flowStep.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@
1717
| sources.js:1:6:1:6 | x | sources.js:1:11:1:11 | x |
1818
| sources.js:1:6:1:11 | x => x | sources.js:1:5:1:12 | (x => x) |
1919
| sources.js:1:11:1:11 | x | sources.js:1:1:1:12 | new (x => x) |
20+
| sources.js:1:11:1:11 | x | sources.js:1:6:1:11 | return of anonymous function |
2021
| sources.js:3:2:5:1 | functio ... x+19;\\n} | sources.js:3:1:5:2 | (functi ... +19;\\n}) |
2122
| sources.js:3:11:3:11 | x | sources.js:3:11:3:11 | x |
2223
| sources.js:3:11:3:11 | x | sources.js:4:10:4:10 | x |
2324
| sources.js:4:10:4:13 | x+19 | sources.js:3:1:5:6 | (functi ... \\n})(23) |
25+
| sources.js:4:10:4:13 | x+19 | sources.js:3:2:5:1 | return of anonymous function |
2426
| sources.js:5:4:5:5 | 23 | sources.js:3:11:3:11 | x |
2527
| sources.js:9:14:9:18 | array | sources.js:9:14:9:18 | array |
2628
| sources.js:9:14:9:18 | array | sources.js:10:19:10:23 | array |
@@ -89,7 +91,9 @@
8991
| tst.js:16:13:16:13 | a | tst.js:16:13:16:13 | a |
9092
| tst.js:16:13:16:13 | a | tst.js:18:12:18:12 | a |
9193
| tst.js:18:12:18:12 | a | tst.js:16:1:20:9 | (functi ... ("arg") |
94+
| tst.js:18:12:18:12 | a | tst.js:16:2:20:1 | return of function f |
9295
| tst.js:19:10:19:11 | "" | tst.js:16:1:20:9 | (functi ... ("arg") |
96+
| tst.js:19:10:19:11 | "" | tst.js:16:2:20:1 | return of function f |
9397
| tst.js:20:4:20:8 | "arg" | tst.js:16:13:16:13 | a |
9498
| tst.js:22:5:22:25 | readFileSync | tst.js:23:1:23:12 | readFileSync |
9599
| tst.js:22:7:22:18 | readFileSync | tst.js:22:5:22:25 | readFileSync |
@@ -102,11 +106,13 @@
102106
| tst.js:28:2:28:1 | x | tst.js:29:3:29:3 | x |
103107
| tst.js:28:2:29:3 | () =>\\n x | tst.js:28:1:30:1 | (() =>\\n ... ables\\n) |
104108
| tst.js:29:3:29:3 | x | tst.js:28:1:30:3 | (() =>\\n ... les\\n)() |
109+
| tst.js:29:3:29:3 | x | tst.js:28:2:29:3 | return of anonymous function |
105110
| tst.js:32:1:32:0 | x | tst.js:33:10:33:10 | x |
106111
| tst.js:32:1:34:1 | functio ... ables\\n} | tst.js:32:10:32:10 | g |
107112
| tst.js:32:10:32:10 | g | tst.js:35:1:35:1 | g |
108113
| tst.js:32:10:32:10 | g | tst.js:60:1:60:1 | g |
109114
| tst.js:32:10:32:10 | g | tst.js:62:4:62:4 | g |
115+
| tst.js:33:10:33:10 | x | tst.js:32:1:34:1 | return of function g |
110116
| tst.js:37:5:42:1 | o | tst.js:43:1:43:1 | o |
111117
| tst.js:37:5:42:1 | o | tst.js:44:1:44:1 | o |
112118
| tst.js:37:5:42:1 | o | tst.js:61:3:61:3 | o |
@@ -142,6 +148,7 @@
142148
| tst.js:90:15:90:15 | o | tst.js:90:4:90:11 | { r: z } |
143149
| tst.js:90:15:90:15 | o | tst.js:90:4:90:15 | { r: z } = o |
144150
| tst.js:91:10:91:18 | x + y + z | tst.js:87:1:96:2 | (functi ... r: 0\\n}) |
151+
| tst.js:91:10:91:18 | x + y + z | tst.js:87:2:92:1 | return of anonymous function |
145152
| tst.js:92:4:96:1 | {\\n p: ... r: 0\\n} | tst.js:87:11:87:24 | { p: x, ...o } |
146153
| tst.js:98:2:103:1 | functio ... + z;\\n} | tst.js:98:1:103:2 | (functi ... + z;\\n}) |
147154
| tst.js:98:11:98:24 | rest | tst.js:99:15:99:18 | rest |
@@ -156,6 +163,7 @@
156163
| tst.js:101:13:101:16 | rest | tst.js:101:3:101:9 | [ , z ] |
157164
| tst.js:101:13:101:16 | rest | tst.js:101:3:101:16 | [ , z ] = rest |
158165
| tst.js:102:10:102:18 | x + y + z | tst.js:98:1:103:17 | (functi ... 3, 0 ]) |
166+
| tst.js:102:10:102:18 | x + y + z | tst.js:98:2:103:1 | return of anonymous function |
159167
| tst.js:103:4:103:16 | [ 19, 23, 0 ] | tst.js:98:11:98:24 | [ x, ...rest ] |
160168
| tst.js:105:1:105:1 | x | tst.js:105:1:105:6 | x ?? y |
161169
| tst.js:105:6:105:6 | y | tst.js:105:1:105:6 | x ?? y |

0 commit comments

Comments
 (0)