Skip to content

Commit 332bc35

Browse files
erik-krogherik-krogh
authored
Merge pull request #10708 from erik-krogh/kernelSink
RB: add a query flagging uses of `Kernel.open()` that are not with a constant string
2 parents a327802 + 9fe18e5 commit 332bc35

16 files changed

Lines changed: 160 additions & 83 deletions

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/**
2+
* Provides utility classes and predicates for reasoning about `Kernel.open` and related methods.
3+
*/
4+
5+
private import codeql.ruby.AST
6+
private import codeql.ruby.DataFlow
7+
private import codeql.ruby.AST
8+
private import codeql.ruby.ApiGraphs
9+
private import codeql.ruby.frameworks.core.Kernel::Kernel
10+
11+
/** A call to a method that might access a file or start a process. */
12+
class AmbiguousPathCall extends DataFlow::CallNode {
13+
string name;
14+
15+
AmbiguousPathCall() {
16+
this.(KernelMethodCall).getMethodName() = "open" and
17+
name = "Kernel.open"
18+
or
19+
this = API::getTopLevelMember("IO").getAMethodCall("read") and
20+
not this = API::getTopLevelMember("File").getAMethodCall("read") and // needed in e.g. opal/opal, where some calls have both paths, but I'm not sure why
21+
name = "IO.read"
22+
}
23+
24+
/** Gets the name for the method being called. */
25+
string getName() { result = name }
26+
27+
/** Gets the name for a safer method that can be used instead. */
28+
string getReplacement() {
29+
result = "File.read" and name = "IO.read"
30+
or
31+
result = "File.open" and name = "Kernel.open"
32+
}
33+
34+
/** Gets the argument that specifies the path to be accessed. */
35+
DataFlow::Node getPathArgument() { result = this.getArgument(0) }
36+
}

‎ruby/ql/src/change-notes/2022-10-06-non-constant-kernel-open.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/non-constant-kernel-open`, to detect uses of Kernel.open and related methods with non-constant values.

‎ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.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>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
7+
character, it will execute the remaining string as a shell command. If a
8+
malicious user can control the file name, they can execute arbitrary code.
9+
The same vulnerability applies to <code>IO.read</code>.
10+
</p>
11+
12+
</overview>
13+
<recommendation>
14+
15+
<p>Use <code>File.open</code> instead of <code>Kernel.open</code>, as the former
16+
does not have this vulnerability. Similarly, use <code>File.read</code> instead
17+
of <code>IO.read</code>.</p>
18+
19+
</recommendation>
20+
<example>
21+
22+
<p>
23+
The following example shows code that calls <code>Kernel.open</code> on a
24+
user-supplied file path.
25+
</p>
26+
27+
<sample src="examples/kernel_open.rb" />
28+
29+
<p>Instead, <code>File.open</code> should be used, as in the following example.</p>
30+
31+
<sample src="examples/file_open.rb" />
32+
33+
</example>
34+
<references>
35+
36+
<li>
37+
OWASP:
38+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
39+
</li>
40+
41+
<li>
42+
Example CVE: <a href="https://www.ruby-lang.org/en/news/2021/05/02/os-command-injection-in-rdoc/">Command Injection in RDoc</a>.
43+
</li>
44+
45+
</references>
46+
</qhelp>

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

Lines changed: 3 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,4 @@
1-
<!DOCTYPE qhelp PUBLIC
2-
"-//Semmle//qhelp//EN"
3-
"qhelp.dtd">
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
42
<qhelp>
5-
<overview>
6-
<p>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
7-
character, it will execute the remaining string as a shell command. If a
8-
malicious user can control the file name, they can execute arbitrary code.
9-
The same vulnerability applies to <code>IO.read</code>.
10-
</p>
11-
12-
</overview>
13-
<recommendation>
14-
15-
<p>Use <code>File.open</code> instead of <code>Kernel.open</code>, as the former
16-
does not have this vulnerability. Similarly, use <code>File.read</code> instead
17-
of <code>IO.read</code>.</p>
18-
19-
</recommendation>
20-
<example>
21-
22-
<p>
23-
The following example shows code that calls <code>Kernel.open</code> on a
24-
user-supplied file path.
25-
</p>
26-
27-
<sample src="examples/kernel_open.rb" />
28-
29-
<p>Instead, <code>File.open</code> should be used, as in the following example.</p>
30-
31-
<sample src="examples/file_open.rb" />
32-
33-
</example>
34-
<references>
35-
36-
<li>
37-
OWASP:
38-
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
39-
</li>
40-
41-
<li>
42-
Example CVE: <a href="https://www.ruby-lang.org/en/news/2021/05/02/os-command-injection-in-rdoc/">Command Injection in RDoc</a>.
43-
</li>
44-
45-
</references>
46-
</qhelp>
3+
<include src="KernelOpen.inc.qhelp" />
4+
</qhelp>

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

Lines changed: 8 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* @name Use of `Kernel.open` or `IO.read`
2+
* @name Use of `Kernel.open` or `IO.read` with user-controlled input
33
* @description Using `Kernel.open` or `IO.read` may allow a malicious
44
* user to execute arbitrary system commands.
55
* @kind path-problem
@@ -14,49 +14,20 @@
1414
* external/cwe/cwe-073
1515
*/
1616

17-
import codeql.ruby.AST
18-
import codeql.ruby.ApiGraphs
19-
import codeql.ruby.frameworks.core.Kernel::Kernel
17+
import codeql.ruby.DataFlow
2018
import codeql.ruby.TaintTracking
21-
import codeql.ruby.dataflow.BarrierGuards
2219
import codeql.ruby.dataflow.RemoteFlowSources
23-
import codeql.ruby.DataFlow
20+
import codeql.ruby.dataflow.BarrierGuards
2421
import DataFlow::PathGraph
25-
26-
/**
27-
* A method call that has a suggested replacement.
28-
*/
29-
abstract class Replacement extends DataFlow::CallNode {
30-
abstract string getFrom();
31-
32-
abstract string getTo();
33-
}
34-
35-
class KernelOpenCall extends KernelMethodCall, Replacement {
36-
KernelOpenCall() { this.getMethodName() = "open" }
37-
38-
override string getFrom() { result = "Kernel.open" }
39-
40-
override string getTo() { result = "File.open" }
41-
}
42-
43-
class IOReadCall extends DataFlow::CallNode, Replacement {
44-
IOReadCall() { this = API::getTopLevelMember("IO").getAMethodCall("read") }
45-
46-
override string getFrom() { result = "IO.read" }
47-
48-
override string getTo() { result = "File.read" }
49-
}
22+
import codeql.ruby.security.KernelOpenQuery
5023

5124
class Configuration extends TaintTracking::Configuration {
5225
Configuration() { this = "KernelOpen" }
5326

5427
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
5528

5629
override predicate isSink(DataFlow::Node sink) {
57-
exists(KernelOpenCall c | c.getArgument(0) = sink)
58-
or
59-
exists(IOReadCall c | c.getArgument(0) = sink)
30+
sink = any(AmbiguousPathCall r).getPathArgument()
6031
}
6132

6233
override predicate isSanitizer(DataFlow::Node node) {
@@ -73,5 +44,6 @@ where
7344
sourceNode = source.getNode() and
7445
call.getArgument(0) = sink.getNode()
7546
select sink.getNode(), source, sink,
76-
"This call to " + call.(Replacement).getFrom() + " depends on a $@. Replace it with " +
77-
call.(Replacement).getTo() + ".", source.getNode(), "user-provided value"
47+
"This call to " + call.(AmbiguousPathCall).getName() +
48+
" depends on a $@. Consider replacing it with " + call.(AmbiguousPathCall).getReplacement() +
49+
".", source.getNode(), "user-provided value"

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
<include src="KernelOpen.inc.qhelp" />
4+
</qhelp>

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* @name Use of `Kernel.open` or `IO.read` with a non-constant value
3+
* @description Using `Kernel.open` or `IO.read` may allow a malicious
4+
* user to execute arbitrary system commands.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @security-severity 6.5
8+
* @precision high
9+
* @id rb/non-constant-kernel-open
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.KernelOpenQuery
18+
import codeql.ruby.ast.Literal
19+
20+
from AmbiguousPathCall call
21+
where
22+
// there is not a constant string argument
23+
not exists(call.getPathArgument().asExpr().getExpr().getConstantValue()) and
24+
// if it's a format string, then the first argument is not a constant string
25+
not call.getPathArgument().getALocalSource().asExpr().getExpr().(StringLiteral).getComponent(0)
26+
instanceof StringTextComponent
27+
select call,
28+
"Call to " + call.getName() + " with a non-constant value. Consider replacing it with " +
29+
call.getReplacement() + "."

ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected renamed to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected

File renamed without changes.

ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref renamed to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref

File renamed without changes.

ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb renamed to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb

File renamed without changes.

0 commit comments

Comments
 (0)