Skip to content

Commit aadb148

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#2855 from asger-semmle/js/returned-partial-call
Approved by esbena
2 parents ea4ca31 + e665e3c commit aadb148

File tree

14 files changed

+204
-21
lines changed

14 files changed

+204
-21
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313

1414
* The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
1515

16-
* Calls can now be resolved to class members in more cases, leading to more results from the security queries.
16+
* The call graph construction has been improved, leading to more results from the security queries:
17+
- Calls can now be resolved to indirectly-defined class members in more cases.
18+
- Calls through partial invocations such as `.bind` can now be resolved in more cases.
1719

1820
* Support for the following frameworks and libraries has been improved:
1921
- [Electron](https://electronjs.org/)

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

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
private import javascript
7272
private import internal.FlowSteps
7373
private import internal.AccessPaths
74+
private import internal.CallGraphs
7475

7576
/**
7677
* A data flow tracking configuration for finding inter-procedural paths from
@@ -620,10 +621,11 @@ private predicate exploratoryFlowStep(
620621
isAdditionalStoreStep(pred, succ, _, cfg) or
621622
isAdditionalLoadStep(pred, succ, _, cfg) or
622623
isAdditionalLoadStoreStep(pred, succ, _, cfg) or
623-
// the following two disjuncts taken together over-approximate flow through
624+
// the following three disjuncts taken together over-approximate flow through
624625
// higher-order calls
625626
callback(pred, succ) or
626-
succ = pred.(DataFlow::FunctionNode).getAParameter()
627+
succ = pred.(DataFlow::FunctionNode).getAParameter() or
628+
exploratoryBoundInvokeStep(pred, succ)
627629
}
628630

629631
/**
@@ -751,7 +753,7 @@ private predicate flowThroughCall(
751753
) {
752754
exists(Function f, DataFlow::ValueNode ret |
753755
ret.asExpr() = f.getAReturnedExpr() and
754-
calls(output, f) and // Do not consider partial calls
756+
(calls(output, f) or callsBound(output, f, _)) and // Do not consider partial calls
755757
reachableFromInput(f, output, input, ret, cfg, summary) and
756758
not isBarrierEdge(cfg, ret, output) and
757759
not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
@@ -761,7 +763,7 @@ private predicate flowThroughCall(
761763
exists(Function f, DataFlow::Node invk, DataFlow::Node ret |
762764
DataFlow::exceptionalFunctionReturnNode(ret, f) and
763765
DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and
764-
calls(invk, f) and
766+
(calls(invk, f) or callsBound(invk, f, _)) and
765767
reachableFromInput(f, invk, input, ret, cfg, summary) and
766768
not isBarrierEdge(cfg, ret, output) and
767769
not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
@@ -1032,6 +1034,13 @@ private predicate flowIntoHigherOrderCall(
10321034
succ = cb.getParameter(i) and
10331035
summary = oldSummary.append(PathSummary::call())
10341036
)
1037+
or
1038+
exists(DataFlow::SourceNode cb, DataFlow::FunctionNode f, int i, int boundArgs, PathSummary oldSummary |
1039+
higherOrderCall(pred, cb, i, cfg, oldSummary) and
1040+
cb = CallGraph::getABoundFunctionReference(f, boundArgs, false) and
1041+
succ = f.getParameter(boundArgs + i) and
1042+
summary = oldSummary.append(PathSummary::call())
1043+
)
10351044
}
10361045

10371046
/**

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ module DataFlow {
147147
final FunctionNode getABoundFunctionValue(int boundArgs) {
148148
result = getAFunctionValue() and boundArgs = 0
149149
or
150-
CallGraph::getABoundFunctionReference(result, boundArgs).flowsTo(this)
150+
CallGraph::getABoundFunctionReference(result, boundArgs, _).flowsTo(this)
151151
}
152152

153153
/**

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ module CallGraph {
7474
*/
7575
pragma[nomagic]
7676
private
77-
DataFlow::SourceNode getABoundFunctionReference(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t) {
77+
DataFlow::SourceNode getABoundFunctionReferenceAux(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t) {
7878
exists(DataFlow::PartialInvokeNode partial, DataFlow::Node callback |
7979
result = partial.getBoundFunction(callback, boundArgs) and
8080
getAFunctionReference(function, 0, t.continue()).flowsTo(callback)
@@ -90,7 +90,7 @@ module CallGraph {
9090
private
9191
DataFlow::SourceNode getABoundFunctionReferenceAux(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t, DataFlow::StepSummary summary) {
9292
exists(DataFlow::SourceNode prev |
93-
prev = getABoundFunctionReference(function, boundArgs, t) and
93+
prev = getABoundFunctionReferenceAux(function, boundArgs, t) and
9494
DataFlow::StepSummary::step(prev, result, summary)
9595
)
9696
}
@@ -100,8 +100,12 @@ module CallGraph {
100100
* with `function` as the underlying function.
101101
*/
102102
cached
103-
DataFlow::SourceNode getABoundFunctionReference(DataFlow::FunctionNode function, int boundArgs) {
104-
result = getABoundFunctionReference(function, boundArgs, DataFlow::TypeTracker::end())
103+
DataFlow::SourceNode getABoundFunctionReference(DataFlow::FunctionNode function, int boundArgs, boolean contextDependent) {
104+
exists(DataFlow::TypeTracker t |
105+
result = getABoundFunctionReferenceAux(function, boundArgs, t) and
106+
t.end() and
107+
contextDependent = t.hasCall()
108+
)
105109
}
106110

107111
/**

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

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import javascript
88
import semmle.javascript.dataflow.Configuration
9+
import semmle.javascript.dataflow.internal.CallGraphs
910

1011
/**
1112
* Holds if flow should be tracked through properties of `obj`.
@@ -91,6 +92,32 @@ private module CachedSteps {
9192
cached
9293
predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
9394

95+
/**
96+
* Holds if `invk` may invoke a bound version of `f` with `boundArgs` already bound.
97+
*
98+
* The receiver is assumed to be bound as well, and should not propagate into `f`.
99+
*
100+
* Does not hold for context-dependent call sites, such as callback invocations.
101+
*/
102+
cached
103+
predicate callsBound(DataFlow::InvokeNode invk, Function f, int boundArgs) {
104+
CallGraph::getABoundFunctionReference(f.flow(), boundArgs, false).flowsTo(invk.getCalleeNode())
105+
}
106+
107+
/**
108+
* Holds if `pred` may flow to `succ` through an invocation of a bound function.
109+
*
110+
* Should only be used for graph pruning, as the edge may lead to spurious flow.
111+
*/
112+
cached
113+
predicate exploratoryBoundInvokeStep(DataFlow::Node pred, DataFlow::Node succ) {
114+
exists(DataFlow::InvokeNode invk, DataFlow::FunctionNode f, int i, int boundArgs |
115+
CallGraph::getABoundFunctionReference(f, boundArgs, _).flowsTo(invk.getCalleeNode()) and
116+
pred = invk.getArgument(i) and
117+
succ = f.getParameter(i + boundArgs)
118+
)
119+
}
120+
94121
/**
95122
* Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
96123
*
@@ -141,6 +168,14 @@ private module CachedSteps {
141168
partiallyCalls(invk, callback, f) and
142169
parm = DataFlow::thisNode(f)
143170
)
171+
or
172+
exists(int boundArgs, int i, Parameter p |
173+
callsBound(invk, f, boundArgs) and
174+
f.getParameter(boundArgs + i) = p and
175+
not p.isRestParameter() and
176+
arg = invk.getArgument(i) and
177+
parm = DataFlow::parameterNode(p)
178+
)
144179
}
145180

146181
/**
@@ -158,7 +193,7 @@ private module CachedSteps {
158193
*/
159194
cached
160195
predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
161-
exists(Function f | calls(succ, f) |
196+
exists(Function f | calls(succ, f) or callsBound(succ, f, _) |
162197
returnExpr(f, pred, _)
163198
or
164199
succ instanceof DataFlow::NewNode and
@@ -167,8 +202,11 @@ private module CachedSteps {
167202
or
168203
exists(InvokeExpr invoke, Function fun |
169204
DataFlow::exceptionalFunctionReturnNode(pred, fun) and
170-
DataFlow::exceptionalInvocationReturnNode(succ, invoke) and
205+
DataFlow::exceptionalInvocationReturnNode(succ, invoke)
206+
|
171207
calls(invoke.flow(), fun)
208+
or
209+
callsBound(invoke.flow(), fun, _)
172210
)
173211
}
174212

@@ -464,4 +502,3 @@ module PathSummary {
464502
*/
465503
PathSummary return() { exists(FlowLabel lbl | result = MkPathSummary(true, false, lbl, lbl)) }
466504
}
467-

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
spuriousCallee
22
missingCallee
3-
| constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} |
4-
| constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} |
3+
| constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 |
4+
| constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 |
55
badAnnotation

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,16 +33,37 @@ class AnnotatedCall extends InvokeExpr {
3333
string getCallTargetName() { result = calls }
3434

3535
AnnotatedFunction getAnExpectedCallee() { result.getCalleeName() = getCallTargetName() }
36+
37+
int getBoundArgs() {
38+
result = getAnnotation(this, "boundArgs").toInt()
39+
}
40+
41+
int getBoundArgsOrMinusOne() {
42+
result = getBoundArgs()
43+
or
44+
not exists(getBoundArgs()) and
45+
result = -1
46+
}
47+
}
48+
49+
predicate callEdge(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
50+
FlowSteps::calls(call.flow(), target) and boundArgs = -1
51+
or
52+
FlowSteps::callsBound(call.flow(), target, boundArgs)
3653
}
3754

38-
query predicate spuriousCallee(AnnotatedCall call, AnnotatedFunction target) {
39-
FlowSteps::calls(call.flow(), target) and
40-
not target = call.getAnExpectedCallee()
55+
query predicate spuriousCallee(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
56+
callEdge(call, target, boundArgs) and
57+
not (
58+
target = call.getAnExpectedCallee() and
59+
boundArgs = call.getBoundArgsOrMinusOne()
60+
)
4161
}
4262

43-
query predicate missingCallee(AnnotatedCall call, AnnotatedFunction target) {
44-
not FlowSteps::calls(call.flow(), target) and
45-
target = call.getAnExpectedCallee()
63+
query predicate missingCallee(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
64+
not callEdge(call, target, boundArgs) and
65+
target = call.getAnExpectedCallee() and
66+
boundArgs = call.getBoundArgsOrMinusOne()
4667
}
4768

4869
query predicate badAnnotation(string name) {

‎javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/bound-function.js

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
var url = require('url')
2+
var http = require('http')
3+
4+
function Mount () {}
5+
6+
/** name:mount.serve */
7+
Mount.prototype.serve = function (x) {
8+
}
9+
10+
function makeMount() {
11+
var m = new Mount()
12+
return m.serve.bind(m);
13+
}
14+
15+
function makeMount2(x) {
16+
var m = new Mount()
17+
return m.serve.bind(m, x);
18+
}
19+
20+
var mount = makeMount()
21+
var mount2 = makeMount2(null);
22+
23+
http.createServer(function (req, res) {
24+
/** calls:mount.serve */
25+
/** boundArgs:0 */
26+
mount(req, res)
27+
28+
/** calls:mount.serve */
29+
/** boundArgs:1 */
30+
mount2(res)
31+
});

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@
2222
| partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x |
2323
| partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x |
2424
| partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x |
25+
| partial.js:6:15:6:24 | "tainted2" | partial.js:10:15:10:15 | y |
26+
| partial.js:6:15:6:24 | "tainted2" | partial.js:16:15:16:15 | y |
27+
| partial.js:6:15:6:24 | "tainted2" | partial.js:22:15:22:15 | y |
28+
| partial.js:6:15:6:24 | "tainted2" | partial.js:28:15:28:15 | y |
2529
| promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val |
2630
| promises.js:2:16:2:24 | "tainted" | promises.js:38:32:38:32 | v |
2731
| promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v |

‎javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@
2323
| partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x |
2424
| partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x |
2525
| partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x |
26+
| partial.js:6:15:6:24 | "tainted2" | partial.js:10:15:10:15 | y |
27+
| partial.js:6:15:6:24 | "tainted2" | partial.js:16:15:16:15 | y |
28+
| partial.js:6:15:6:24 | "tainted2" | partial.js:22:15:22:15 | y |
29+
| partial.js:6:15:6:24 | "tainted2" | partial.js:28:15:28:15 | y |
2630
| promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val |
2731
| promises.js:2:16:2:24 | "tainted" | promises.js:38:32:38:32 | v |
2832
| promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v |

0 commit comments

Comments
 (0)