Add a query for uses of Kernel.open and IO.read

Author: hmac

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

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
+character, it will execute the remaining string as a shell command. If a
+malicious user can control the file name, they can execute arbitrary code.
+The same vulnerability applies to <code>IO.read</code>.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Use <code>File.open</code> instead of <code>Kernel.open</code>, as the former
+does not have this vulnerability. Similarly, use <code>File.read</code> instead
+of <code>IO.read</code>.</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example shows code that calls <code>Kernel.open</code> on a
+user-supplied file path.
+</p>
+
+<sample src="examples/kernel_open.rb" />
+
+<p>Instead, <code>File.open</code> should be used, as in the following example.</p>
+
+<sample src="examples/file_open.rb" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+
+<li>
+Example CVE: <a href="https://www.ruby-lang.org/en/news/2021/05/02/os-command-injection-in-rdoc/">Command Injection in RDoc</a>.
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,75 @@
+/**
+ * @name Use of `Kernel.open` or `IO.read`
+ * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ *              user to execute arbitrary system commands.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.8
+ * @precision high
+ * @id rb/kernel-open
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ */
+
+import ruby
+import codeql.ruby.ApiGraphs
+import codeql.ruby.frameworks.StandardLibrary
+import codeql.ruby.TaintTracking
+import codeql.ruby.dataflow.BarrierGuards
+import codeql.ruby.dataflow.RemoteFlowSources
+import DataFlow::PathGraph
+
+/**
+ * Method calls that have a suggested replacement.
+ */
+abstract class Replacement extends MethodCall {
+  abstract string getFrom();
+
+  abstract string getTo();
+}
+
+class KernelOpenCall extends KernelMethodCall, Replacement {
+  KernelOpenCall() { this.getMethodName() = "open" }
+
+  override string getFrom() { result = "Kernel.open" }
+
+  override string getTo() { result = "File.open" }
+}
+
+class IOReadCall extends MethodCall, Replacement {
+  IOReadCall() { this = API::getTopLevelMember("IO").getAMethodCall("read").asExpr().getExpr() }
+
+  override string getFrom() { result = "IO.read" }
+
+  override string getTo() { result = "File.read" }
+}
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "KernelOpen" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(KernelOpenCall c | c.getArgument(0) = sink.asExpr().getExpr())
+    or
+    exists(IOReadCall c | c.getArgument(0) = sink.asExpr().getExpr())
+  }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof StringConstCompare or
+    guard instanceof StringConstArrayInclusionCall
+  }
+}
+
+from
+  Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink,
+  DataFlow::Node sourceNode, MethodCall call
+where
+  config.hasFlowPath(source, sink) and
+  sourceNode = source.getNode() and
+  call.getArgument(0) = sink.getNode().asExpr().getExpr()
+select sink.getNode(), source, sink,
+  "This call to " + call.(Replacement).getFrom() +
+    " depends on a user-provided value. Replace it with " + call.(Replacement).getTo() + "."

ql/src/queries/security/cwe-078/examples/file_open.rb

@@ -0,0 +1,6 @@
+class UsersController < ActionController::Base
+    def create
+      filename = params[:filename]
+      File.open(filename)
+    end
+  end  

ql/src/queries/security/cwe-078/examples/kernel_open.rb

@@ -0,0 +1,6 @@
+class UsersController < ActionController::Base
+  def create
+    filename = params[:filename]
+    open(filename) # BAD
+  end
+end  

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

@@ -0,0 +1,11 @@
+edges
+| KernelOpen.rb:3:12:3:17 | call to params :  | KernelOpen.rb:4:10:4:13 | file |
+| KernelOpen.rb:3:12:3:17 | call to params :  | KernelOpen.rb:5:13:5:16 | file |
+nodes
+| KernelOpen.rb:3:12:3:17 | call to params :  | semmle.label | call to params :  |
+| KernelOpen.rb:4:10:4:13 | file | semmle.label | file |
+| KernelOpen.rb:5:13:5:16 | file | semmle.label | file |
+subpaths
+#select
+| 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 user-provided value. Replace it with File.open. |
+| 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 user-provided value. Replace it with File.read. |

ql/test/query-tests/security/cwe-078/KernelOpen.qlref

@@ -0,0 +1 @@
+queries/security/cwe-078/KernelOpen.ql

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

@@ -0,0 +1,17 @@
+class UsersController < ActionController::Base
+  def create
+    file = params[:file]
+    open(file) # BAD
+    IO.read(file) # BAD
+
+    File.open(file).read # GOOD
+
+    if file == "some/const/path.txt"
+      open(file) # GOOD - file path is sanitised by guard
+    end
+
+    if %w(some/const/1.txt some/const/2.txt).include? file
+      IO.read(file) # GOOD - file path is sanitised by guard
+    end
+  end
+end