Skip to content

Commit 9534f31

Browse files
erik-krogherik-krogh
authored
Merge pull request #10343 from erik-krogh/spreadFunction
JS: recognize calls to `Function` when spread arguments are used
2 parents 2681b3d + 6447234 commit 9534f31

File tree

4 files changed

+68
-7
lines changed

4 files changed

+68
-7
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/CodeInjectionCustomizations.qll

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ module CodeInjection {
170170
exists(string callName | c = DataFlow::globalVarRef(callName).getAnInvocation() |
171171
callName = "eval" and index = 0
172172
or
173-
callName = "Function"
173+
callName = "Function" and index = -1
174174
or
175175
callName = "execScript" and index = 0
176176
or
@@ -185,14 +185,13 @@ module CodeInjection {
185185
callName = "setImmediate" and index = 0
186186
)
187187
or
188-
exists(DataFlow::GlobalVarRefNode wasm, string methodName |
189-
wasm.getName() = "WebAssembly" and c = wasm.getAMemberCall(methodName)
190-
|
191-
methodName = "compile" or
192-
methodName = "compileStreaming"
193-
)
188+
c = DataFlow::globalVarRef("WebAssembly").getAMemberCall(["compile", "compileStreaming"]) and
189+
index = -1
194190
|
195191
this = c.getArgument(index)
192+
or
193+
index = -1 and
194+
this = c.getAnArgument()
196195
)
197196
or
198197
// node-serialize is not intended to be safe for untrusted inputs

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,18 @@ nodes
157157
| tst.js:26:26:26:40 | location.search |
158158
| tst.js:26:26:26:53 | locatio ... ring(1) |
159159
| tst.js:26:26:26:53 | locatio ... ring(1) |
160+
| tst.js:29:9:29:82 | source |
161+
| tst.js:29:18:29:41 | documen ... .search |
162+
| tst.js:29:18:29:41 | documen ... .search |
163+
| tst.js:29:18:29:82 | documen ... , "$1") |
164+
| tst.js:31:18:31:23 | source |
165+
| tst.js:31:18:31:23 | source |
166+
| tst.js:33:14:33:19 | source |
167+
| tst.js:33:14:33:19 | source |
168+
| tst.js:35:28:35:33 | source |
169+
| tst.js:35:28:35:33 | source |
170+
| tst.js:37:33:37:38 | source |
171+
| tst.js:37:33:37:38 | source |
160172
edges
161173
| NoSQLCodeInjection.js:18:24:18:31 | req.body | NoSQLCodeInjection.js:18:24:18:37 | req.body.query |
162174
| NoSQLCodeInjection.js:18:24:18:31 | req.body | NoSQLCodeInjection.js:18:24:18:37 | req.body.query |
@@ -262,6 +274,17 @@ edges
262274
| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
263275
| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
264276
| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
277+
| tst.js:29:9:29:82 | source | tst.js:31:18:31:23 | source |
278+
| tst.js:29:9:29:82 | source | tst.js:31:18:31:23 | source |
279+
| tst.js:29:9:29:82 | source | tst.js:33:14:33:19 | source |
280+
| tst.js:29:9:29:82 | source | tst.js:33:14:33:19 | source |
281+
| tst.js:29:9:29:82 | source | tst.js:35:28:35:33 | source |
282+
| tst.js:29:9:29:82 | source | tst.js:35:28:35:33 | source |
283+
| tst.js:29:9:29:82 | source | tst.js:37:33:37:38 | source |
284+
| tst.js:29:9:29:82 | source | tst.js:37:33:37:38 | source |
285+
| tst.js:29:18:29:41 | documen ... .search | tst.js:29:18:29:82 | documen ... , "$1") |
286+
| tst.js:29:18:29:41 | documen ... .search | tst.js:29:18:29:82 | documen ... , "$1") |
287+
| tst.js:29:18:29:82 | documen ... , "$1") | tst.js:29:9:29:82 | source |
265288
#select
266289
| NoSQLCodeInjection.js:18:24:18:37 | req.body.query | NoSQLCodeInjection.js:18:24:18:31 | req.body | NoSQLCodeInjection.js:18:24:18:37 | req.body.query | $@ flows to here and is interpreted as code. | NoSQLCodeInjection.js:18:24:18:31 | req.body | User-provided value |
267290
| NoSQLCodeInjection.js:19:24:19:48 | "name = ... dy.name | NoSQLCodeInjection.js:19:36:19:43 | req.body | NoSQLCodeInjection.js:19:24:19:48 | "name = ... dy.name | $@ flows to here and is interpreted as code. | NoSQLCodeInjection.js:19:36:19:43 | req.body | User-provided value |
@@ -314,3 +337,7 @@ edges
314337
| tst.js:20:30:20:51 | documen ... on.hash | tst.js:20:30:20:51 | documen ... on.hash | tst.js:20:30:20:51 | documen ... on.hash | $@ flows to here and is interpreted as code. | tst.js:20:30:20:51 | documen ... on.hash | User-provided value |
315338
| tst.js:23:6:23:46 | atob(do ... ing(1)) | tst.js:23:11:23:32 | documen ... on.hash | tst.js:23:6:23:46 | atob(do ... ing(1)) | $@ flows to here and is interpreted as code. | tst.js:23:11:23:32 | documen ... on.hash | User-provided value |
316339
| tst.js:26:26:26:53 | locatio ... ring(1) | tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) | $@ flows to here and is interpreted as code. | tst.js:26:26:26:40 | location.search | User-provided value |
340+
| tst.js:31:18:31:23 | source | tst.js:29:18:29:41 | documen ... .search | tst.js:31:18:31:23 | source | $@ flows to here and is interpreted as code. | tst.js:29:18:29:41 | documen ... .search | User-provided value |
341+
| tst.js:33:14:33:19 | source | tst.js:29:18:29:41 | documen ... .search | tst.js:33:14:33:19 | source | $@ flows to here and is interpreted as code. | tst.js:29:18:29:41 | documen ... .search | User-provided value |
342+
| tst.js:35:28:35:33 | source | tst.js:29:18:29:41 | documen ... .search | tst.js:35:28:35:33 | source | $@ flows to here and is interpreted as code. | tst.js:29:18:29:41 | documen ... .search | User-provided value |
343+
| tst.js:37:33:37:38 | source | tst.js:29:18:29:41 | documen ... .search | tst.js:37:33:37:38 | source | $@ flows to here and is interpreted as code. | tst.js:29:18:29:41 | documen ... .search | User-provided value |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,18 @@ nodes
161161
| tst.js:26:26:26:40 | location.search |
162162
| tst.js:26:26:26:53 | locatio ... ring(1) |
163163
| tst.js:26:26:26:53 | locatio ... ring(1) |
164+
| tst.js:29:9:29:82 | source |
165+
| tst.js:29:18:29:41 | documen ... .search |
166+
| tst.js:29:18:29:41 | documen ... .search |
167+
| tst.js:29:18:29:82 | documen ... , "$1") |
168+
| tst.js:31:18:31:23 | source |
169+
| tst.js:31:18:31:23 | source |
170+
| tst.js:33:14:33:19 | source |
171+
| tst.js:33:14:33:19 | source |
172+
| tst.js:35:28:35:33 | source |
173+
| tst.js:35:28:35:33 | source |
174+
| tst.js:37:33:37:38 | source |
175+
| tst.js:37:33:37:38 | source |
164176
edges
165177
| NoSQLCodeInjection.js:18:24:18:31 | req.body | NoSQLCodeInjection.js:18:24:18:37 | req.body.query |
166178
| NoSQLCodeInjection.js:18:24:18:31 | req.body | NoSQLCodeInjection.js:18:24:18:37 | req.body.query |
@@ -270,5 +282,16 @@ edges
270282
| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
271283
| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
272284
| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
285+
| tst.js:29:9:29:82 | source | tst.js:31:18:31:23 | source |
286+
| tst.js:29:9:29:82 | source | tst.js:31:18:31:23 | source |
287+
| tst.js:29:9:29:82 | source | tst.js:33:14:33:19 | source |
288+
| tst.js:29:9:29:82 | source | tst.js:33:14:33:19 | source |
289+
| tst.js:29:9:29:82 | source | tst.js:35:28:35:33 | source |
290+
| tst.js:29:9:29:82 | source | tst.js:35:28:35:33 | source |
291+
| tst.js:29:9:29:82 | source | tst.js:37:33:37:38 | source |
292+
| tst.js:29:9:29:82 | source | tst.js:37:33:37:38 | source |
293+
| tst.js:29:18:29:41 | documen ... .search | tst.js:29:18:29:82 | documen ... , "$1") |
294+
| tst.js:29:18:29:41 | documen ... .search | tst.js:29:18:29:82 | documen ... , "$1") |
295+
| tst.js:29:18:29:82 | documen ... , "$1") | tst.js:29:9:29:82 | source |
273296
#select
274297
| eslint-escope-build.js:21:16:21:16 | c | eslint-escope-build.js:20:22:20:22 | c | eslint-escope-build.js:21:16:21:16 | c | $@ flows to here and is interpreted as code. | eslint-escope-build.js:20:22:20:22 | c | User-provided value |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/tst.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,15 @@ eval(atob(document.location.hash.substring(1)));
2424

2525
// NOT OK
2626
$('<a>').attr("onclick", location.search.substring(1));
27+
28+
(function test() {
29+
var source = document.location.search.replace(/.*\bfoo\s*=\s*([^;]*).*/, "$1");
30+
31+
new Function(source); // NOT OK
32+
33+
Function(source); // NOT OK
34+
35+
new Function("a", "b", source); // NOT OK
36+
37+
new Function(...["a", "b"], source); // NOT OK
38+
})();

0 commit comments

Comments
 (0)