Skip to content

Commit c82d8cb

Browse files
erik-krogherik-krogh
authored
Merge pull request github#11013 from erik-krogh/sndCmd
JS: second-order-command-injection
2 parents 858ae3d + 1f51bd4 commit c82d8cb

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+469
-70
lines changed

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

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
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+
/** Gets an example argument that can cause this command to execute arbitrary code. */
30+
string getVulnerableArgumentExample() {
31+
this = "git" and result = "--upload-pack"
32+
or
33+
this = "hg" and result = "--config=alias.<alias>=<command>"
34+
}
35+
}
36+
37+
/** A source for second order command injection. */
38+
abstract class Source extends DataFlow::Node {
39+
/** Gets a string that describes the source. For use in the alert message. */
40+
abstract string describe();
41+
42+
/** Gets a label for which this is a source. */
43+
abstract DataFlow::FlowLabel getALabel();
44+
}
45+
46+
/** A parameter of an exported function, seen as a source for second order command injection. */
47+
class ExternalInputSource extends Source {
48+
ExternalInputSource() { this = Exports::getALibraryInputParameter() }
49+
50+
override string describe() { result = "library input" }
51+
52+
override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() or result.isTaint() }
53+
}
54+
55+
/** A source of remote flow, seen as a source for second order command injection. */
56+
class RemoteFlowAsSource extends Source instanceof RemoteFlowSource {
57+
override string describe() { result = "a user-provided value" }
58+
59+
override DataFlow::FlowLabel getALabel() { result.isTaint() }
60+
}
61+
62+
private class TaintedObjectSourceAsSource extends Source instanceof TaintedObject::Source {
63+
override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() }
64+
65+
override string describe() { result = "a user-provided value" }
66+
}
67+
68+
/** A sanitizer for second order command injection. */
69+
abstract class Sanitizer extends DataFlow::Node { }
70+
71+
/** A sink for second order command injection. */
72+
abstract class Sink extends DataFlow::Node {
73+
/** Gets a label for which this is a sink. */
74+
abstract DataFlow::FlowLabel getALabel();
75+
76+
/** Gets the command getting invoked. I.e. `git` or `hg`. */
77+
abstract string getCommand();
78+
79+
/**
80+
* Gets an example argument for the comand that allows for second order command injection.
81+
* E.g. `--upload-pack` for `git`.
82+
*/
83+
abstract string getVulnerableArgumentExample();
84+
}
85+
86+
/**
87+
* A sink that invokes a command described by the `VulnerableCommand` class.
88+
*/
89+
abstract class VulnerableCommandSink extends Sink {
90+
VulnerableCommand cmd;
91+
92+
override string getCommand() { result = cmd }
93+
94+
override string getVulnerableArgumentExample() { result = cmd.getVulnerableArgumentExample() }
95+
}
96+
97+
/** A call that (indirectly) executes a shell command with a list of arguments. */
98+
abstract private class CommandExecutingCall extends DataFlow::CallNode {
99+
/** Gets the dataflow node representing the command to execute. */
100+
abstract DataFlow::Node getCommandArg();
101+
102+
/** Gets the dataflow node representing the arguments to the command. */
103+
abstract DataFlow::Node getArgList();
104+
}
105+
106+
/** A `SystemCommandExecution` seen as a command executing call. */
107+
private class SystemExecAsCmdCall extends CommandExecutingCall instanceof SystemCommandExecution {
108+
override DataFlow::Node getCommandArg() {
109+
result = SystemCommandExecution.super.getACommandArgument()
110+
}
111+
112+
override DataFlow::Node getArgList() { result = SystemCommandExecution.super.getArgumentList() }
113+
}
114+
115+
/** A function whose parameters is directly used a command and argument list for a shell invocation. */
116+
private class IndirectCmdFunc extends DataFlow::FunctionNode {
117+
int cmdIndex;
118+
int argIndex;
119+
120+
IndirectCmdFunc() {
121+
exists(CommandExecutingCall call |
122+
this.getParameter(cmdIndex).flowsTo(call.getCommandArg()) and
123+
this.getParameter(argIndex).flowsTo(call.getArgList())
124+
)
125+
}
126+
127+
/** Gets the parameter index that indicates the command to be executed. */
128+
int getCmdIndex() { result = cmdIndex }
129+
130+
/** Gets the parameter index that indicates the argument list to be passed to the command. */
131+
int getArgIndex() { result = argIndex }
132+
}
133+
134+
/** A call to a function that eventually executes a shell command with a list of arguments. */
135+
private class IndirectExecCall extends DataFlow::CallNode, CommandExecutingCall {
136+
IndirectCmdFunc func;
137+
138+
IndirectExecCall() { this.getACallee() = func.getFunction() }
139+
140+
override DataFlow::Node getCommandArg() { result = this.getArgument(func.getCmdIndex()) }
141+
142+
override DataFlow::Node getArgList() { result = this.getArgument(func.getArgIndex()) }
143+
}
144+
145+
/** Gets a dataflow node that ends up being used as an argument list to an invocation of `git` or `hg`. */
146+
private DataFlow::SourceNode usedAsVersionControlArgs(
147+
DataFlow::TypeBackTracker t, DataFlow::Node argList, VulnerableCommand cmd
148+
) {
149+
t.start() and
150+
exists(CommandExecutingCall exec | exec.getCommandArg().mayHaveStringValue(cmd) |
151+
exec.getArgList() = argList and
152+
result = argList.getALocalSource()
153+
)
154+
or
155+
exists(DataFlow::TypeBackTracker t2 |
156+
result = usedAsVersionControlArgs(t2, argList, cmd).backtrack(t2, t)
157+
)
158+
}
159+
160+
/** An argument to an invocation of `git`/`hg` that can cause second order command injection. */
161+
class ArgSink extends VulnerableCommandSink {
162+
ArgSink() {
163+
exists(DataFlow::ArrayCreationNode args |
164+
args = usedAsVersionControlArgs(DataFlow::TypeBackTracker::end(), _, cmd)
165+
|
166+
this = [args.getAnElement(), args.getASpreadArgument()] and
167+
args.getElement(0).mayHaveStringValue(cmd.getAVulnerableSubCommand()) and
168+
// not an "--" argument (even if it's earlier, then we assume it's on purpose)
169+
not args.getElement(_).mayHaveStringValue("--")
170+
)
171+
}
172+
173+
override DataFlow::FlowLabel getALabel() { result.isTaint() }
174+
}
175+
176+
/**
177+
* An arguments array given to an invocation of `git` or `hg` that can cause second order command injection.
178+
*/
179+
class ArgsArraySink extends VulnerableCommandSink {
180+
ArgsArraySink() {
181+
exists(SystemExecAsCmdCall exec | exec.getCommandArg().mayHaveStringValue(cmd) |
182+
this = exec.getArgList()
183+
)
184+
}
185+
186+
// only vulnerable if an attacker controls the entire array
187+
override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() }
188+
}
189+
190+
/**
191+
* A sanitizer that blocks flow when a string is tested to start with a certain prefix.
192+
*/
193+
class PrefixStringSanitizer extends TaintTracking::SanitizerGuardNode instanceof StringOps::StartsWith {
194+
override predicate sanitizes(boolean outcome, Expr e) {
195+
e = super.getBaseString().asExpr() and
196+
outcome = super.getPolarity()
197+
}
198+
}
199+
200+
/**
201+
* A sanitizer that blocks flow when a string does not start with "--"
202+
*/
203+
class DoubleDashSanitizer extends TaintTracking::SanitizerGuardNode instanceof StringOps::StartsWith {
204+
DoubleDashSanitizer() { super.getSubstring().mayHaveStringValue("--") }
205+
206+
override predicate sanitizes(boolean outcome, Expr e) {
207+
e = super.getBaseString().asExpr() and
208+
outcome = super.getPolarity().booleanNot()
209+
}
210+
}
211+
212+
/** A call to path.relative which sanitizes the taint. */
213+
class PathRelativeSanitizer extends Sanitizer {
214+
PathRelativeSanitizer() { this = NodeJSLib::Path::moduleMember("relative").getACall() }
215+
}
216+
}

‎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 starts 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. For example,
19+
ensure 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 snippet 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: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* @name Second order command injection
3+
* @description Using user-controlled data as arguments to some commands, such as git clone,
4+
* can allow arbitrary commands to be executed.
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, Sink sinkNode
21+
where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode()
22+
select sink.getNode(), source, sink,
23+
"Command line argument that depends on $@ can execute an arbitrary command if " +
24+
sinkNode.getVulnerableArgumentExample() + " is used with " + sinkNode.getCommand() + ".",
25+
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: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
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.
7+
This currently flags up unsafe invocations of git and hg.

0 commit comments

Comments
 (0)