Skip to content

Commit c82410f

Browse files
erik-krogherik-krogh
authored
Merge pull request #10680 from erik-krogh/unsafeRbCmd
RB: add an unsafe-shell-command-construction query
2 parents 8c8f141 + 3f871a0 commit c82410f

20 files changed

Lines changed: 497 additions & 0 deletions

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,9 @@ class ExprNode extends Node, TExprNode {
147147
class ParameterNode extends Node, TParameterNode instanceof ParameterNodeImpl {
148148
/** Gets the parameter corresponding to this node, if any. */
149149
final Parameter getParameter() { result = super.getParameter() }
150+
151+
/** Gets the name of the parameter, if any. */
152+
final string getName() { result = this.getParameter().(NamedParameter).getName() }
150153
}
151154

152155
/**

‎ruby/ql/lib/codeql/ruby/frameworks/Core.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow
77
private import codeql.ruby.dataflow.FlowSummary
88
import core.BasicObject::BasicObject
99
import core.Object::Object
10+
import core.Gem::Gem
1011
import core.Kernel::Kernel
1112
import core.Module
1213
import core.Array

‎ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/**
2+
* Provides modeling for the `Gem` module and `.gemspec` files.
3+
*/
4+
5+
private import ruby
6+
private import Ast
7+
private import codeql.ruby.ApiGraphs
8+
9+
/** Provides modeling for the `Gem` module and `.gemspec` files. */
10+
module Gem {
11+
/**
12+
* A .gemspec file that lists properties of a Ruby gem.
13+
* The contents of a .gemspec file generally look like:
14+
* ```Ruby
15+
* Gem::Specification.new do |s|
16+
* s.name = 'library-name'
17+
* s.require_path = "lib"
18+
* # more properties
19+
* end
20+
* ```
21+
*/
22+
class GemSpec instanceof File {
23+
API::Node specCall;
24+
25+
GemSpec() {
26+
this.getExtension() = "gemspec" and
27+
specCall = API::root().getMember("Gem").getMember("Specification").getMethod("new") and
28+
specCall.getLocation().getFile() = this
29+
}
30+
31+
/** Gets the name of this .gemspec file. */
32+
string toString() { result = File.super.getBaseName() }
33+
34+
/**
35+
* Gets a value of the `name` property of this .gemspec file.
36+
* These properties are set using the `Gem::Specification.new` method.
37+
*/
38+
private Expr getSpecProperty(string name) {
39+
exists(Expr rhs |
40+
rhs =
41+
specCall
42+
.getBlock()
43+
.getParameter(0)
44+
.getMethod(name + "=")
45+
.getParameter(0)
46+
.asSink()
47+
.asExpr()
48+
.getExpr()
49+
.(Ast::AssignExpr)
50+
.getRightOperand()
51+
|
52+
result = rhs
53+
or
54+
// some properties are arrays, we just unfold them
55+
result = rhs.(ArrayLiteral).getAnElement()
56+
)
57+
}
58+
59+
/** Gets the name of the gem */
60+
string getName() { result = getSpecProperty("name").getConstantValue().getString() }
61+
62+
/** Gets a path that is loaded when the gem is required */
63+
private string getARequirePath() {
64+
result = getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()
65+
or
66+
not exists(getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()) and
67+
result = "lib" // the default is "lib"
68+
}
69+
70+
/** Gets a file that could be loaded when the gem is required. */
71+
private File getAPossiblyRequiredFile() {
72+
result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*()
73+
}
74+
75+
/** Gets a class/module that is exported by this gem. */
76+
private ModuleBase getAPublicModule() {
77+
result.(Toplevel).getLocation().getFile() = getAPossiblyRequiredFile()
78+
or
79+
result = getAPublicModule().getAModule()
80+
or
81+
result = getAPublicModule().getAClass()
82+
or
83+
result = getAPublicModule().getStmt(_).(SingletonClass)
84+
}
85+
86+
/** Gets a parameter from an exported method, which is an input to this gem. */
87+
DataFlow::ParameterNode getAnInputParameter() {
88+
exists(MethodBase method | method = getAPublicModule().getAMethod() |
89+
result.getParameter() = method.getAParameter() and
90+
method.isPublic()
91+
)
92+
}
93+
}
94+
95+
/** Gets a parameter that is an input to a named gem. */
96+
DataFlow::ParameterNode getALibraryInput() {
97+
exists(GemSpec spec |
98+
exists(spec.getName()) and // we only consider `.gemspec` files that have a name
99+
result = spec.getAnInputParameter()
100+
)
101+
}
102+
}

‎ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ module CommandInjection {
4949
class ShellwordsEscapeAsSanitizer extends Sanitizer {
5050
ShellwordsEscapeAsSanitizer() {
5151
this = API::getTopLevelMember("Shellwords").getAMethodCall(["escape", "shellescape"])
52+
or
53+
// The method is also added as `String#shellescape`.
54+
this.(DataFlow::CallNode).getMethodName() = "shellescape"
5255
}
5356
}
5457
}

‎ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* shell command constructed from library input vulnerabilities, as
4+
* well as extension points for adding your own.
5+
*/
6+
7+
private import codeql.ruby.DataFlow
8+
private import codeql.ruby.DataFlow2
9+
private import codeql.ruby.ApiGraphs
10+
private import codeql.ruby.frameworks.core.Gem::Gem as Gem
11+
private import codeql.ruby.AST as Ast
12+
private import codeql.ruby.Concepts as Concepts
13+
14+
/**
15+
* Module containing sources, sinks, and sanitizers for shell command constructed from library input.
16+
*/
17+
module UnsafeShellCommandConstruction {
18+
/** A source for shell command constructed from library input vulnerabilities. */
19+
abstract class Source extends DataFlow::Node { }
20+
21+
/** An input parameter to a gem seen as a source. */
22+
private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
23+
LibraryInputAsSource() {
24+
this = Gem::getALibraryInput() and
25+
// we exclude arguments named `cmd` or similar, as they seem to execute commands on purpose
26+
not exists(string name | name = super.getName() |
27+
name = ["cmd", "command"]
28+
or
29+
name.regexpMatch(".*(Cmd|Command)$")
30+
)
31+
}
32+
}
33+
34+
/** A sink for shell command constructed from library input vulnerabilities. */
35+
abstract class Sink extends DataFlow::Node {
36+
/** Gets a description of how the string in this sink was constructed. */
37+
abstract string describe();
38+
39+
/** Gets the dataflow node where the string is constructed. */
40+
DataFlow::Node getStringConstruction() { result = this }
41+
42+
/** Gets the dataflow node that executed the string as a shell command. */
43+
abstract DataFlow::Node getCommandExecution();
44+
}
45+
46+
/** Holds if the string constructed at `source` is executed at `shellExec` */
47+
predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) {
48+
source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec)
49+
}
50+
51+
import codeql.ruby.typetracking.TypeTracker as TypeTracker
52+
53+
private DataFlow::LocalSourceNode backtrackShellExec(
54+
TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec
55+
) {
56+
t.start() and
57+
result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource()
58+
or
59+
exists(TypeTracker::TypeBackTracker t2 |
60+
result = backtrackShellExec(t2, shellExec).backtrack(t2, t)
61+
)
62+
}
63+
64+
/**
65+
* A string constructed from a string-literal (e.g. `"foo #{sink}"`),
66+
* where the resulting string ends up being executed as a shell command.
67+
*/
68+
class StringInterpolationAsSink extends Sink {
69+
Concepts::SystemCommandExecution s;
70+
Ast::StringLiteral lit;
71+
72+
StringInterpolationAsSink() {
73+
isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and
74+
this.asExpr().getExpr() = lit.getComponent(_)
75+
}
76+
77+
override string describe() { result = "string construction" }
78+
79+
override DataFlow::Node getCommandExecution() { result = s }
80+
81+
override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit }
82+
}
83+
84+
import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
85+
86+
/**
87+
* A string constructed from a printf-style call,
88+
* where the resulting string ends up being executed as a shell command.
89+
*/
90+
class TaintedFormatStringAsSink extends Sink {
91+
Concepts::SystemCommandExecution s;
92+
TaintedFormat::PrintfStyleCall call;
93+
94+
TaintedFormatStringAsSink() {
95+
isUsedAsShellCommand(call, s) and
96+
this = [call.getFormatArgument(_), call.getFormatString()]
97+
}
98+
99+
override string describe() { result = "formatted string" }
100+
101+
override DataFlow::Node getCommandExecution() { result = s }
102+
103+
override DataFlow::Node getStringConstruction() { result = call }
104+
}
105+
}

‎ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about shell command
3+
* constructed from library input vulnerabilities
4+
*
5+
* Note, for performance reasons: only import this file if `Configuration` is needed,
6+
* otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead.
7+
*/
8+
9+
import codeql.ruby.DataFlow
10+
import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
11+
private import codeql.ruby.TaintTracking
12+
private import CommandInjectionCustomizations::CommandInjection as CommandInjection
13+
private import codeql.ruby.dataflow.BarrierGuards
14+
15+
/**
16+
* A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.
17+
*/
18+
class Configuration extends TaintTracking::Configuration {
19+
Configuration() { this = "UnsafeShellCommandConstruction" }
20+
21+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
24+
25+
override predicate isSanitizer(DataFlow::Node node) {
26+
node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection`
27+
node instanceof StringConstCompareBarrier or
28+
node instanceof StringConstArrayInclusionCallBarrier
29+
}
30+
31+
// override to require the path doesn't have unmatched return steps
32+
override DataFlow::FlowFeature getAFeature() {
33+
result instanceof DataFlow::FeatureHasSourceCallContext
34+
}
35+
}

‎ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md‎

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, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs.

‎ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Dynamically constructing a shell command with inputs from exported
8+
functions may inadvertently change the meaning of the shell command.
9+
10+
Clients using the exported function may use inputs containing
11+
characters that the shell interprets in a special way, for instance
12+
quotes and spaces.
13+
14+
This can result in the shell command misbehaving, or even
15+
allowing a malicious user to execute arbitrary commands on the system.
16+
</p>
17+
18+
19+
</overview>
20+
<recommendation>
21+
22+
<p>
23+
If possible, provide the dynamic arguments to the shell as an array
24+
to APIs such as <code>system(..)</code> to avoid interpretation by the shell.
25+
</p>
26+
27+
<p>
28+
Alternatively, if the shell command must be constructed
29+
dynamically, then add code to ensure that special characters
30+
do not alter the shell command unexpectedly.
31+
</p>
32+
33+
</recommendation>
34+
<example>
35+
36+
<p>
37+
The following example shows a dynamically constructed shell
38+
command that downloads a file from a remote URL.
39+
</p>
40+
41+
<sample src="examples/unsafe-shell-command-construction.rb" />
42+
43+
<p>
44+
The shell command will, however, fail to work as intended if the
45+
input contains spaces or other special characters interpreted in a
46+
special way by the shell.
47+
</p>
48+
49+
<p>
50+
Even worse, a client might pass in user-controlled
51+
data, not knowing that the input is interpreted as a shell command.
52+
This could allow a malicious user to provide the input <code>http://example.org; cat /etc/passwd</code>
53+
in order to execute the command <code>cat /etc/passwd</code>.
54+
</p>
55+
56+
<p>
57+
To avoid such potentially catastrophic behaviors, provide the
58+
input from exported functions as an argument that does not
59+
get interpreted by a shell:
60+
</p>
61+
62+
<sample src="examples/unsafe-shell-command-construction_fixed.rb" />
63+
64+
</example>
65+
<references>
66+
67+
<li>
68+
OWASP:
69+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
70+
</li>
71+
72+
</references>
73+
</qhelp>

‎ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* @name Unsafe shell command constructed from library input
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 6.3
8+
* @precision high
9+
* @id rb/shell-command-constructed-from-input
10+
* @tags correctness
11+
* security
12+
* external/cwe/cwe-078
13+
* external/cwe/cwe-088
14+
* external/cwe/cwe-073
15+
*/
16+
17+
import codeql.ruby.security.UnsafeShellCommandConstructionQuery
18+
import DataFlow::PathGraph
19+
20+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
21+
where
22+
config.hasFlowPath(source, sink) and
23+
sinkNode = sink.getNode()
24+
select sinkNode.getStringConstruction(), source, sink,
25+
"This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(),
26+
"library input", sinkNode.getCommandExecution(), "shell command"

‎ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module Utils
2+
def download(path)
3+
system("wget #{path}") # NOT OK
4+
end
5+
end

0 commit comments

Comments
 (0)