Skip to content

Commit 5a983cb

Browse files
author
Esben Sparre Andreasen
committed
JS: add query js/shell-command-injection-from-environment
1 parent 80a32ae commit 5a983cb

13 files changed

+294
-0
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
1818
| Loop bound injection (`js/loop-bound-injection`) | security, external/cwe/cwe-834 | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
1919
| Suspicious method name (`js/suspicious-method-name-declaration`) | correctness, typescript, methods | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
20+
| Shell command built from environment values (`js/shell-command-injection-from-environment`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights shell commands that may change behavior inadvertently depending on the execution environment, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are shown on LGTM by default.|
2021
| Use of returnless function (`js/use-of-returnless-function`) | maintainability, correctness | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |
2122
| Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
2223

‎javascript/config/suites/javascript/security‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
+ semmlecode-javascript-queries/Security/CWE-022/ZipSlip.ql: /Security/CWE/CWE-022
1010
+ semmlecode-javascript-queries/Security/CWE-078/CommandInjection.ql: /Security/CWE/CWE-078
1111
+ semmlecode-javascript-queries/Security/CWE-078/IndirectCommandInjection.ql: /Security/CWE/CWE-078
12+
+ semmlecode-javascript-queries/Security/CWE-078/ShellCommandInjectionFromEnvironment: /Security/CWE/CWE-078
1213
+ semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079
1314
+ semmlecode-javascript-queries/Security/CWE-079/StoredXss.ql: /Security/CWE/CWE-079
1415
+ semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079

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

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
8+
Dynamically constructing a shell command with values from the
9+
local environment, such as file paths, may inadvertently
10+
change the meaning of the shell command.
11+
12+
Such changes can occur when an environment value contains
13+
characters that the shell interprets in a special way, for instance
14+
quotes and spaces.
15+
16+
This can result in the shell command misbehaving, or even
17+
allowing a malicious user to execute arbitrary commands on the system.
18+
19+
</p>
20+
21+
22+
</overview>
23+
<recommendation>
24+
25+
<p>
26+
27+
If possible, use hard-coded string literals to specify the
28+
shell command to run, and provide the dynamic arguments to the shell
29+
command separately to avoid interpretation by the shell.
30+
31+
</p>
32+
33+
<p>
34+
35+
Alternatively, if the shell command must be constructed
36+
dynamically, then add code to ensure that special characters in
37+
environment values do not alter the shell command unexpectedly.
38+
39+
</p>
40+
41+
</recommendation>
42+
<example>
43+
44+
<p>
45+
46+
The following example shows a dynamically constructed shell
47+
command that recursively removes a temporary directory that is located
48+
next to the currently executing JavaScript file. Such utilities are
49+
often found in custom build scripts.
50+
51+
</p>
52+
53+
<sample src="examples/shell-command-injection-from-environment.js" />
54+
55+
<p>
56+
57+
The shell command will, however, fail to work as intended if the
58+
absolute path of the script's directory contains spaces. In that
59+
case, the shell command will interpret the absolute path as multiple
60+
paths, instead of a single path.
61+
</p>
62+
63+
<p>
64+
65+
For instance, if the absolute path of
66+
the temporary directory is <code>/home/username/important
67+
project/temp</code>, then the shell command will recursively delete
68+
<code>/home/username/important</code> and <code>project/temp</code>,
69+
where the latter path gets resolved relative to the working directory
70+
of the JavaScript process.
71+
72+
</p>
73+
74+
75+
<p>
76+
77+
Even worse, although less likely, a malicious user could
78+
provide the path <code>/home/username/; cat /etc/passwd #/important
79+
project/temp</code> in order to execute the command <code>cat
80+
/etc/passwd</code>.
81+
82+
</p>
83+
84+
<p>
85+
86+
To avoid such potentially catastrophic behaviors, provide the
87+
directory as an argument that does not get interpreted by a
88+
shell:
89+
90+
</p>
91+
92+
<sample src="examples/shell-command-injection-from-environment_fixed.js" />
93+
94+
</example>
95+
<references>
96+
97+
<li>
98+
OWASP:
99+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
100+
</li>
101+
102+
</references>
103+
</qhelp>

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* @name Shell command built from environment values
3+
* @description Building a shell command string with values from the enclosing
4+
* environment may cause subtle bugs or vulnerabilities.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @precision high
8+
* @id js/shell-command-injection-from-environment
9+
* @tags correctness
10+
* security
11+
* external/cwe/cwe-078
12+
* external/cwe/cwe-088
13+
*/
14+
15+
import javascript
16+
import DataFlow::PathGraph
17+
import semmle.javascript.security.dataflow.ShellCommandInjectionFromEnvironment::ShellCommandInjectionFromEnvironment
18+
19+
from
20+
Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight,
21+
Source sourceNode
22+
where
23+
sourceNode = source.getNode() and
24+
cfg.hasFlowPath(source, sink) and
25+
if cfg.isSinkWithHighlight(sink.getNode(), _)
26+
then cfg.isSinkWithHighlight(sink.getNode(), highlight)
27+
else highlight = sink.getNode()
28+
select highlight, source, sink, "This shell command depends on an uncontrolled $@.", sourceNode,
29+
sourceNode.getSourceType()

‎javascript/ql/src/Security/CWE-078/examples/shell-command-injection-from-environment.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
var cp = require("child_process"),
2+
path = require("path");
3+
function cleanupTemp() {
4+
let cmd = "rm -rf " + path.join(__dirname, "temp");
5+
cp.execSync(cmd); // BAD
6+
}

‎javascript/ql/src/Security/CWE-078/examples/shell-command-injection-from-environment_fixed.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
var cp = require("child_process"),
2+
path = require("path");
3+
function cleanupTemp() {
4+
let cmd = "rm",
5+
args = ["-rf", path.join(__dirname, "temp")];
6+
cp.execFileSync(cmd, args); // GOOD
7+
}

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about
3+
* command-injection vulnerabilities (CWE-078).
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `ShellCommandInjectionFromEnvironment::Configuration` is needed, otherwise
7+
* `ShellCommandInjectionFromEnvironmentCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
12+
module ShellCommandInjectionFromEnvironment {
13+
import ShellCommandInjectionFromEnvironmentCustomizations::ShellCommandInjectionFromEnvironment
14+
import IndirectCommandArgument
15+
16+
/**
17+
* A taint-tracking configuration for reasoning about command-injection vulnerabilities.
18+
*/
19+
class Configuration extends TaintTracking::Configuration {
20+
Configuration() { this = "ShellCommandInjectionFromEnvironment" }
21+
22+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
23+
24+
predicate isSinkWithHighlight(DataFlow::Node sink, DataFlow::Node highlight) {
25+
sink instanceof Sink and highlight = sink
26+
or
27+
isIndirectCommandArgument(sink, highlight)
28+
}
29+
30+
override predicate isSink(DataFlow::Node sink) { isSinkWithHighlight(sink, _) }
31+
32+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
33+
}
34+
}

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* command-injection vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
import javascript
8+
import semmle.javascript.security.dataflow.TaintedPathCustomizations
9+
10+
module ShellCommandInjectionFromEnvironment {
11+
/**
12+
* A data flow source for command-injection vulnerabilities.
13+
*/
14+
abstract class Source extends DataFlow::Node {
15+
/** Gets a string that describes the type of this data flow source. */
16+
abstract string getSourceType();
17+
}
18+
19+
/**
20+
* A data flow sink for command-injection vulnerabilities.
21+
*/
22+
abstract class Sink extends DataFlow::Node { }
23+
24+
/**
25+
* A sanitizer for command-injection vulnerabilities.
26+
*/
27+
abstract class Sanitizer extends DataFlow::Node { }
28+
29+
/** An file name from the local file system, considered as a flow source for command injection. */
30+
class FileNameSourceAsSource extends Source {
31+
FileNameSourceAsSource() { this instanceof FileNameSource }
32+
33+
override string getSourceType() { result = "file name" }
34+
}
35+
36+
/** An absolute path from the local file system, considered as a flow source for command injection. */
37+
class AbsolutePathSource extends Source {
38+
AbsolutePathSource() {
39+
exists(ModuleScope ms | this.asExpr() = ms.getVariable("__dirname").getAnAccess())
40+
or
41+
exists(DataFlow::SourceNode process | process = NodeJSLib::process() |
42+
this = process.getAPropertyRead("execPath") or
43+
this = process.getAMemberCall("cwd")
44+
)
45+
or
46+
this = any(TaintedPath::ResolvingPathCall c).getOutput()
47+
}
48+
49+
override string getSourceType() { result = "absolute path" }
50+
}
51+
52+
/**
53+
* A shell command argument.
54+
*/
55+
class ShellCommandSink extends Sink, DataFlow::ValueNode {
56+
ShellCommandSink() { any(SystemCommandExecution sys).isShellInterpreted(this) }
57+
}
58+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ nodes
6969
| other.js:19:36:19:38 | cmd |
7070
| third-party-command-injection.js:5:20:5:26 | command |
7171
| third-party-command-injection.js:6:21:6:27 | command |
72+
| tst_shell-command-injection-from-environment.js:4:25:4:61 | ['-rf', ... temp")] |
7273
edges
7374
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
7475
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:18:17:18:19 | cmd |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ nodes
5454
| command-line-parameter-command-injection.js:27:14:27:57 | `node $ ... ption"` |
5555
| command-line-parameter-command-injection.js:27:32:27:35 | args |
5656
| command-line-parameter-command-injection.js:27:32:27:45 | args.join(' ') |
57+
| tst_shell-command-injection-from-environment.js:4:25:4:61 | ['-rf', ... temp")] |
5758
edges
5859
| child_process-test.js:36:7:36:20 | sh | child_process-test.js:39:5:39:5 | sh |
5960
| child_process-test.js:36:7:36:20 | sh | child_process-test.js:39:14:39:15 | sh |

0 commit comments

Comments
 (0)