RB: add a query flagging uses of Kernel.open() that are not with a constant string

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

@@ -0,0 +1,36 @@
+/**
+ * Provides utility classes and predicates for reasoning about `Kernel.open` and related methods.
+ */
+
+private import codeql.ruby.AST
+private import codeql.ruby.DataFlow
+private import codeql.ruby.AST
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.frameworks.core.Kernel::Kernel
+
+/** A call to a method that might access a file or start a process. */
+class AmbiguousPathCall extends DataFlow::CallNode {
+  string name;
+
+  AmbiguousPathCall() {
+    this.(KernelMethodCall).getMethodName() = "open" and
+    name = "Kernel.open"
+    or
+    this = API::getTopLevelMember("IO").getAMethodCall("read") and
+    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
+    name = "IO.read"
+  }
+
+  /** Gets the name for the method being called. */
+  string getName() { result = name }
+
+  /** Gets the name for a safer method that can be used instead. */
+  string getReplacement() {
+    result = "File.read" and name = "IO.read"
+    or
+    result = "File.open" and name = "Kernel.open"
+  }
+
+  /** Gets the argument that specifies the path to be accessed. */
+  DataFlow::Node getPathArgument() { result = this.getArgument(0) }
+}

ruby/ql/src/change-notes/2022-10-06-non-constant-kernel-open.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* 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

@@ -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>

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

@@ -1,46 +1,4 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
+<!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>
+  <include src="KernelOpen.inc.qhelp" />
+</qhelp>

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

@@ -1,5 +1,5 @@
 /**
- * @name Use of `Kernel.open` or `IO.read`
+ * @name Use of `Kernel.open` or `IO.read` with user-controlled input
  * @description Using `Kernel.open` or `IO.read` may allow a malicious
  *              user to execute arbitrary system commands.
  * @kind path-problem
@@ -14,49 +14,20 @@
  *       external/cwe/cwe-073
  */
 
-import codeql.ruby.AST
-import codeql.ruby.ApiGraphs
-import codeql.ruby.frameworks.core.Kernel::Kernel
+import codeql.ruby.DataFlow
 import codeql.ruby.TaintTracking
-import codeql.ruby.dataflow.BarrierGuards
 import codeql.ruby.dataflow.RemoteFlowSources
-import codeql.ruby.DataFlow
+import codeql.ruby.dataflow.BarrierGuards
 import DataFlow::PathGraph
-
-/**
- * A method call that has a suggested replacement.
- */
-abstract class Replacement extends DataFlow::CallNode {
-  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 DataFlow::CallNode, Replacement {
-  IOReadCall() { this = API::getTopLevelMember("IO").getAMethodCall("read") }
-
-  override string getFrom() { result = "IO.read" }
-
-  override string getTo() { result = "File.read" }
-}
+import codeql.ruby.security.KernelOpenQuery
 
 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)
-    or
-    exists(IOReadCall c | c.getArgument(0) = sink)
+    sink = any(AmbiguousPathCall r).getPathArgument()
   }
 
   override predicate isSanitizer(DataFlow::Node node) {
@@ -73,5 +44,6 @@ where
   sourceNode = source.getNode() and
   call.getArgument(0) = sink.getNode()
 select sink.getNode(), source, sink,
-  "This call to " + call.(Replacement).getFrom() + " depends on a $@. Replace it with " +
-    call.(Replacement).getTo() + ".", source.getNode(), "user-provided value"
+  "This call to " + call.(AmbiguousPathCall).getName() +
+    " depends on a $@. Consider replacing it with " + call.(AmbiguousPathCall).getReplacement() +
+    ".", source.getNode(), "user-provided value"

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

@@ -0,0 +1,4 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+  <include src="KernelOpen.inc.qhelp" />
+</qhelp>

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

@@ -0,0 +1,29 @@
+/**
+ * @name Use of `Kernel.open` or `IO.read` with a non-constant value
+ * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ *              user to execute arbitrary system commands.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 6.5
+ * @precision high
+ * @id rb/non-constant-kernel-open
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ *       external/cwe/cwe-073
+ */
+
+import codeql.ruby.security.KernelOpenQuery
+import codeql.ruby.ast.Literal
+
+from AmbiguousPathCall call
+where
+  // there is not a constant string argument
+  not exists(call.getPathArgument().asExpr().getExpr().getConstantValue()) and
+  // if it's a format string, then the first argument is not a constant string
+  not call.getPathArgument().getALocalSource().asExpr().getExpr().(StringLiteral).getComponent(0)
+    instanceof StringTextComponent
+select call,
+  "Call to " + call.getName() + " with a non-constant value. Consider replacing it with " +
+    call.getReplacement() + "."

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

@@ -9,5 +9,5 @@ nodes
 | 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 $@. Replace it with File.open. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
-| 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 $@. Replace it with File.read. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value |
+| 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 |
+| 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 |

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

@@ -0,0 +1,4 @@
+| NonConstantKernelOpen.rb:4:5:4:14 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:5:5:5:17 | call to read | Call to IO.read with a non-constant value. Consider replacing it with File.read. |
+| NonConstantKernelOpen.rb:9:5:9:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:19:5:19:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |

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

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

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

@@ -0,0 +1,23 @@
+class UsersController < ActionController::Base
+  def create
+    file = params[:file]
+    open(file) # BAD
+    IO.read(file) # BAD
+
+    File.open(file).read # GOOD
+
+    Kernel.open(file) # BAD
+
+    File.open(file, "r") # GOOD
+
+    Kernel.open("constant") # GOOD
+
+    IO.read("constant") # GOOD
+
+    Kernel.open("this is #{fine}") # GOOD
+
+    Kernel.open("#{this_is} bad") # BAD
+
+    open("| #{this_is_an_explicit_command} foo bar") # GOOD
+  end
+end