Skip to content

Commit 04816f1

Browse files
authored
Merge pull request #22323 from lcmangalagiri/rust-cwe-078-command-injection
Rust: Add command injection query (CWE-078)
2 parents 9567f1a + 3d4cd37 commit 04816f1

14 files changed

Lines changed: 564 additions & 0 deletions

File tree

rust/ql/integration-tests/query-suite/rust-code-scanning.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ ql/rust/ql/src/queries/diagnostics/UnextractedElements.ql
1010
ql/rust/ql/src/queries/diagnostics/UnresolvedMacroCalls.ql
1111
ql/rust/ql/src/queries/security/CWE-020/RegexInjection.ql
1212
ql/rust/ql/src/queries/security/CWE-022/TaintedPath.ql
13+
ql/rust/ql/src/queries/security/CWE-078/CommandInjection.ql
1314
ql/rust/ql/src/queries/security/CWE-079/XSS.ql
1415
ql/rust/ql/src/queries/security/CWE-089/SqlInjection.ql
1516
ql/rust/ql/src/queries/security/CWE-295/DisabledCertificateCheck.ql

rust/ql/integration-tests/query-suite/rust-security-and-quality.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ ql/rust/ql/src/queries/diagnostics/UnextractedElements.ql
1010
ql/rust/ql/src/queries/diagnostics/UnresolvedMacroCalls.ql
1111
ql/rust/ql/src/queries/security/CWE-020/RegexInjection.ql
1212
ql/rust/ql/src/queries/security/CWE-022/TaintedPath.ql
13+
ql/rust/ql/src/queries/security/CWE-078/CommandInjection.ql
1314
ql/rust/ql/src/queries/security/CWE-079/XSS.ql
1415
ql/rust/ql/src/queries/security/CWE-089/SqlInjection.ql
1516
ql/rust/ql/src/queries/security/CWE-117/LogInjection.ql

rust/ql/integration-tests/query-suite/rust-security-extended.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ ql/rust/ql/src/queries/diagnostics/UnextractedElements.ql
1010
ql/rust/ql/src/queries/diagnostics/UnresolvedMacroCalls.ql
1111
ql/rust/ql/src/queries/security/CWE-020/RegexInjection.ql
1212
ql/rust/ql/src/queries/security/CWE-022/TaintedPath.ql
13+
ql/rust/ql/src/queries/security/CWE-078/CommandInjection.ql
1314
ql/rust/ql/src/queries/security/CWE-079/XSS.ql
1415
ql/rust/ql/src/queries/security/CWE-089/SqlInjection.ql
1516
ql/rust/ql/src/queries/security/CWE-117/LogInjection.ql
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/rust-all
4+
extensible: sinkModel
5+
data:
6+
# std::process::Command - the command name itself
7+
- ["<std::process::Command>::new", "Argument[0]", "command-injection", "manual"]
8+
# std::process::Command - arguments passed to the command
9+
- ["<std::process::Command>::arg", "Argument[0]", "command-injection", "manual"]
10+
- ["<std::process::Command>::args", "Argument[0]", "command-injection", "manual"]
11+
# tokio::process::Command - the command name itself
12+
- ["<tokio::process::Command>::new", "Argument[0]", "command-injection", "manual"]
13+
# tokio::process::Command - arguments passed to the command
14+
- ["<tokio::process::Command>::arg", "Argument[0]", "command-injection", "manual"]
15+
- ["<tokio::process::Command>::args", "Argument[0]", "command-injection", "manual"]
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/**
2+
* Provides classes and predicates for reasoning about command injection
3+
* vulnerabilities (CWE-078).
4+
*/
5+
6+
import rust
7+
private import codeql.rust.dataflow.DataFlow
8+
private import codeql.rust.dataflow.FlowSink
9+
private import codeql.rust.dataflow.FlowBarrier
10+
private import codeql.rust.Concepts
11+
private import codeql.rust.security.Barriers as Barriers
12+
13+
/**
14+
* Provides default sources, sinks and barriers for detecting command injection
15+
* vulnerabilities, as well as extension points for adding your own.
16+
*/
17+
module CommandInjection {
18+
/**
19+
* A data flow source for command injection vulnerabilities.
20+
*/
21+
abstract class Source extends DataFlow::Node { }
22+
23+
/**
24+
* A data flow sink for command injection vulnerabilities.
25+
*/
26+
abstract class Sink extends QuerySink::Range {
27+
override string getSinkType() { result = "CommandInjection" }
28+
}
29+
30+
/**
31+
* A barrier for command injection vulnerabilities.
32+
*/
33+
abstract class Barrier extends DataFlow::Node { }
34+
35+
/**
36+
* An active threat-model source, considered as a flow source.
37+
*/
38+
private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }
39+
40+
/**
41+
* A sink for command injection from model data.
42+
*/
43+
private class ModelsAsDataSink extends Sink {
44+
ModelsAsDataSink() { sinkNode(this, "command-injection") }
45+
}
46+
47+
/**
48+
* A barrier for command injection from model data.
49+
*/
50+
private class ModelsAsDataBarrier extends Barrier {
51+
ModelsAsDataBarrier() { barrierNode(this, "command-injection") }
52+
}
53+
54+
/**
55+
* A barrier for command injection vulnerabilities for nodes whose type is a
56+
* numeric type, which is unlikely to expose any vulnerability.
57+
*/
58+
private class NumericTypeBarrier extends Barrier instanceof Barriers::NumericTypeBarrier { }
59+
60+
private class BooleanTypeBarrier extends Barrier instanceof Barriers::BooleanTypeBarrier { }
61+
62+
private class FieldlessEnumTypeBarrier extends Barrier instanceof Barriers::FieldlessEnumTypeBarrier
63+
{ }
64+
65+
/**
66+
* A sanitizer guard for command injection vulnerabilities.
67+
*/
68+
class SanitizerGuard extends Barrier {
69+
SanitizerGuard() { this = DataFlow::BarrierGuard<sanitizerGuard/3>::getABarrierNode() }
70+
}
71+
}
72+
73+
private predicate sanitizerGuard(AstNode g, Expr e, boolean branch) {
74+
g.(SanitizerGuard::Range).checks(e, branch)
75+
}
76+
77+
/**
78+
* Provides a class for modeling new command injection safety checks.
79+
*/
80+
module SanitizerGuard {
81+
/**
82+
* A data-flow node that checks whether a command is safe.
83+
*/
84+
abstract class Range extends AstNode {
85+
/**
86+
* Holds if this guard validates `e` upon evaluating to `branch`.
87+
*/
88+
abstract predicate checks(Expr e, boolean branch);
89+
}
90+
}
91+
92+
/**
93+
* A successful membership check against a (presumed) command allowlist. For example:
94+
* ```
95+
* if allowlist.contains(&commmand) { ... }
96+
* ```
97+
*/
98+
private class AllowlistContainsCheck extends SanitizerGuard::Range, MethodCall {
99+
AllowlistContainsCheck() { this.getStaticTarget().getName().getText() = "contains" }
100+
101+
override predicate checks(Expr e, boolean branch) {
102+
e.getParentNode*() = this.getPositionalArgument(0) and
103+
branch = true
104+
}
105+
}
106+
107+
/**
108+
* An equality check against a (presumed) allowed command value. For example:
109+
* ```
110+
* if command == "ls" { ... }
111+
* ```
112+
*/
113+
private class AllowlistEqualityCheck extends SanitizerGuard::Range, EqualityOperation {
114+
override predicate checks(Expr e, boolean branch) {
115+
e = this.getAnOperand() and
116+
(
117+
this instanceof EqualsOperation and branch = true
118+
or
119+
this instanceof NotEqualsOperation and branch = false
120+
)
121+
}
122+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rust/command-line-injection`, to detect uncontrolled command lines.
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
7+
<p>
8+
If a system command is built from user-provided data without sufficient sanitization, a user may be able to run malicious commands. An attacker can craft input to change the meaning of the command, potentially gaining control of the system.
9+
</p>
10+
11+
</overview>
12+
<recommendation>
13+
14+
<p>
15+
If possible, use hard-coded string literals for commands. If the command must be built from user-provided data, do not pass user input directly to shell commands. Instead, use APIs that accept command arguments as separate parameters (such as <code>std::process::Command</code> with individual <code>.arg()</code> calls for each argument), which avoids shell interpretation of special characters. If shell execution is necessary, validate and sanitize user input against an allowlist of permitted values.
16+
</p>
17+
18+
</recommendation>
19+
<example>
20+
21+
<p>
22+
In the following example, a command is constructed directly from user-controlled input obtained via an HTTP request. An attacker could supply a malicious value to execute arbitrary commands.
23+
</p>
24+
25+
<sample src="CommandInjectionBad.rs" />
26+
27+
<p>
28+
A safer approach uses a fixed command with validated arguments, or avoids shell interpretation entirely:
29+
</p>
30+
31+
<sample src="CommandInjectionGood.rs" />
32+
33+
</example>
34+
<references>
35+
36+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/Command_Injection">Command Injection</a>.</li>
37+
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection#Shell_injection">Shell injection</a>.</li>
38+
39+
</references>
40+
</qhelp>
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/**
2+
* @name Uncontrolled command line
3+
* @description Using externally controlled strings in a command line may allow a malicious
4+
* user to change the meaning of the command.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 9.8
8+
* @precision high
9+
* @id rust/command-line-injection
10+
* @tags security
11+
* external/cwe/cwe-078
12+
* external/cwe/cwe-088
13+
*/
14+
15+
import rust
16+
import codeql.rust.dataflow.DataFlow
17+
import codeql.rust.dataflow.TaintTracking
18+
import codeql.rust.security.CommandInjectionExtensions
19+
20+
/**
21+
* A taint configuration for detecting command injection vulnerabilities.
22+
*/
23+
module CommandInjectionConfig implements DataFlow::ConfigSig {
24+
import CommandInjection
25+
26+
predicate isSource(DataFlow::Node node) { node instanceof Source }
27+
28+
predicate isSink(DataFlow::Node node) { node instanceof Sink }
29+
30+
predicate isBarrier(DataFlow::Node barrier) { barrier instanceof Barrier }
31+
32+
predicate observeDiffInformedIncrementalMode() { any() }
33+
}
34+
35+
module CommandInjectionFlow = TaintTracking::Global<CommandInjectionConfig>;
36+
37+
import CommandInjectionFlow::PathGraph
38+
39+
from CommandInjectionFlow::PathNode sourceNode, CommandInjectionFlow::PathNode sinkNode
40+
where CommandInjectionFlow::flowPath(sourceNode, sinkNode)
41+
select sinkNode.getNode(), sourceNode, sinkNode, "This command line depends on a $@.",
42+
sourceNode.getNode(), "user-provided value"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
use std::process::Command;
2+
3+
fn handle_request(user_input: &str) {
4+
// BAD: user input is passed directly to a shell command
5+
Command::new("sh")
6+
.arg("-c")
7+
.arg(user_input)
8+
.output()
9+
.expect("failed to execute");
10+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
use std::process::Command;
2+
3+
fn handle_request(filename: &str) {
4+
// GOOD: user input is checked against an allowlist before passing into a shell command
5+
let allowed_names = ["report.pdf", "summary.txt", "data.csv"];
6+
if allowed_names.contains(&filename) {
7+
Command::new("cat")
8+
.arg(filename)
9+
.output()
10+
.expect("failed to execute");
11+
}
12+
}

0 commit comments

Comments
 (0)