Skip to content

Commit 269bbc9

Browse files
asger-semmleasger-semmle
committed
JavaScript: add flow steps through partial function application
1 parent 7f56be6 commit 269bbc9

File tree

7 files changed

+147
-0
lines changed

7 files changed

+147
-0
lines changed

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

Lines changed: 44 additions & 0 deletions
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+
abstract class AdditionalPartialInvokeNode extends DataFlow::InvokeNode {
292+
/**
293+
* Holds if `argument` is passed as argument `index` to the function in `callback`.
294+
*/
295+
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 = 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`.
@@ -445,6 +488,7 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
445488
DataFlow::Configuration cfg, boolean valuePreserving) {
446489
exists (Function f, DataFlow::ValueNode ret |
447490
ret.asExpr() = f.getAReturnedExpr() and
491+
calls(invk, f) and // Do not consider partial calls
448492
reachableFromInput(f, invk, input, ret, cfg, PathSummary::level(valuePreserving))
449493
)
450494
}

‎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+
predicate partiallyCalls(DataFlow::AdditionalPartialInvokeNode invk, DataFlow::Node callback, Function f) {
36+
invk.isPartialArgument(callback, _, _) and
37+
exists (AbstractFunction callee | callee = callback.analyze().getAValue() |
38+
if invk.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/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)