Skip to content

Commit b35f450

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #162 from asger-semmle/partial-calls
Approved by esben-semmle, xiemaisi
2 parents 829a5cc + f0886fd commit b35f450

File tree

9 files changed

+213
-1
lines changed

9 files changed

+213
-1
lines changed

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

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,18 @@ abstract class AdditionalSink extends DataFlow::Node {
283283
abstract predicate isSinkFor(Configuration cfg);
284284
}
285285

286+
/**
287+
* An invocation that is modeled as a partial function application.
288+
*
289+
* This contributes additional argument-passing flow edges that should be added to all data flow configurations.
290+
*/
291+
cached abstract class AdditionalPartialInvokeNode extends DataFlow::InvokeNode {
292+
/**
293+
* Holds if `argument` is passed as argument `index` to the function in `callback`.
294+
*/
295+
cached abstract predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index);
296+
}
297+
286298
/**
287299
* Additional flow step to model flow from import specifiers into the SSA variable
288300
* corresponding to the imported variable.
@@ -299,6 +311,37 @@ private class FlowStepThroughImport extends AdditionalFlowStep, DataFlow::ValueN
299311
}
300312
}
301313

314+
/**
315+
* A partial call through the built-in `Function.prototype.bind`.
316+
*/
317+
private class BindPartialCall extends AdditionalPartialInvokeNode, DataFlow::MethodCallNode {
318+
BindPartialCall() {
319+
getMethodName() = "bind"
320+
}
321+
322+
override predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index) {
323+
callback = getReceiver() and
324+
argument = getArgument(index + 1)
325+
}
326+
}
327+
328+
/**
329+
* A partial call through `_.partial` or a function with a similar interface.
330+
*/
331+
private class LibraryPartialCall extends AdditionalPartialInvokeNode {
332+
LibraryPartialCall() {
333+
this = LodashUnderscore::member("partial").getACall() or
334+
this = DataFlow::moduleMember("ramda", "partial").getACall()
335+
}
336+
337+
override predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index) {
338+
callback = getArgument(0) and
339+
exists (DataFlow::ArrayLiteralNode array |
340+
array.flowsTo(getArgument(1)) and
341+
argument = array.getElement(index))
342+
}
343+
}
344+
302345
/**
303346
* Holds if there is a flow step from `pred` to `succ` described by `summary`
304347
* under configuration `cfg`.
@@ -395,16 +438,18 @@ private predicate isRelevant(DataFlow::Node nd, DataFlow::Configuration cfg) {
395438
* either `pred` is an argument of `f` and `succ` the corresponding parameter, or
396439
* `pred` is a variable definition whose value is captured by `f` at `succ`.
397440
*/
441+
pragma[noopt]
398442
private predicate callInputStep(Function f, DataFlow::Node invk,
399443
DataFlow::Node pred, DataFlow::Node succ,
400444
DataFlow::Configuration cfg) {
401-
isRelevant(pred, cfg) and
402445
(
446+
isRelevant(pred, cfg) and
403447
exists (Parameter parm |
404448
argumentPassing(invk, pred, f, parm) and
405449
succ = DataFlow::parameterNode(parm)
406450
)
407451
or
452+
isRelevant(pred, cfg) and
408453
exists (SsaDefinition prevDef, SsaDefinition def |
409454
pred = DataFlow::ssaDefinitionNode(prevDef) and
410455
calls(invk, f) and captures(f, prevDef, def) and
@@ -445,6 +490,7 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
445490
DataFlow::Configuration cfg, boolean valuePreserving) {
446491
exists (Function f, DataFlow::ValueNode ret |
447492
ret.asExpr() = f.getAReturnedExpr() and
493+
calls(invk, f) and // Do not consider partial calls
448494
reachableFromInput(f, invk, input, ret, cfg, PathSummary::level(valuePreserving))
449495
)
450496
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,21 @@ predicate calls(DataFlow::InvokeNode invk, Function f) {
2727
f = invk.getACallee()
2828
}
2929

30+
/**
31+
* Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
32+
*
33+
* This only holds for explicitly modeled partial calls.
34+
*/
35+
private predicate partiallyCalls(DataFlow::AdditionalPartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f) {
36+
invk.isPartialArgument(callback, _, _) and
37+
exists (AbstractFunction callee | callee = callback.getAValue() |
38+
if callback.getAValue().isIndefinite("global") then
39+
(f = callee.getFunction() and f.getFile() = invk.getFile())
40+
else
41+
f = callee.getFunction()
42+
)
43+
}
44+
3045
/**
3146
* Holds if `f` captures the variable defined by `def` in `cap`.
3247
*/
@@ -69,6 +84,11 @@ predicate argumentPassing(DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Fu
6984
f.getParameter(i) = parm and not parm.isRestParameter() and
7085
arg = invk.getArgument(i)
7186
)
87+
or
88+
exists (DataFlow::Node callback, int i |
89+
invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
90+
partiallyCalls(invk, callback, f) and
91+
parm = f.getParameter(i) and not parm.isRestParameter())
7292
}
7393

7494

‎javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:17:14:17:14 | x |
2+
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:20:14:20:14 | y |
3+
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value |
4+
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:41:10:41:18 | id(taint) |
5+
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:51:14:51:14 | x |
16
| tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |
27
| tst.js:2:13:2:20 | source() | tst.js:5:10:5:22 | "/" + x + "!" |
38
| tst.js:2:13:2:20 | source() | tst.js:14:10:14:17 | x.sort() |

‎javascript/ql/test/library-tests/TaintTracking/partialCalls.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
let R = require('ramda');
2+
3+
function test() {
4+
let taint = source();
5+
6+
function safe1(x, y) {
7+
sink(x); // OK - x is not tainted
8+
}
9+
function safe2(x, y) {
10+
sink(y); // OK - y is not tainted
11+
}
12+
13+
safe1.bind(null, "hello", taint)();
14+
safe2.bind(null, taint, "hello")();
15+
16+
function unsafe1(x, y) {
17+
sink(x); // NOT OK - x is tainted
18+
}
19+
function unsafe2(x ,y) {
20+
sink(y); // NOT OK - y is tainted
21+
}
22+
23+
unsafe1.bind(null, taint, "hello")();
24+
unsafe2.bind(null, "hello", taint)();
25+
26+
function safeprop(x) {
27+
sink(x.value); // OK - property `value` is not tainted
28+
}
29+
function unsafeprop(x) {
30+
sink(x.value); // NOT OK - property `value` is tainted
31+
}
32+
33+
safeprop.bind(null, {value: "hello", somethingElse: taint})();
34+
unsafeprop.bind(null, {value: taint, somethingElse: "hello"})();
35+
36+
function id(x) {
37+
return x;
38+
}
39+
40+
sink(id("hello")); // OK
41+
sink(id(taint)); // NOT OK
42+
43+
let taintGetter = id.bind(null, taint);
44+
sink(taintGetter); // OK - this is a function object
45+
sink(taintGetter()); // NOT OK - but not currently detected
46+
47+
function safearray(x) {
48+
sink(x); // OK
49+
}
50+
function unsafearray(x) {
51+
sink(x); // NOT OK
52+
}
53+
54+
let xs = ["hello"];
55+
let ys = [taint];
56+
R.partial(safearray, xs)();
57+
R.partial(unsafearray, ys)();
58+
}

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22
| etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
33
| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
44
| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
5+
| partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |
6+
| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value |
7+
| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value |
8+
| partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value |
59
| promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
610
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
711
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssPath.expected‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,18 @@ edges
1111
| formatting.js:4:16:4:29 | req.query.evil | formatting.js:4:9:4:29 | evil |
1212
| formatting.js:6:43:6:46 | evil | formatting.js:6:14:6:47 | util.fo ... , evil) |
1313
| formatting.js:7:49:7:52 | evil | formatting.js:7:14:7:53 | require ... , evil) |
14+
| partial.js:9:25:9:25 | x | partial.js:10:14:10:14 | x |
15+
| partial.js:10:14:10:14 | x | partial.js:10:14:10:18 | x + y |
16+
| partial.js:13:42:13:48 | req.url | partial.js:9:25:9:25 | x |
17+
| partial.js:18:25:18:25 | x | partial.js:19:14:19:14 | x |
18+
| partial.js:19:14:19:14 | x | partial.js:19:14:19:18 | x + y |
19+
| partial.js:22:52:22:58 | req.url | partial.js:18:25:18:25 | x |
20+
| partial.js:27:25:27:25 | x | partial.js:28:14:28:14 | x |
21+
| partial.js:28:14:28:14 | x | partial.js:28:14:28:18 | x + y |
22+
| partial.js:31:48:31:54 | req.url | partial.js:27:25:27:25 | x |
23+
| partial.js:36:25:36:25 | x | partial.js:37:14:37:14 | x |
24+
| partial.js:37:14:37:14 | x | partial.js:37:14:37:18 | x + y |
25+
| partial.js:40:43:40:49 | req.url | partial.js:36:25:36:25 | x |
1426
| promises.js:5:3:5:59 | new Pro ... .data)) | promises.js:6:11:6:11 | x |
1527
| promises.js:5:44:5:57 | req.query.data | promises.js:5:3:5:59 | new Pro ... .data)) |
1628
| promises.js:5:44:5:57 | req.query.data | promises.js:6:11:6:11 | x |
@@ -25,6 +37,10 @@ edges
2537
| etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
2638
| formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
2739
| formatting.js:7:14:7:53 | require ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
40+
| partial.js:10:14:10:18 | x + y | partial.js:13:42:13:48 | req.url | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |
41+
| partial.js:19:14:19:18 | x + y | partial.js:22:52:22:58 | req.url | partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value |
42+
| partial.js:28:14:28:18 | x + y | partial.js:31:48:31:54 | req.url | partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value |
43+
| partial.js:37:14:37:18 | x + y | partial.js:40:43:40:49 | req.url | partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value |
2844
| promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
2945
| promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
3046
| tst2.js:7:12:7:12 | p | tst2.js:6:9:6:9 | p | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
22
| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
33
| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
4+
| partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |
5+
| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value |
6+
| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value |
7+
| partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value |
48
| promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
59
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
610
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer_old.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ WARNING: Predicate flowsFrom has been deprecated and may be removed in future (R
55
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
66
| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
77
| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
8+
| partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |
9+
| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value |
10+
| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value |
11+
| partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value |
812
| promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
913
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
1014
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/partial.js‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
let express = require('express');
2+
let underscore = require('underscore');
3+
let lodash = require('lodash');
4+
let R = require('ramda');
5+
6+
let app = express();
7+
8+
app.get("/some/path", (req, res) => {
9+
function sendResponse(x, y) {
10+
res.send(x + y); // NOT OK
11+
}
12+
13+
let callback = sendResponse.bind(null, req.url);
14+
[1, 2, 3].forEach(callback);
15+
});
16+
17+
app.get("/underscore", (req, res) => {
18+
function sendResponse(x, y) {
19+
res.send(x + y); // NOT OK
20+
}
21+
22+
let callback = underscore.partial(sendResponse, [req.url]);
23+
[1, 2, 3].forEach(callback);
24+
});
25+
26+
app.get("/lodash", (req, res) => {
27+
function sendResponse(x, y) {
28+
res.send(x + y); // NOT OK
29+
}
30+
31+
let callback = lodash.partial(sendResponse, [req.url]);
32+
[1, 2, 3].forEach(callback);
33+
});
34+
35+
app.get("/ramda", (req, res) => {
36+
function sendResponse(x, y) {
37+
res.send(x + y); // NOT OK
38+
}
39+
40+
let callback = R.partial(sendResponse, [req.url]);
41+
[1, 2, 3].forEach(callback);
42+
});
43+
44+
app.get("/return", (req, res) => {
45+
function getFirst(x, y) {
46+
return x;
47+
}
48+
49+
let callback = getFirst.bind(null, req.url);
50+
51+
res.send(callback); // OK - the callback itself is not tainted
52+
res.send(callback()); // NOT OK - but not currently detected
53+
54+
res.send(getFirst("Hello")); // OK - argument is not tainted from this call site
55+
});

0 commit comments

Comments
 (0)