Skip to content

Commit d8752a0

Browse files
p-p-
committed
Add additional sinks to the rb/kernel-open query
1 parent 53b86fd commit d8752a0

File tree

6 files changed

+94
-6
lines changed

6 files changed

+94
-6
lines changed

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

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,63 @@ class AmbiguousPathCall extends DataFlow::CallNode {
1919
this = API::getTopLevelMember("IO").getAMethodCall("read") and
2020
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
2121
name = "IO.read"
22+
or
23+
this = API::getTopLevelMember("IO").getAMethodCall("write") and
24+
name = "IO.write"
25+
or
26+
this = API::getTopLevelMember("IO").getAMethodCall("binread") and
27+
name = "IO.binread"
28+
or
29+
this = API::getTopLevelMember("IO").getAMethodCall("binwrite") and
30+
name = "IO.binwrite"
31+
or
32+
this = API::getTopLevelMember("IO").getAMethodCall("foreach") and
33+
name = "IO.foreach"
34+
or
35+
this = API::getTopLevelMember("IO").getAMethodCall("readlines") and
36+
name = "IO.readlines"
37+
or
38+
this = API::getTopLevelMember("URI").getAMethodCall("open") and
39+
name = "URI.open"
2240
}
2341

2442
/** Gets the name for the method being called. */
2543
string getName() { result = name }
2644

2745
/** Gets the name for a safer method that can be used instead. */
2846
string getReplacement() {
47+
result = "File.open" and name = "Kernel.open"
48+
or
2949
result = "File.read" and name = "IO.read"
3050
or
31-
result = "File.open" and name = "Kernel.open"
51+
result = "File.write" and name = "IO.write"
52+
or
53+
result = "File.binread" and name = "IO.binread"
54+
or
55+
result = "File.binwrite" and name = "IO.binwrite"
56+
or
57+
result = "File.foreach" and name = "IO.foreach"
58+
or
59+
result = "File.readlines" and name = "IO.readlines"
60+
or
61+
result = "URI(<uri>).open" and name = "URI.open"
3262
}
3363

3464
/** Gets the argument that specifies the path to be accessed. */
3565
DataFlow::Node getPathArgument() { result = this.getArgument(0) }
3666
}
67+
68+
/**
69+
* A sanitizer for kernel open vulnerabilities.
70+
*/
71+
abstract class Sanitizer extends DataFlow::Node { }
72+
73+
/**
74+
* If a File.join is performed the resulting string will not start with a pipe |
75+
* This is true as long the tainted data doesn't flow into the first argument.
76+
*/
77+
private class FileJoinCall extends Sanitizer, DataFlow::ExprNode {
78+
FileJoinCall() {
79+
this = API::getTopLevelMember("File").getAMethodCall("join").getArgument(any(int i | i > 0))
80+
}
81+
}

‎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(<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/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,35 @@ 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:13:23:13:26 | file : | KernelOpen.rb:13:13:13:31 | call to join |
513
nodes
614
| KernelOpen.rb:3:12:3:17 | call to params : | semmle.label | call to params : |
715
| KernelOpen.rb:3:12:3:24 | ...[...] : | semmle.label | ...[...] : |
816
| KernelOpen.rb:4:10:4:13 | file | semmle.label | file |
917
| KernelOpen.rb:5:13:5:16 | file | semmle.label | file |
18+
| KernelOpen.rb:6:14:6:17 | file | semmle.label | file |
19+
| KernelOpen.rb:7:16:7:19 | file | semmle.label | file |
20+
| KernelOpen.rb:8:17:8:20 | file | semmle.label | file |
21+
| KernelOpen.rb:9:16:9:19 | file | semmle.label | file |
22+
| KernelOpen.rb:10:18:10:21 | file | semmle.label | file |
23+
| KernelOpen.rb:11:14:11:17 | file | semmle.label | file |
24+
| KernelOpen.rb:13:13:13:31 | call to join | semmle.label | call to join |
25+
| KernelOpen.rb:13:23:13:26 | file : | semmle.label | file : |
1026
subpaths
1127
#select
1228
| 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 |
1329
| 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 |
30+
| 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 |
31+
| 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 |
32+
| 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 |
33+
| 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 |
34+
| 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 |
35+
| 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 |
36+
| 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 |

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

Lines changed: 9 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

0 commit comments

Comments
 (0)