Skip to content

Commit bf4a324

Browse files
author
Esben Sparre Andreasen
committed
JS: add query js/indirect-command-line-injection
1 parent 3739587 commit bf4a324

15 files changed

+459
-32
lines changed

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,10 @@
1717

1818
## New queries
1919

20-
| **Query** | **Tags** | **Purpose** |
21-
|-----------|----------|-------------|
22-
| | | |
20+
| **Query** | **Tags** | **Purpose** |
21+
|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
22+
| Indirect uncontrolled command line (`js/indirect-command-line-injection`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights command-line invocations that may indirectly introduce a command-line injection vulnerability elsewhere, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are not shown on LGTM by default. |
23+
2324

2425
## Changes to existing queries
2526

‎javascript/config/suites/javascript/security‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
+ semmlecode-javascript-queries/Security/CWE-022/TaintedPath.ql: /Security/CWE/CWE-022
88
+ semmlecode-javascript-queries/Security/CWE-022/ZipSlip.ql: /Security/CWE/CWE-022
99
+ semmlecode-javascript-queries/Security/CWE-078/CommandInjection.ql: /Security/CWE/CWE-078
10+
+ semmlecode-javascript-queries/Security/CWE-078/IndirectCommandInjection.ql: /Security/CWE/CWE-078
1011
+ semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079
1112
+ semmlecode-javascript-queries/Security/CWE-079/StoredXss.ql: /Security/CWE/CWE-079
1213
+ semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079

‎javascript/ql/src/Security/CWE-078/IndirectCommandInjection.qhelp‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
7+
<p>
8+
9+
Forwarding command-line arguments to
10+
<code>child_process.exec</code> or some other library routine that
11+
executes a system command within a shell can change the meaning of the
12+
command unexpectedly due to unescaped special characters.
13+
14+
</p>
15+
16+
<p>
17+
18+
When the forwarded command-line arguments come from a parent
19+
process that has not escaped the special characters in the arguments,
20+
then the parent process may indirectly be vulnerable to command-line
21+
injection since the special characters are evaluated unexpectedly.
22+
23+
</p>
24+
25+
</overview>
26+
<recommendation>
27+
28+
<p>
29+
30+
If possible, use hard-coded string literals to specify the
31+
command to run or library to load. Instead of forwarding the
32+
command-line arguments to the process, examine the command-line
33+
arguments and then choose among hard-coded string literals.
34+
35+
</p>
36+
37+
<p>
38+
39+
If the applicable libraries or commands cannot be determined
40+
at compile time, then add code to verify that each forwarded
41+
command-line argument is properly escaped before using it.
42+
43+
</p>
44+
45+
<p>
46+
47+
If the forwarded command-line arguments are part of the
48+
arguments of the system command, prefer a library routine that handles
49+
the arguments as an array of strings rather than a single concatenated
50+
string. This prevents the unexpected evaluation of special characters.
51+
52+
</p>
53+
54+
</recommendation>
55+
<example>
56+
57+
<p>
58+
59+
The following wrapper script example executes another
60+
JavaScript file in a child process and forwards some command-line
61+
arguments. This is problematic because the special characters in the
62+
command-line arguments may change the meaning of the child process invocation
63+
unexpectedly. For instance, if one of the command-line arguments is
64+
<code>"dollar$separated$name"</code>, then the child process will
65+
substitute the two environment variables <code>$separated</code> and
66+
<code>$name</code> before invoking <code>node</code>.
67+
68+
</p>
69+
70+
<sample src="examples/indirect-command-injection.js" />
71+
72+
<p>
73+
74+
If another program uses <code>child_process.execFile</code> to
75+
invoke the above wrapper script with input from a remote user, then
76+
there may be a command-line injection vulnerability.
77+
78+
This may be surprising, since a command-line invocation with
79+
<code>child_process.execFile</code> is generally considered safe. But
80+
in this case, the remote user input is simply forwarded to the
81+
problematic <code>process.exec</code> call in the wrapper script.
82+
83+
</p>
84+
85+
<p>
86+
87+
To guard against this, use an API that does not perform environment
88+
variable substitution, such as <code>child_process.execFile</code>:
89+
90+
</p>
91+
92+
<sample src="examples/indirect-command-injection_fixed.js" />
93+
94+
</example>
95+
96+
<references>
97+
98+
<li>
99+
OWASP:
100+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
101+
</li>
102+
103+
</references>
104+
105+
</qhelp>

‎javascript/ql/src/Security/CWE-078/IndirectCommandInjection.ql‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* @name Indirect uncontrolled command line
3+
* @description Forwarding command-line arguments to a child process
4+
* executed within a shell may indirectly introduce
5+
* command-line injection vulnerabilities.
6+
* @kind path-problem
7+
* @problem.severity warning
8+
* @precision medium
9+
* @id js/indirect-command-line-injection
10+
* @tags correctness
11+
* security
12+
* external/cwe/cwe-078
13+
* external/cwe/cwe-088
14+
*/
15+
16+
import javascript
17+
import DataFlow::PathGraph
18+
import semmle.javascript.security.dataflow.IndirectCommandInjection::IndirectCommandInjection
19+
20+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight
21+
where
22+
cfg.hasFlowPath(source, sink) and
23+
if cfg.isSinkWithHighlight(sink.getNode(), _)
24+
then cfg.isSinkWithHighlight(sink.getNode(), highlight)
25+
else highlight = sink.getNode()
26+
select highlight, source, sink, "This command depends on an unsanitized $@.", source.getNode(),
27+
"command-line argument"

‎javascript/ql/src/Security/CWE-078/examples/indirect-command-injection.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
var cp = require("child_process");
2+
3+
const args = process.argv.slice(2);
4+
const script = path.join(__dirname, 'bin', 'main.js');
5+
cp.execSync(`node ${script} ${args.join(' ')}"`); // BAD

‎javascript/ql/src/Security/CWE-078/examples/indirect-command-injection_fixed.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
var cp = require("child_process");
2+
3+
const args = process.argv.slice(2);
4+
const script = path.join(__dirname, 'bin', 'main.js');
5+
cp.execFileSync('node', [script].concat(args)); // GOOD

‎javascript/ql/src/semmle/javascript/heuristics/AdditionalSources.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ private class RemoteFlowPassword extends HeuristicSource, RemoteFlowSource {
2323
}
2424

2525
/**
26-
* A use of `JSON.stringify`, viewed as a source for command line injections
26+
* A use of `JSON.stringify`, viewed as a source for command-line injections
2727
* since it does not properly escape single quotes and dollar symbols.
2828
*/
2929
private class JSONStringifyAsCommandInjectionSource extends HeuristicSource,

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

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import javascript
1111

1212
module CommandInjection {
1313
import CommandInjectionCustomizations::CommandInjection
14+
import IndirectCommandArgument
1415

1516
/**
1617
* A taint-tracking configuration for reasoning about command-injection vulnerabilities.
@@ -27,7 +28,7 @@ module CommandInjection {
2728
predicate isSinkWithHighlight(DataFlow::Node sink, DataFlow::Node highlight) {
2829
sink instanceof Sink and highlight = sink
2930
or
30-
indirectCommandInjection(sink, highlight)
31+
isIndirectCommandArgument(sink, highlight)
3132
}
3233

3334
override predicate isSink(DataFlow::Node sink) { isSinkWithHighlight(sink, _) }
@@ -74,30 +75,4 @@ module CommandInjection {
7475
(arg = "/c" or arg = "/C")
7576
)
7677
}
77-
78-
/**
79-
* An indirect command execution through `sh -c` or `cmd.exe /c`.
80-
*
81-
* For example, we may have a call to `childProcess.spawn` like this:
82-
*
83-
* ```
84-
* let sh = "sh";
85-
* let args = ["-c", cmd];
86-
* childProcess.spawn(sh, args, cb);
87-
* ```
88-
*
89-
* Here, the indirect sink is `cmd`. For reporting purposes, however,
90-
* we want to report the `spawn` call as the sink, so we bind it to `sys`.
91-
*/
92-
private predicate indirectCommandInjection(DataFlow::Node sink, SystemCommandExecution sys) {
93-
exists(
94-
ArgumentListTracking cfg, DataFlow::ArrayCreationNode args, ConstantString shell, string dashC
95-
|
96-
shellCmd(shell, dashC) and
97-
cfg.hasFlow(DataFlow::valueNode(shell), sys.getACommandArgument()) and
98-
cfg.hasFlow(args, sys.getArgumentList()) and
99-
args.getAPropertyWrite().getRhs().mayHaveStringValue(dashC) and
100-
sink = args.getAPropertyWrite().getRhs()
101-
)
102-
}
10378
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,5 +34,4 @@ module CommandInjection {
3434
class SystemCommandExecutionSink extends Sink, DataFlow::ValueNode {
3535
SystemCommandExecutionSink() { this = any(SystemCommandExecution sys).getACommandArgument() }
3636
}
37-
3837
}

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/**
2+
* Provides predicates for reasoning about indirect command arguments.
3+
*/
4+
5+
import javascript
6+
7+
/**
8+
* Holds if `shell arg <cmd>` runs `<cmd>` as a shell command.
9+
*
10+
* That is, either `shell` is a Unix shell (`sh` or similar) and
11+
* `arg` is `"-c"`, or `shell` is `cmd.exe` and `arg` is `"/c"`.
12+
*/
13+
private predicate shellCmd(ConstantString shell, string arg) {
14+
exists(string s | s = shell.getStringValue() |
15+
(s = "sh" or s = "bash" or s = "/bin/sh" or s = "/bin/bash") and
16+
arg = "-c"
17+
)
18+
or
19+
exists(string s | s = shell.getStringValue().toLowerCase() |
20+
(s = "cmd" or s = "cmd.exe") and
21+
(arg = "/c" or arg = "/C")
22+
)
23+
}
24+
25+
/**
26+
* Data flow configuration for tracking string literals that look like they
27+
* may refer to an operating-system shell, and array literals that may end up being
28+
* interpreted as argument lists for system commands.
29+
*/
30+
private class ArgumentListTracking extends DataFlow::Configuration {
31+
ArgumentListTracking() { this = "ArgumentListTracking" }
32+
33+
override predicate isSource(DataFlow::Node nd) {
34+
nd instanceof DataFlow::ArrayCreationNode
35+
or
36+
exists(ConstantString shell | shellCmd(shell, _) | nd = DataFlow::valueNode(shell))
37+
}
38+
39+
override predicate isSink(DataFlow::Node nd) {
40+
exists(SystemCommandExecution sys |
41+
nd = sys.getACommandArgument() or
42+
nd = sys.getArgumentList()
43+
)
44+
}
45+
}
46+
47+
/**
48+
* Holds if `source` contributes to the arguments of an indirect command execution `sys`.
49+
*
50+
* An indirect command execution is a system execution command that starts with `sh -c`, `cmd.exe /c`, or similar.
51+
*
52+
* For example, `getCommand()` is `source`, and the call to `childProcess.spawn` is `sys` in the following example:
53+
*
54+
* ```
55+
* let cmd = getCommand();
56+
* let sh = "sh";
57+
* let args = ["-c", cmd];
58+
* childProcess.spawn(sh, args, cb);
59+
* ```
60+
*/
61+
predicate isIndirectCommandArgument(DataFlow::Node source, SystemCommandExecution sys) {
62+
exists(
63+
ArgumentListTracking cfg, DataFlow::ArrayCreationNode args, ConstantString shell, string dashC
64+
|
65+
shellCmd(shell, dashC) and
66+
cfg.hasFlow(DataFlow::valueNode(shell), sys.getACommandArgument()) and
67+
cfg.hasFlow(args, sys.getArgumentList()) and
68+
args.getAPropertyWrite().getRhs().mayHaveStringValue(dashC) and
69+
source = args.getAPropertyWrite().getRhs()
70+
)
71+
}

0 commit comments

Comments
 (0)