Skip to content

Commit 769dce5

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #2788 from erik-krogh/CVE42-sink
Approved by esbena
2 parents 5e0d640 + 67cd303 commit 769dce5

File tree

5 files changed

+147
-71
lines changed

5 files changed

+147
-71
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
| Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
4848
| Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. |
4949
| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional ways dangerous paths can be constructed. |
50+
| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional ways of constructing arguments to `cmd.exe` and `/bin/sh`. |
5051

5152
## Changes to libraries
5253

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

Lines changed: 74 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -271,67 +271,82 @@ module TaintTracking {
271271
ArrayFunctionTaintStep() { this = call }
272272

273273
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
274-
// `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are
275-
// `elt` and `ary`; similar for `forEach`
276-
exists(string name, Function f, int i |
277-
(name = "map" or name = "forEach") and
278-
(i = 0 or i = 2) and
279-
call.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and
280-
call.(DataFlow::MethodCallNode).getMethodName() = name and
281-
pred = call.getReceiver() and
282-
succ = DataFlow::parameterNode(f.getParameter(i))
283-
)
284-
or
285-
// `array.map` with tainted return value in callback
286-
exists(DataFlow::FunctionNode f |
287-
call.(DataFlow::MethodCallNode).getMethodName() = "map" and
288-
call.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow
289-
pred = f.getAReturn() and
290-
succ = call
291-
)
292-
or
293-
// `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
294-
exists(string name |
295-
name = "push" or
296-
name = "unshift"
297-
|
298-
pred = call.getAnArgument() and
299-
succ.(DataFlow::SourceNode).getAMethodCall(name) = call
300-
)
301-
or
302-
// `array.push(...e)`, `array.unshift(...e)`: if `e` is tainted, then so is `array`.
303-
exists(string name |
304-
name = "push" or
305-
name = "unshift"
306-
|
307-
pred = call.getASpreadArgument() and
308-
// Make sure we handle reflective calls
309-
succ = call.getReceiver().getALocalSource() and
310-
call.getCalleeName() = name
311-
)
312-
or
313-
// `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
314-
exists(string name | name = "splice" |
315-
pred = call.getArgument(2) and
316-
succ.(DataFlow::SourceNode).getAMethodCall(name) = call
317-
)
318-
or
319-
// `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`.
320-
exists(string name |
321-
name = "pop" or
322-
name = "shift" or
323-
name = "slice" or
324-
name = "splice"
325-
|
326-
call.(DataFlow::MethodCallNode).calls(pred, name) and
327-
succ = call
328-
)
329-
or
330-
// `e = Array.from(x)`: if `x` is tainted, then so is `e`.
331-
call = DataFlow::globalVarRef("Array").getAPropertyRead("from").getACall() and
274+
arrayFunctionTaintStep(pred, succ, call)
275+
}
276+
}
277+
278+
/**
279+
* A taint propagating data flow edge from `pred` to `succ` caused by a call `call` to a builtin array functions.
280+
*/
281+
predicate arrayFunctionTaintStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode call) {
282+
// `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are
283+
// `elt` and `ary`; similar for `forEach`
284+
exists(string name, Function f, int i |
285+
(name = "map" or name = "forEach") and
286+
(i = 0 or i = 2) and
287+
call.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and
288+
call.(DataFlow::MethodCallNode).getMethodName() = name and
289+
pred = call.getReceiver() and
290+
succ = DataFlow::parameterNode(f.getParameter(i))
291+
)
292+
or
293+
// `array.map` with tainted return value in callback
294+
exists(DataFlow::FunctionNode f |
295+
call.(DataFlow::MethodCallNode).getMethodName() = "map" and
296+
call.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow
297+
pred = f.getAReturn() and
298+
succ = call
299+
)
300+
or
301+
// `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
302+
exists(string name |
303+
name = "push" or
304+
name = "unshift"
305+
|
332306
pred = call.getAnArgument() and
307+
succ.(DataFlow::SourceNode).getAMethodCall(name) = call
308+
)
309+
or
310+
// `array.push(...e)`, `array.unshift(...e)`: if `e` is tainted, then so is `array`.
311+
exists(string name |
312+
name = "push" or
313+
name = "unshift"
314+
|
315+
pred = call.getASpreadArgument() and
316+
// Make sure we handle reflective calls
317+
succ = call.getReceiver().getALocalSource() and
318+
call.getCalleeName() = name
319+
)
320+
or
321+
// `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
322+
exists(string name | name = "splice" |
323+
pred = call.getArgument(2) and
324+
succ.(DataFlow::SourceNode).getAMethodCall(name) = call
325+
)
326+
or
327+
// `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`.
328+
exists(string name |
329+
name = "pop" or
330+
name = "shift" or
331+
name = "slice" or
332+
name = "splice"
333+
|
334+
call.(DataFlow::MethodCallNode).calls(pred, name) and
333335
succ = call
334-
}
336+
)
337+
or
338+
// `e = Array.from(x)`: if `x` is tainted, then so is `e`.
339+
call = DataFlow::globalVarRef("Array").getAPropertyRead("from").getACall() and
340+
pred = call.getAnArgument() and
341+
succ = call
342+
or
343+
// `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
344+
call.(DataFlow::MethodCallNode).calls(pred, "concat") and
345+
succ = call
346+
or
347+
call.(DataFlow::MethodCallNode).getMethodName() = "concat" and
348+
succ = call and
349+
pred = call.getAnArgument()
335350
}
336351

337352
/**

‎javascript/ql/src/semmle/javascript/security/dataflow/IndirectCommandArgument.qll‎

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,14 @@ private DataFlow::Node commandArgument(SystemCommandExecution sys, DataFlow::Typ
3030
t.start() and
3131
result = sys.getACommandArgument()
3232
or
33-
exists(DataFlow::TypeBackTracker t2 |
34-
t = t2.smallstep(result, commandArgument(sys, t2))
35-
)
33+
exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, commandArgument(sys, t2)))
34+
}
35+
36+
/**
37+
* Gets a data-flow node whose value ends up being interpreted as the command argument in `sys`.
38+
*/
39+
private DataFlow::Node commandArgument(SystemCommandExecution sys) {
40+
result = commandArgument(sys, DataFlow::TypeBackTracker::end())
3641
}
3742

3843
/**
@@ -43,11 +48,23 @@ private DataFlow::SourceNode argumentList(SystemCommandExecution sys, DataFlow::
4348
t.start() and
4449
result = sys.getArgumentList().getALocalSource()
4550
or
46-
exists(DataFlow::TypeBackTracker t2 |
47-
result = argumentList(sys, t2).backtrack(t2, t)
51+
exists(DataFlow::TypeBackTracker t2, DataFlow::SourceNode pred |
52+
pred = argumentList(sys, t2)
53+
|
54+
result = pred.backtrack(t2, t)
55+
or
56+
t = t2.continue() and
57+
TaintTracking::arrayFunctionTaintStep(result, pred, _)
4858
)
4959
}
5060

61+
/**
62+
* Gets a data-flow node whose value ends up being interpreted as the argument list in `sys`.
63+
*/
64+
private DataFlow::SourceNode argumentList(SystemCommandExecution sys) {
65+
result = argumentList(sys, DataFlow::TypeBackTracker::end())
66+
}
67+
5168
/**
5269
* Holds if `source` contributes to the arguments of an indirect command execution `sys`.
5370
*
@@ -61,15 +78,22 @@ private DataFlow::SourceNode argumentList(SystemCommandExecution sys, DataFlow::
6178
* let args = ["-c", cmd];
6279
* childProcess.spawn(sh, args, cb);
6380
* ```
81+
* or
82+
* ```
83+
* let cmd = getCommand();
84+
* childProcess.spawn("cmd.exe", ["/c"].concat(cmd), cb);
85+
* ```
6486
*/
6587
predicate isIndirectCommandArgument(DataFlow::Node source, SystemCommandExecution sys) {
66-
exists(
67-
DataFlow::ArrayCreationNode args, DataFlow::Node shell, string dashC
68-
|
88+
exists(DataFlow::ArrayCreationNode args, DataFlow::Node shell, string dashC |
6989
shellCmd(shell.asExpr(), dashC) and
70-
shell = commandArgument(sys, DataFlow::TypeBackTracker::end()) and
71-
args = argumentList(sys, DataFlow::TypeBackTracker::end()) and
90+
shell = commandArgument(sys) and
7291
args.getAPropertyWrite().getRhs().mayHaveStringValue(dashC) and
73-
source = args.getAPropertyWrite().getRhs()
92+
args = argumentList(sys) and
93+
(
94+
source = argumentList(sys)
95+
or
96+
source = argumentList(sys).getAPropertyWrite().getRhs()
97+
)
7498
)
7599
}

‎javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,23 @@ nodes
2222
| child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
2323
| child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
2424
| child_process-test.js:25:21:25:23 | cmd |
25+
| child_process-test.js:39:18:39:30 | [ flag, cmd ] |
26+
| child_process-test.js:39:18:39:30 | [ flag, cmd ] |
2527
| child_process-test.js:39:26:39:28 | cmd |
2628
| child_process-test.js:39:26:39:28 | cmd |
2729
| child_process-test.js:43:15:43:17 | cmd |
2830
| child_process-test.js:43:15:43:17 | cmd |
2931
| child_process-test.js:50:15:50:17 | cmd |
3032
| child_process-test.js:50:15:50:17 | cmd |
33+
| child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) |
34+
| child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) |
35+
| child_process-test.js:53:46:53:57 | ["bar", cmd] |
36+
| child_process-test.js:53:46:53:57 | ["bar", cmd] |
37+
| child_process-test.js:53:54:53:56 | cmd |
38+
| child_process-test.js:53:54:53:56 | cmd |
39+
| child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) |
40+
| child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) |
41+
| child_process-test.js:54:46:54:48 | cmd |
3142
| execSeries.js:3:20:3:22 | arr |
3243
| execSeries.js:6:14:6:16 | arr |
3344
| execSeries.js:6:14:6:21 | arr[i++] |
@@ -100,13 +111,24 @@ edges
100111
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:43:15:43:17 | cmd |
101112
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:50:15:50:17 | cmd |
102113
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:50:15:50:17 | cmd |
114+
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:53:54:53:56 | cmd |
115+
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:53:54:53:56 | cmd |
116+
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:54:46:54:48 | cmd |
103117
| child_process-test.js:6:15:6:38 | url.par ... , true) | child_process-test.js:6:15:6:44 | url.par ... ).query |
104118
| child_process-test.js:6:15:6:44 | url.par ... ).query | child_process-test.js:6:15:6:49 | url.par ... ry.path |
105119
| child_process-test.js:6:15:6:49 | url.par ... ry.path | child_process-test.js:6:9:6:49 | cmd |
106120
| child_process-test.js:6:25:6:31 | req.url | child_process-test.js:6:15:6:38 | url.par ... , true) |
107121
| child_process-test.js:6:25:6:31 | req.url | child_process-test.js:6:15:6:38 | url.par ... , true) |
108122
| child_process-test.js:25:21:25:23 | cmd | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
109123
| child_process-test.js:25:21:25:23 | cmd | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
124+
| child_process-test.js:39:26:39:28 | cmd | child_process-test.js:39:18:39:30 | [ flag, cmd ] |
125+
| child_process-test.js:39:26:39:28 | cmd | child_process-test.js:39:18:39:30 | [ flag, cmd ] |
126+
| child_process-test.js:53:46:53:57 | ["bar", cmd] | child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) |
127+
| child_process-test.js:53:46:53:57 | ["bar", cmd] | child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) |
128+
| child_process-test.js:53:54:53:56 | cmd | child_process-test.js:53:46:53:57 | ["bar", cmd] |
129+
| child_process-test.js:53:54:53:56 | cmd | child_process-test.js:53:46:53:57 | ["bar", cmd] |
130+
| child_process-test.js:54:46:54:48 | cmd | child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) |
131+
| child_process-test.js:54:46:54:48 | cmd | child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) |
110132
| execSeries.js:3:20:3:22 | arr | execSeries.js:6:14:6:16 | arr |
111133
| execSeries.js:6:14:6:16 | arr | execSeries.js:6:14:6:21 | arr[i++] |
112134
| execSeries.js:6:14:6:21 | arr[i++] | execSeries.js:14:24:14:30 | command |
@@ -165,10 +187,16 @@ edges
165187
| child_process-test.js:22:18:22:20 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:22:18:22:20 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
166188
| child_process-test.js:23:13:23:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:23:13:23:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
167189
| child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
190+
| child_process-test.js:39:5:39:31 | cp.spaw ... cmd ]) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:39:18:39:30 | [ flag, cmd ] | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
168191
| child_process-test.js:39:5:39:31 | cp.spaw ... cmd ]) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:39:26:39:28 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
169192
| child_process-test.js:44:5:44:34 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
170193
| child_process-test.js:51:5:51:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
171-
| child_process-test.js:56:3:56:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
194+
| child_process-test.js:53:5:53:59 | cp.spaw ... cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
195+
| child_process-test.js:53:5:53:59 | cp.spaw ... cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:46:53:57 | ["bar", cmd] | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
196+
| child_process-test.js:53:5:53:59 | cp.spaw ... cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:54:53:56 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
197+
| child_process-test.js:54:5:54:50 | cp.spaw ... t(cmd)) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
198+
| child_process-test.js:59:5:59:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
199+
| child_process-test.js:64:3:64:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
172200
| execSeries.js:14:41:14:47 | command | execSeries.js:18:34:18:40 | req.url | execSeries.js:14:41:14:47 | command | This command depends on $@. | execSeries.js:18:34:18:40 | req.url | a user-provided value |
173201
| other.js:7:33:7:35 | cmd | other.js:5:25:5:31 | req.url | other.js:7:33:7:35 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
174202
| other.js:8:28:8:30 | cmd | other.js:5:25:5:31 | req.url | other.js:8:28:8:30 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-078/child_process-test.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ var server = http.createServer(function(req, res) {
5050
args[1] = cmd;
5151
cp.execFile(`/bin` + "/bash", args); // NOT OK
5252

53+
cp.spawn('cmd.exe', ['/C', 'foo'].concat(["bar", cmd])); // NOT OK
54+
cp.spawn('cmd.exe', ['/C', 'foo'].concat(cmd)); // NOT OK
55+
56+
let myArgs = [];
57+
myArgs.push(`-` + "c");
58+
myArgs.push(cmd);
59+
cp.execFile(`/bin` + "/bash", args); // NOT OK
60+
5361
});
5462

5563
function run(cmd, args) {

0 commit comments

Comments
 (0)