Skip to content

Commit 4ff823c

Browse files
erik-krogherik-krogh
authored
Merge pull request #11366 from p-/p--ruby-kernel-open-addition
Ruby: Add additional sinks to the `rb/kernel-open` query
2 parents 912aa46 + d2c8e70 commit 4ff823c

File tree

11 files changed

+137
-21
lines changed

11 files changed

+137
-21
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ module File {
133133
}
134134

135135
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
136-
input = "Argument[0..]" and
136+
input = "Argument[0,1..]" and
137137
output = "ReturnValue" and
138138
preservesValue = false
139139
}

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

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow
77
private import codeql.ruby.AST
88
private import codeql.ruby.ApiGraphs
99
private import codeql.ruby.frameworks.core.Kernel::Kernel
10+
private import codeql.ruby.frameworks.Files
1011

1112
/** A call to a method that might access a file or start a process. */
1213
class AmbiguousPathCall extends DataFlow::CallNode {
@@ -16,21 +17,57 @@ class AmbiguousPathCall extends DataFlow::CallNode {
1617
this.(KernelMethodCall).getMethodName() = "open" and
1718
name = "Kernel.open"
1819
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"
20+
exists(string methodName |
21+
methodName = ["read", "write", "binread", "binwrite", "foreach", "readlines"]
22+
|
23+
methodCallOnlyOnIO(this, methodName) and
24+
name = "IO." + methodName
25+
)
26+
or
27+
this = API::getTopLevelMember("URI").getAMethodCall("open") and
28+
name = "URI.open"
2229
}
2330

2431
/** Gets the name for the method being called. */
2532
string getName() { result = name }
2633

2734
/** Gets the name for a safer method that can be used instead. */
2835
string getReplacement() {
36+
result = "File.open" and name = "Kernel.open"
37+
or
2938
result = "File.read" and name = "IO.read"
3039
or
31-
result = "File.open" and name = "Kernel.open"
40+
result = "File.write" and name = "IO.write"
41+
or
42+
result = "File.binread" and name = "IO.binread"
43+
or
44+
result = "File.binwrite" and name = "IO.binwrite"
45+
or
46+
result = "File.foreach" and name = "IO.foreach"
47+
or
48+
result = "File.readlines" and name = "IO.readlines"
49+
or
50+
result = "URI(<uri>).open" and name = "URI.open"
3251
}
3352

3453
/** Gets the argument that specifies the path to be accessed. */
3554
DataFlow::Node getPathArgument() { result = this.getArgument(0) }
3655
}
56+
57+
private predicate methodCallOnlyOnIO(DataFlow::CallNode node, string methodName) {
58+
node = API::getTopLevelMember("IO").getAMethodCall(methodName) and
59+
not node = API::getTopLevelMember("File").getAMethodCall(methodName) // needed in e.g. opal/opal, where some calls have both paths (opal implements an own corelib)
60+
}
61+
62+
/**
63+
* A sanitizer for kernel open vulnerabilities.
64+
*/
65+
abstract class Sanitizer extends DataFlow::Node { }
66+
67+
/**
68+
* If a `File.join` is performed the resulting string will not start with a pipe `|`.
69+
* This is true as long the tainted data doesn't flow into the first argument.
70+
*/
71+
private class FileJoinSanitizer extends Sanitizer {
72+
FileJoinSanitizer() { this = any(File::FileJoinSummary s).getParameter("1..") }
73+
}

‎ruby/ql/src/change-notes/2022-11-22-kernel-open-sinks.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Extended the `rb/kernel-open` query with following sinks: `IO.write`, `IO.binread`, `IO.binwrite`, `IO.foreach`, `IO.readlines`, and `URI.open`.

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,19 @@
66
<p>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
77
character, it will execute the remaining string as a shell command. If a
88
malicious user can control the file name, they can execute arbitrary code.
9-
The same vulnerability applies to <code>IO.read</code>.
9+
The same vulnerability applies to <code>IO.read</code>, <code>IO.write</code>,
10+
<code>IO.binread</code>, <code>IO.binwrite</code>, <code>IO.foreach</code>,
11+
<code>IO.readlines</code> and <code>URI.open</code>.
1012
</p>
1113

1214
</overview>
1315
<recommendation>
1416

1517
<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
18+
does not have this vulnerability. Similarly, use the methods from the <code>File</code>
19+
class instead of the <code>IO</code> class e.g. <code>File.read</code> instead
1720
of <code>IO.read</code>.</p>
21+
<p>Instead of <code>URI.open</code> use <code>URI(..).open</code> or an HTTP Client.</p>
1822

1923
</recommendation>
2024
<example>
@@ -36,6 +40,7 @@ user-supplied file path.
3640
<li>
3741
OWASP:
3842
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
43+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Ruby_on_Rails_Cheat_Sheet.html#command-injection">Ruby on Rails Cheat Sheet: Command Injection</a>.
3944
</li>
4045

4146
<li>

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
2-
* @name Use of `Kernel.open` or `IO.read` with user-controlled input
3-
* @description Using `Kernel.open` or `IO.read` may allow a malicious
2+
* @name Use of `Kernel.open`, `IO.read` or similar sinks with user-controlled input
3+
* @description Using `Kernel.open`, `IO.read`, `IO.write`, `IO.binread`, `IO.binwrite`,
4+
* `IO.foreach`, `IO.readlines`, or `URI.open` may allow a malicious
45
* user to execute arbitrary system commands.
56
* @kind path-problem
67
* @problem.severity error
@@ -32,7 +33,8 @@ class Configuration extends TaintTracking::Configuration {
3233

3334
override predicate isSanitizer(DataFlow::Node node) {
3435
node instanceof StringConstCompareBarrier or
35-
node instanceof StringConstArrayInclusionCallBarrier
36+
node instanceof StringConstArrayInclusionCallBarrier or
37+
node instanceof Sanitizer
3638
}
3739
}
3840

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
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
2+
* @name Use of `Kernel.open` or `IO.read` or similar sinks with a non-constant value
3+
* @description Using `Kernel.open`, `IO.read`, `IO.write`, `IO.binread`, `IO.binwrite`,
4+
* `IO.foreach`, `IO.readlines`, or `URI.open` may allow a malicious
45
* user to execute arbitrary system commands.
56
* @kind problem
67
* @problem.severity warning
@@ -15,15 +16,25 @@
1516
*/
1617

1718
import codeql.ruby.security.KernelOpenQuery
18-
import codeql.ruby.ast.Literal
19+
import codeql.ruby.AST
20+
import codeql.ruby.ApiGraphs
1921

2022
from AmbiguousPathCall call
2123
where
22-
// there is not a constant string argument
23-
not exists(call.getPathArgument().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
24+
not hasConstantPrefix(call.getPathArgument().getALocalSource().asExpr().getExpr()) and
25+
not call.getPathArgument().getALocalSource() =
26+
API::getTopLevelMember("File").getAMethodCall("join")
2727
select call,
2828
"Call to " + call.getName() + " with a non-constant value. Consider replacing it with " +
2929
call.getReplacement() + "."
30+
31+
predicate hasConstantPrefix(Expr e) {
32+
// if it's a format string, then the first argument is not a constant string
33+
e.(StringlikeLiteral).getComponent(0) instanceof StringTextComponent
34+
or
35+
// it is not a constant string argument
36+
exists(e.getConstantValue())
37+
or
38+
// not a concatenation that starts with a constant string
39+
hasConstantPrefix(e.(AddExpr).getLeftOperand())
40+
}

‎ruby/ql/test/library-tests/dataflow/local/TaintStep.expected

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2807,13 +2807,14 @@
28072807
| file://:0:0:0:0 | parameter position 0 of File.absolute_path | file://:0:0:0:0 | [summary] to write: return (return) in File.absolute_path |
28082808
| file://:0:0:0:0 | parameter position 0 of File.dirname | file://:0:0:0:0 | [summary] to write: return (return) in File.dirname |
28092809
| file://:0:0:0:0 | parameter position 0 of File.expand_path | file://:0:0:0:0 | [summary] to write: return (return) in File.expand_path |
2810+
| file://:0:0:0:0 | parameter position 0 of File.join | file://:0:0:0:0 | [summary] to write: return (return) in File.join |
28102811
| file://:0:0:0:0 | parameter position 0 of File.path | file://:0:0:0:0 | [summary] to write: return (return) in File.path |
28112812
| file://:0:0:0:0 | parameter position 0 of File.realdirpath | file://:0:0:0:0 | [summary] to write: return (return) in File.realdirpath |
28122813
| file://:0:0:0:0 | parameter position 0 of File.realpath | file://:0:0:0:0 | [summary] to write: return (return) in File.realpath |
28132814
| file://:0:0:0:0 | parameter position 0 of Hash[] | file://:0:0:0:0 | [summary] read: argument position 0.any element in Hash[] |
28142815
| file://:0:0:0:0 | parameter position 0 of String.try_convert | file://:0:0:0:0 | [summary] to write: return (return) in String.try_convert |
28152816
| file://:0:0:0:0 | parameter position 0 of \| | file://:0:0:0:0 | [summary] read: argument position 0.any element in \| |
2816-
| file://:0:0:0:0 | parameter position 0.. of File.join | file://:0:0:0:0 | [summary] to write: return (return) in File.join |
2817+
| file://:0:0:0:0 | parameter position 1.. of File.join | file://:0:0:0:0 | [summary] to write: return (return) in File.join |
28172818
| file://:0:0:0:0 | parameter self of & | file://:0:0:0:0 | [summary] read: argument self.any element in & |
28182819
| file://:0:0:0:0 | parameter self of * | file://:0:0:0:0 | [summary] read: argument self.any element in * |
28192820
| file://:0:0:0:0 | parameter self of - | file://:0:0:0:0 | [summary] read: argument self.any element in - |

‎ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,38 @@ edges
22
| KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:3:12:3:24 | ...[...] : |
33
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:4:10:4:13 | file |
44
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:5:13:5:16 | file |
5+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:6:14:6:17 | file |
6+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:7:16:7:19 | file |
7+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:8:17:8:20 | file |
8+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:9:16:9:19 | file |
9+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:10:18:10:21 | file |
10+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:11:14:11:17 | file |
11+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:13:23:13:26 | file : |
12+
| KernelOpen.rb:3:12:3:24 | ...[...] : | KernelOpen.rb:26:10:26:13 | file |
13+
| KernelOpen.rb:13:23:13:26 | file : | KernelOpen.rb:13:13:13:31 | call to join |
514
nodes
615
| KernelOpen.rb:3:12:3:17 | call to params : | semmle.label | call to params : |
716
| KernelOpen.rb:3:12:3:24 | ...[...] : | semmle.label | ...[...] : |
817
| KernelOpen.rb:4:10:4:13 | file | semmle.label | file |
918
| KernelOpen.rb:5:13:5:16 | file | semmle.label | file |
19+
| KernelOpen.rb:6:14:6:17 | file | semmle.label | file |
20+
| KernelOpen.rb:7:16:7:19 | file | semmle.label | file |
21+
| KernelOpen.rb:8:17:8:20 | file | semmle.label | file |
22+
| KernelOpen.rb:9:16:9:19 | file | semmle.label | file |
23+
| KernelOpen.rb:10:18:10:21 | file | semmle.label | file |
24+
| KernelOpen.rb:11:14:11:17 | file | semmle.label | file |
25+
| KernelOpen.rb:13:13:13:31 | call to join | semmle.label | call to join |
26+
| KernelOpen.rb:13:23:13:26 | file : | semmle.label | file : |
27+
| KernelOpen.rb:26:10:26:13 | file | semmle.label | file |
1028
subpaths
1129
#select
1230
| KernelOpen.rb:4:10:4:13 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:4:10:4:13 | file | This call to Kernel.open depends on a $@. Consider replacing it with File.open. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
1331
| KernelOpen.rb:5:13:5:16 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:5:13:5:16 | file | This call to IO.read depends on a $@. Consider replacing it with File.read. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
32+
| KernelOpen.rb:6:14:6:17 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:6:14:6:17 | file | This call to IO.write depends on a $@. Consider replacing it with File.write. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
33+
| KernelOpen.rb:7:16:7:19 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:7:16:7:19 | file | This call to IO.binread depends on a $@. Consider replacing it with File.binread. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
34+
| KernelOpen.rb:8:17:8:20 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:8:17:8:20 | file | This call to IO.binwrite depends on a $@. Consider replacing it with File.binwrite. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
35+
| KernelOpen.rb:9:16:9:19 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:9:16:9:19 | file | This call to IO.foreach depends on a $@. Consider replacing it with File.foreach. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
36+
| KernelOpen.rb:10:18:10:21 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:10:18:10:21 | file | This call to IO.readlines depends on a $@. Consider replacing it with File.readlines. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
37+
| KernelOpen.rb:11:14:11:17 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:11:14:11:17 | file | This call to URI.open depends on a $@. Consider replacing it with URI(<uri>).open. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
38+
| KernelOpen.rb:13:13:13:31 | call to join | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:13:13:13:31 | call to join | This call to IO.read depends on a $@. Consider replacing it with File.read. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
39+
| KernelOpen.rb:26:10:26:13 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:26:10:26:13 | file | This call to Kernel.open depends on a $@. Consider replacing it with File.open. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,15 @@ def create
33
file = params[:file]
44
open(file) # BAD
55
IO.read(file) # BAD
6+
IO.write(file) # BAD
7+
IO.binread(file) # BAD
8+
IO.binwrite(file) # BAD
9+
IO.foreach(file) # BAD
10+
IO.readlines(file) # BAD
11+
URI.open(file) # BAD
12+
13+
IO.read(File.join(file, "")) # BAD - file as first argument to File.join
14+
IO.read(File.join("", file)) # GOOD - file path is sanitised by guard
615

716
File.open(file).read # GOOD
817

@@ -13,5 +22,7 @@ def create
1322
if %w(some/const/1.txt some/const/2.txt).include? file
1423
IO.read(file) # GOOD - file path is sanitised by guard
1524
end
25+
26+
open(file) # BAD - sanity check to verify that file was not mistakenly marked as sanitized
1627
end
1728
end

‎ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.expected

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
11
| NonConstantKernelOpen.rb:4:5:4:14 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
22
| NonConstantKernelOpen.rb:5:5:5:17 | call to read | Call to IO.read with a non-constant value. Consider replacing it with File.read. |
3-
| NonConstantKernelOpen.rb:9:5:9:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
4-
| NonConstantKernelOpen.rb:19:5:19:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
3+
| NonConstantKernelOpen.rb:6:5:6:18 | call to write | Call to IO.write with a non-constant value. Consider replacing it with File.write. |
4+
| NonConstantKernelOpen.rb:7:5:7:20 | call to binread | Call to IO.binread with a non-constant value. Consider replacing it with File.binread. |
5+
| NonConstantKernelOpen.rb:8:5:8:21 | call to binwrite | Call to IO.binwrite with a non-constant value. Consider replacing it with File.binwrite. |
6+
| NonConstantKernelOpen.rb:9:5:9:20 | call to foreach | Call to IO.foreach with a non-constant value. Consider replacing it with File.foreach. |
7+
| NonConstantKernelOpen.rb:10:5:10:22 | call to readlines | Call to IO.readlines with a non-constant value. Consider replacing it with File.readlines. |
8+
| NonConstantKernelOpen.rb:11:5:11:18 | call to open | Call to URI.open with a non-constant value. Consider replacing it with URI(<uri>).open. |
9+
| NonConstantKernelOpen.rb:15:5:15:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
10+
| NonConstantKernelOpen.rb:25:5:25:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
11+
| NonConstantKernelOpen.rb:33:5:33:14 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |

0 commit comments

Comments
 (0)