Skip to content

Commit bd121b9

Browse files
erik-krogherik-krogh
authored
Merge pull request #15893 from erik-krogh/more-filter-taint
JS: allow more flow through .filter()
2 parents 53502a8 + 129286a commit bd121b9

File tree

4 files changed

+61
-2
lines changed

4 files changed

+61
-2
lines changed

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ module ArrayTaintTracking {
3636
succ = call
3737
)
3838
or
39-
// `array.filter(x => x)` and `array.filter(x => !!x)` keeps the taint
39+
// `array.filter(x => x)` and `array.filter(x => !<something>)` keeps the taint
40+
// the latter is assumed to filter away only specific values, thus keeping the taint
4041
call.(DataFlow::MethodCallNode).getMethodName() = "filter" and
4142
pred = call.getReceiver() and
4243
succ = call and
@@ -47,7 +48,7 @@ module ArrayTaintTracking {
4748
|
4849
param = ret
4950
or
50-
param = DataFlow::exprNode(ret.asExpr().(LogNotExpr).getOperand().(LogNotExpr).getOperand())
51+
ret.asExpr() instanceof LogNotExpr
5152
)
5253
or
5354
// `array.reduce` with tainted value in callback

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,26 @@ nodes
8484
| ReflectedXss.js:110:16:110:30 | request.query.p |
8585
| ReflectedXss.js:110:16:110:30 | request.query.p |
8686
| ReflectedXss.js:110:16:110:30 | request.query.p |
87+
| ReflectedXss.js:114:11:114:41 | queryKeys |
88+
| ReflectedXss.js:114:13:114:27 | keys: queryKeys |
89+
| ReflectedXss.js:114:13:114:27 | keys: queryKeys |
90+
| ReflectedXss.js:116:11:116:45 | keys |
91+
| ReflectedXss.js:116:18:116:26 | queryKeys |
92+
| ReflectedXss.js:116:18:116:45 | queryKe ... s?.keys |
93+
| ReflectedXss.js:116:31:116:45 | paramKeys?.keys |
94+
| ReflectedXss.js:116:31:116:45 | paramKeys?.keys |
95+
| ReflectedXss.js:118:11:118:61 | keyArray |
96+
| ReflectedXss.js:118:22:118:61 | typeof ... : keys |
97+
| ReflectedXss.js:118:49:118:54 | [keys] |
98+
| ReflectedXss.js:118:50:118:53 | keys |
99+
| ReflectedXss.js:118:58:118:61 | keys |
100+
| ReflectedXss.js:119:11:119:72 | invalidKeys |
101+
| ReflectedXss.js:119:25:119:32 | keyArray |
102+
| ReflectedXss.js:119:25:119:72 | keyArra ... s(key)) |
103+
| ReflectedXss.js:122:30:122:73 | `${inva ... telist` |
104+
| ReflectedXss.js:122:30:122:73 | `${inva ... telist` |
105+
| ReflectedXss.js:122:33:122:43 | invalidKeys |
106+
| ReflectedXss.js:122:33:122:54 | invalid ... n(', ') |
87107
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
88108
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
89109
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id |
@@ -307,6 +327,26 @@ edges
307327
| ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
308328
| ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
309329
| ReflectedXss.js:110:16:110:30 | request.query.p | ReflectedXss.js:110:16:110:30 | request.query.p |
330+
| ReflectedXss.js:114:11:114:41 | queryKeys | ReflectedXss.js:116:18:116:26 | queryKeys |
331+
| ReflectedXss.js:114:13:114:27 | keys: queryKeys | ReflectedXss.js:114:11:114:41 | queryKeys |
332+
| ReflectedXss.js:114:13:114:27 | keys: queryKeys | ReflectedXss.js:114:11:114:41 | queryKeys |
333+
| ReflectedXss.js:116:11:116:45 | keys | ReflectedXss.js:118:50:118:53 | keys |
334+
| ReflectedXss.js:116:11:116:45 | keys | ReflectedXss.js:118:58:118:61 | keys |
335+
| ReflectedXss.js:116:18:116:26 | queryKeys | ReflectedXss.js:116:18:116:45 | queryKe ... s?.keys |
336+
| ReflectedXss.js:116:18:116:45 | queryKe ... s?.keys | ReflectedXss.js:116:11:116:45 | keys |
337+
| ReflectedXss.js:116:31:116:45 | paramKeys?.keys | ReflectedXss.js:116:18:116:45 | queryKe ... s?.keys |
338+
| ReflectedXss.js:116:31:116:45 | paramKeys?.keys | ReflectedXss.js:116:18:116:45 | queryKe ... s?.keys |
339+
| ReflectedXss.js:118:11:118:61 | keyArray | ReflectedXss.js:119:25:119:32 | keyArray |
340+
| ReflectedXss.js:118:22:118:61 | typeof ... : keys | ReflectedXss.js:118:11:118:61 | keyArray |
341+
| ReflectedXss.js:118:49:118:54 | [keys] | ReflectedXss.js:118:22:118:61 | typeof ... : keys |
342+
| ReflectedXss.js:118:50:118:53 | keys | ReflectedXss.js:118:49:118:54 | [keys] |
343+
| ReflectedXss.js:118:58:118:61 | keys | ReflectedXss.js:118:22:118:61 | typeof ... : keys |
344+
| ReflectedXss.js:119:11:119:72 | invalidKeys | ReflectedXss.js:122:33:122:43 | invalidKeys |
345+
| ReflectedXss.js:119:25:119:32 | keyArray | ReflectedXss.js:119:25:119:72 | keyArra ... s(key)) |
346+
| ReflectedXss.js:119:25:119:72 | keyArra ... s(key)) | ReflectedXss.js:119:11:119:72 | invalidKeys |
347+
| ReflectedXss.js:122:33:122:43 | invalidKeys | ReflectedXss.js:122:33:122:54 | invalid ... n(', ') |
348+
| ReflectedXss.js:122:33:122:54 | invalid ... n(', ') | ReflectedXss.js:122:30:122:73 | `${inva ... telist` |
349+
| ReflectedXss.js:122:33:122:54 | invalid ... n(', ') | ReflectedXss.js:122:30:122:73 | `${inva ... telist` |
310350
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
311351
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
312352
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
@@ -461,6 +501,8 @@ edges
461501
| ReflectedXss.js:100:12:100:39 | markdow ... q.body) | ReflectedXss.js:100:31:100:38 | req.body | ReflectedXss.js:100:12:100:39 | markdow ... q.body) | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:100:31:100:38 | req.body | user-provided value |
462502
| ReflectedXss.js:103:12:103:84 | markdow ... q.body) | ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:103:76:103:83 | req.body | user-provided value |
463503
| ReflectedXss.js:110:16:110:30 | request.query.p | ReflectedXss.js:110:16:110:30 | request.query.p | ReflectedXss.js:110:16:110:30 | request.query.p | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:110:16:110:30 | request.query.p | user-provided value |
504+
| ReflectedXss.js:122:30:122:73 | `${inva ... telist` | ReflectedXss.js:114:13:114:27 | keys: queryKeys | ReflectedXss.js:122:30:122:73 | `${inva ... telist` | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:114:13:114:27 | keys: queryKeys | user-provided value |
505+
| ReflectedXss.js:122:30:122:73 | `${inva ... telist` | ReflectedXss.js:116:31:116:45 | paramKeys?.keys | ReflectedXss.js:122:30:122:73 | `${inva ... telist` | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:116:31:116:45 | paramKeys?.keys | user-provided value |
464506
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to a $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
465507
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to a $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
466508
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to a $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,3 +109,17 @@ hapi.route({
109109
handler: function (request){
110110
return request.query.p; // NOT OK
111111
}});
112+
113+
app.get("invalid/keys/:id", async (req, res) => {
114+
const { keys: queryKeys } = req.query;
115+
const paramKeys = req.params;
116+
const keys = queryKeys || paramKeys?.keys;
117+
118+
const keyArray = typeof keys === 'string' ? [keys] : keys;
119+
const invalidKeys = keyArray.filter(key => !whitelist.includes(key));
120+
121+
if (invalidKeys.length) {
122+
res.status(400).send(`${invalidKeys.join(', ')} not in whitelist`);
123+
return;
124+
}
125+
});

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
| ReflectedXss.js:100:12:100:39 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:100:31:100:38 | req.body | user-provided value |
2020
| ReflectedXss.js:103:12:103:84 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:103:76:103:83 | req.body | user-provided value |
2121
| ReflectedXss.js:110:16:110:30 | request.query.p | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:110:16:110:30 | request.query.p | user-provided value |
22+
| ReflectedXss.js:122:30:122:73 | `${inva ... telist` | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:114:13:114:27 | keys: queryKeys | user-provided value |
23+
| ReflectedXss.js:122:30:122:73 | `${inva ... telist` | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:116:31:116:45 | paramKeys?.keys | user-provided value |
2224
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
2325
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
2426
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |

0 commit comments

Comments
 (0)