Skip to content

Commit fc21128

Browse files
erik-krogherik-krogh
committed
add second-order-command-injection query
1 parent 0a7e797 commit fc21128

File tree

11 files changed

+442
-0
lines changed

11 files changed

+442
-0
lines changed

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

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* second order command injection, as well as
4+
* extension points for adding your own.
5+
*/
6+
7+
import javascript
8+
private import semmle.javascript.PackageExports as Exports
9+
private import semmle.javascript.security.TaintedObjectCustomizations
10+
11+
/** Classes and predicates for reasoning about second order command injection. */
12+
module SecondOrderCommandInjection {
13+
/** A shell command that allows for second order command injection. */
14+
private class VulnerableCommand extends string {
15+
VulnerableCommand() { this = ["git", "hg"] }
16+
17+
/**
18+
* Gets a vulnerable subcommand of this command.
19+
* E.g. `git` has `clone` and `pull` as vulnerable subcommands.
20+
* And every command of `hg` is vulnerable due to `--config=alias.<alias>=<command>`.
21+
*/
22+
bindingset[result]
23+
string getAVulnerableSubCommand() {
24+
this = "git" and result = ["clone", "ls-remote", "fetch", "pull"]
25+
or
26+
this = "hg" and exists(result)
27+
}
28+
}
29+
30+
/** A source for second order command injection. */
31+
abstract class Source extends DataFlow::Node {
32+
/** Gets a string that describes the source. For use in the alert message. */
33+
abstract string describe();
34+
35+
/** Gets a label for which this is a source. */
36+
abstract DataFlow::FlowLabel getALabel();
37+
}
38+
39+
/** A parameter of an exported function, seen as a source for second order command injection. */
40+
class ExternalInputSource extends Source {
41+
ExternalInputSource() { this = Exports::getALibraryInputParameter() }
42+
43+
override string describe() { result = "library input" }
44+
45+
override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() or result.isTaint() }
46+
}
47+
48+
/** A source of remote flow, seen as a source for second order command injection. */
49+
class RemoteFlowAsSource extends Source instanceof RemoteFlowSource {
50+
override string describe() { result = "a user-provided value" }
51+
52+
override DataFlow::FlowLabel getALabel() { result.isTaint() }
53+
}
54+
55+
private class TaintedObjectSourceAsSource extends Source instanceof TaintedObject::Source {
56+
override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() }
57+
58+
override string describe() { result = "a user-provided value" }
59+
}
60+
61+
/** A sanitizer for second order command injection. */
62+
abstract class Sanitizer extends DataFlow::Node { }
63+
64+
/** A sink for second order command injection. */
65+
abstract class Sink extends DataFlow::Node {
66+
/** Gets a label for which this is a sink. */
67+
abstract DataFlow::FlowLabel getALabel();
68+
}
69+
70+
/** A call that (indirectly) executes a shell command with a list of arguments. */
71+
abstract private class CommandExecutingCall extends DataFlow::CallNode {
72+
/** Gets the dataflow node representing the command to execute. */
73+
abstract DataFlow::Node getCommandArg();
74+
75+
/** Gets the dataflow node representing the arguments to the command. */
76+
abstract DataFlow::Node getArgList();
77+
}
78+
79+
/** A `SystemCommandExecution` seen as a command executing call. */
80+
private class SystemExecAsCmdCall extends CommandExecutingCall instanceof SystemCommandExecution {
81+
override DataFlow::Node getCommandArg() {
82+
result = SystemCommandExecution.super.getACommandArgument()
83+
}
84+
85+
override DataFlow::Node getArgList() { result = SystemCommandExecution.super.getArgumentList() }
86+
}
87+
88+
/** A function whose parameters is directly used a command and argument list for a shell invocation. */
89+
private class IndirectCmdFunc extends DataFlow::FunctionNode {
90+
int cmdIndex;
91+
int argIndex;
92+
93+
IndirectCmdFunc() {
94+
exists(CommandExecutingCall call |
95+
this.getParameter(cmdIndex).flowsTo(call.getCommandArg()) and
96+
this.getParameter(argIndex).flowsTo(call.getArgList())
97+
)
98+
}
99+
100+
/** Gets the parameter index that indicates the command to be executed. */
101+
int getCmdIndex() { result = cmdIndex }
102+
103+
/** Gets the parameter index that indicates the argument list to be passed to the command. */
104+
int getArgIndex() { result = argIndex }
105+
}
106+
107+
/** A call to a function that eventually executes a shell command with a list of arguments. */
108+
private class IndirectExecCall extends DataFlow::CallNode, CommandExecutingCall {
109+
IndirectCmdFunc func;
110+
111+
IndirectExecCall() { this.getACallee() = func.getFunction() }
112+
113+
override DataFlow::Node getCommandArg() { result = this.getArgument(func.getCmdIndex()) }
114+
115+
override DataFlow::Node getArgList() { result = this.getArgument(func.getArgIndex()) }
116+
}
117+
118+
/** Gets a dataflow node that ends up being used as an argument list to an invocation of `git` or `hg`. */
119+
private DataFlow::SourceNode usedAsVersionControlArgs(
120+
DataFlow::TypeBackTracker t, DataFlow::Node argList, VulnerableCommand cmd
121+
) {
122+
t.start() and
123+
exists(CommandExecutingCall exec | exec.getCommandArg().mayHaveStringValue(cmd) |
124+
exec.getArgList() = argList and
125+
result = argList.getALocalSource()
126+
)
127+
or
128+
exists(DataFlow::TypeBackTracker t2 |
129+
result = usedAsVersionControlArgs(t2, argList, cmd).backtrack(t2, t)
130+
)
131+
}
132+
133+
/** An argument to an invocation of `git`/`hg` that can cause second order command injection. */
134+
class ArgSink extends Sink {
135+
ArgSink() {
136+
exists(DataFlow::ArrayCreationNode args, VulnerableCommand cmd |
137+
args = usedAsVersionControlArgs(DataFlow::TypeBackTracker::end(), _, cmd)
138+
|
139+
this = [args.getAnElement(), args.getASpreadArgument()] and
140+
args.getElement(0).mayHaveStringValue(cmd.getAVulnerableSubCommand()) and
141+
// not an "--" argument (even if it's earlier, then we assume it's on purpose)
142+
not args.getElement(_).mayHaveStringValue("--")
143+
)
144+
}
145+
146+
override DataFlow::FlowLabel getALabel() { result.isTaint() }
147+
}
148+
149+
/**
150+
* An arguments array given to an invocation of `git` or `hg` that can cause second order command injection.
151+
*/
152+
class ArgsArraySink extends Sink {
153+
ArgsArraySink() {
154+
exists(SystemExecAsCmdCall exec |
155+
exec.getCommandArg().mayHaveStringValue(any(VulnerableCommand cmd))
156+
|
157+
this = exec.getArgList()
158+
)
159+
}
160+
161+
// only vulnerable if an attacker controls the entire array
162+
override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() }
163+
}
164+
165+
/**
166+
* A sanitizer that blocks flow when a string is tested to start with a certain prefix.
167+
*/
168+
class PrefixStringSanitizer extends TaintTracking::SanitizerGuardNode instanceof StringOps::StartsWith {
169+
override predicate sanitizes(boolean outcome, Expr e) {
170+
e = super.getBaseString().asExpr() and
171+
outcome = super.getPolarity()
172+
}
173+
}
174+
175+
/**
176+
* A sanitizer that blocks flow when a string does not start with "--"
177+
*/
178+
class DoubleDashSanitizer extends TaintTracking::SanitizerGuardNode instanceof StringOps::StartsWith {
179+
DoubleDashSanitizer() { super.getSubstring().mayHaveStringValue("--") }
180+
181+
override predicate sanitizes(boolean outcome, Expr e) {
182+
e = super.getBaseString().asExpr() and
183+
outcome = super.getPolarity().booleanNot()
184+
}
185+
}
186+
187+
/** A call to path.relative which sanitizes the taint. */
188+
class PathRelativeSanitizer extends Sanitizer {
189+
PathRelativeSanitizer() { this = NodeJSLib::Path::moduleMember("relative").getACall() }
190+
}
191+
}

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about
3+
* second order command-injection vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `SecondOrderCommandInjection::Configuration` is needed, otherwise
7+
* `SecondOrderCommandInjectionCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
import SecondOrderCommandInjectionCustomizations::SecondOrderCommandInjection
12+
private import semmle.javascript.security.TaintedObject
13+
14+
/**
15+
* A taint-tracking configuration for reasoning about command-injection vulnerabilities.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "SecondOrderCommandInjection" }
19+
20+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
21+
source.(Source).getALabel() = label
22+
}
23+
24+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
25+
sink.(Sink).getALabel() = label
26+
}
27+
28+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
29+
30+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
31+
guard instanceof PrefixStringSanitizer or
32+
guard instanceof DoubleDashSanitizer or
33+
guard instanceof TaintedObject::SanitizerGuard
34+
}
35+
36+
override predicate isAdditionalFlowStep(
37+
DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
38+
) {
39+
TaintedObject::step(src, trg, inlbl, outlbl)
40+
}
41+
}

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Some shell commands, like <code>git ls-remote</code>, can execute
8+
arbitrary commands if a user provides a malicious URL that can start with
9+
<code>--upload-pack</code>. This can be used to execute arbitrary code on
10+
the server.
11+
</p>
12+
13+
</overview>
14+
15+
<recommendation>
16+
17+
<p>
18+
Sanitize user input before passing it to the shell command by for example
19+
ensuring that URLs are valid and do not contain malicious commands.
20+
</p>
21+
22+
</recommendation>
23+
<example>
24+
25+
<p>
26+
The following example shows code that executes <code>git ls-remote</code> on a
27+
URL that can be controlled by a malicious user.
28+
</p>
29+
30+
<sample src="examples/second-order-command-injection.js" />
31+
32+
<p>
33+
The problem has been fixed in the below where the URL is validated before
34+
being passed to the shell command.
35+
</p>
36+
37+
<sample src="examples/second-order-command-injection-fixed.js" />
38+
39+
</example>
40+
<references>
41+
<li>Max Justicz: <a href="https://justi.cz/security/2021/04/20/cocoapods-rce.html">Hacking 3,000,000 apps at once through CocoaPods</a>.</li>
42+
<li>Git: <a href="https://git-scm.com/docs/git-ls-remote/2.22.0#Documentation/git-ls-remote.txt---upload-packltexecgt">Git - git-ls-remote Documentation</a>.</li>
43+
<li>OWASP:<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>
44+
45+
</references>
46+
</qhelp>

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* @name Second order command injection
3+
* @description Some shell programs allow arbitrary command execution via their command line arguments.
4+
* This is a second order command injection vulnerability.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 7.0
8+
* @precision high
9+
* @id js/second-order-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.SecondOrderCommandInjectionQuery
19+
20+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
21+
where cfg.hasFlowPath(source, sink)
22+
select sink.getNode(), source, sink,
23+
"Command line argument that allows for arbitrary command execution depends on $@.",
24+
source.getNode(), source.getNode().(Source).describe()

‎javascript/ql/src/Security/CWE-078/examples/second-order-command-injection-fixed.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
const express = require("express");
2+
const app = express();
3+
4+
const cp = require("child_process");
5+
6+
app.get("/ls-remote", (req, res) => {
7+
const remote = req.query.remote;
8+
if (!(remote.startsWith("git@") || remote.startsWith("https://"))) {
9+
throw new Error("Invalid remote: " + remote);
10+
}
11+
cp.execFile("git", ["ls-remote", remote]); // OK
12+
});

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const express = require("express");
2+
const app = express();
3+
4+
const cp = require("child_process");
5+
6+
app.get("/ls-remote", (req, res) => {
7+
const remote = req.query.remote;
8+
cp.execFile("git", ["ls-remote", remote]); // NOT OK
9+
});

‎javascript/ql/src/change-notes/2022-09-05-second-order-command-injection.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `js/second-order-command-line-injection`, to detect shell
5+
commands that may execute arbitrary code when the user has control over
6+
the arguments to a command-line program.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import semmle.javascript.security.dataflow.CommandInjection
44
import semmle.javascript.security.dataflow.IndirectCommandInjection
55
import semmle.javascript.security.dataflow.ShellCommandInjectionFromEnvironment
66
import semmle.javascript.security.dataflow.UnsafeShellCommandConstruction
7+
import semmle.javascript.security.dataflow.SecondOrderCommandInjectionQuery
78

89
class CommandInjectionConsistency extends ConsistencyConfiguration {
910
CommandInjectionConsistency() { this = "ComandInjection" }

0 commit comments

Comments
 (0)