Merge pull request #318 from github/hmac-open-query

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

Author: hmac

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

@@ -36,13 +36,10 @@ private DataFlow::Node fileInstanceInstantiation() {
   result = API::getTopLevelMember("File").getAMethodCall("open")
   or
   // Calls to `Kernel.open` can yield `File` instances
-  exists(KernelMethodCall c |
-    c = result.asExpr().getExpr() and
-    c.getMethodName() = "open" and
-    // Assume that calls that don't invoke shell commands will instead open
-    // a file.
-    not pathArgSpawnsSubprocess(c.getArgument(0))
-  )
+  result.(KernelMethodCall).getMethodName() = "open" and
+  // Assume that calls that don't invoke shell commands will instead open
+  // a file.
+  not pathArgSpawnsSubprocess(result.(KernelMethodCall).getArgument(0).asExpr().getExpr())
 }
 
 private DataFlow::Node fileInstance() {

ql/lib/codeql/ruby/frameworks/StandardLibrary.qll

@@ -8,17 +8,27 @@ private import codeql.ruby.ApiGraphs
  * in every Ruby object. In addition, its module methods can be called by
  * providing a specific receiver as in `Kernel.exit`.
  */
-class KernelMethodCall extends MethodCall {
+class KernelMethodCall extends DataFlow::CallNode {
+  private MethodCall methodCall;
+
   KernelMethodCall() {
-    this = API::getTopLevelMember("Kernel").getAMethodCall(_).asExpr().getExpr()
-    or
-    this instanceof UnknownMethodCall and
+    methodCall = this.asExpr().getExpr() and
     (
-      this.getReceiver() instanceof Self and isPrivateKernelMethod(this.getMethodName())
+      this = API::getTopLevelMember("Kernel").getAMethodCall(_)
       or
-      isPublicKernelMethod(this.getMethodName())
+      methodCall instanceof UnknownMethodCall and
+      (
+        this.getReceiver().asExpr().getExpr() instanceof Self and
+        isPrivateKernelMethod(methodCall.getMethodName())
+        or
+        isPublicKernelMethod(methodCall.getMethodName())
+      )
     )
   }
+
+  string getMethodName() { result = methodCall.getMethodName() }
+
+  int getNumberOfArguments() { result = methodCall.getNumberOfArguments() }
 }
 
 /**
@@ -161,19 +171,14 @@ class SubshellHeredocExecution extends SystemCommandExecution::Range {
  * ```
  * Ruby documentation: https://docs.ruby-lang.org/en/3.0.0/Kernel.html#method-i-system
  */
-class KernelSystemCall extends SystemCommandExecution::Range {
-  KernelMethodCall methodCall;
+class KernelSystemCall extends SystemCommandExecution::Range, KernelMethodCall {
+  KernelSystemCall() { this.getMethodName() = "system" }
 
-  KernelSystemCall() {
-    methodCall.getMethodName() = "system" and
-    this.asExpr().getExpr() = methodCall
-  }
-
-  override DataFlow::Node getAnArgument() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+  override DataFlow::Node getAnArgument() { result = this.getArgument(_) }
 
   override predicate isShellInterpreted(DataFlow::Node arg) {
     // Kernel.system invokes a subshell if you provide a single string as argument
-    methodCall.getNumberOfArguments() = 1 and arg.asExpr().getExpr() = methodCall.getAnArgument()
+    this.getNumberOfArguments() = 1 and arg = getAnArgument()
   }
 }
 
@@ -182,19 +187,14 @@ class KernelSystemCall extends SystemCommandExecution::Range {
  * `Kernel.exec` takes the same argument forms as `Kernel.system`. See `KernelSystemCall` for details.
  * Ruby documentation: https://docs.ruby-lang.org/en/3.0.0/Kernel.html#method-i-exec
  */
-class KernelExecCall extends SystemCommandExecution::Range {
-  KernelMethodCall methodCall;
+class KernelExecCall extends SystemCommandExecution::Range, KernelMethodCall {
+  KernelExecCall() { this.getMethodName() = "exec" }
 
-  KernelExecCall() {
-    methodCall.getMethodName() = "exec" and
-    this.asExpr().getExpr() = methodCall
-  }
-
-  override DataFlow::Node getAnArgument() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+  override DataFlow::Node getAnArgument() { result = this.getArgument(_) }
 
   override predicate isShellInterpreted(DataFlow::Node arg) {
     // Kernel.exec invokes a subshell if you provide a single string as argument
-    methodCall.getNumberOfArguments() = 1 and arg.asExpr().getExpr() = methodCall.getAnArgument()
+    this.getNumberOfArguments() = 1 and arg = getAnArgument()
   }
 }
 
@@ -208,19 +208,14 @@ class KernelExecCall extends SystemCommandExecution::Range {
  * spawn([env,] command... [,options]) -> pid
  * ```
  */
-class KernelSpawnCall extends SystemCommandExecution::Range {
-  KernelMethodCall methodCall;
+class KernelSpawnCall extends SystemCommandExecution::Range, KernelMethodCall {
+  KernelSpawnCall() { this.getMethodName() = "spawn" }
 
-  KernelSpawnCall() {
-    methodCall.getMethodName() = "spawn" and
-    this.asExpr().getExpr() = methodCall
-  }
-
-  override DataFlow::Node getAnArgument() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+  override DataFlow::Node getAnArgument() { result = this.getArgument(_) }
 
   override predicate isShellInterpreted(DataFlow::Node arg) {
     // Kernel.spawn invokes a subshell if you provide a single string as argument
-    methodCall.getNumberOfArguments() = 1 and arg.asExpr().getExpr() = methodCall.getAnArgument()
+    this.getNumberOfArguments() = 1 and arg = getAnArgument()
   }
 }
 
@@ -284,14 +279,10 @@ class Open3PipelineCall extends SystemCommandExecution::Range {
  * a # => 2
  * ```
  */
-class EvalCallCodeExecution extends CodeExecution::Range {
-  KernelMethodCall methodCall;
-
-  EvalCallCodeExecution() {
-    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "eval"
-  }
+class EvalCallCodeExecution extends CodeExecution::Range, KernelMethodCall {
+  EvalCallCodeExecution() { this.getMethodName() = "eval" }
 
-  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
+  override DataFlow::Node getCode() { result = this.getArgument(0) }
 }
 
 /**
@@ -302,51 +293,41 @@ class EvalCallCodeExecution extends CodeExecution::Range {
  * arr # => [1]
  * ```
  */
-class SendCallCodeExecution extends CodeExecution::Range {
-  KernelMethodCall methodCall;
+class SendCallCodeExecution extends CodeExecution::Range, KernelMethodCall {
+  SendCallCodeExecution() { this.getMethodName() = "send" }
 
-  SendCallCodeExecution() {
-    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "send"
-  }
-
-  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
+  override DataFlow::Node getCode() { result = this.getArgument(0) }
 }
 
 /**
  * A call to `BasicObject#instance_eval`, which executes its first argument as Ruby code.
  */
-class InstanceEvalCallCodeExecution extends CodeExecution::Range {
-  BasicObjectInstanceMethodCall methodCall;
-
+class InstanceEvalCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode {
   InstanceEvalCallCodeExecution() {
-    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "instance_eval"
+    this.asExpr().getExpr().(UnknownMethodCall).getMethodName() = "instance_eval"
   }
 
-  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
+  override DataFlow::Node getCode() { result = this.getArgument(0) }
 }
 
 /**
  * A call to `Module#class_eval`, which executes its first argument as Ruby code.
  */
-class ClassEvalCallCodeExecution extends CodeExecution::Range {
-  UnknownMethodCall methodCall;
-
+class ClassEvalCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode {
   ClassEvalCallCodeExecution() {
-    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "class_eval"
+    this.asExpr().getExpr().(UnknownMethodCall).getMethodName() = "class_eval"
   }
 
-  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
+  override DataFlow::Node getCode() { result = this.getArgument(0) }
 }
 
 /**
  * A call to `Module#module_eval`, which executes its first argument as Ruby code.
  */
-class ModuleEvalCallCodeExecution extends CodeExecution::Range {
-  UnknownMethodCall methodCall;
-
+class ModuleEvalCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode {
   ModuleEvalCallCodeExecution() {
-    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "module_eval"
+    this.asExpr().getExpr().(UnknownMethodCall).getMethodName() = "module_eval"
   }
 
-  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
+  override DataFlow::Node getCode() { result = this.getArgument(0) }
 }

ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll

@@ -26,7 +26,7 @@ class OpenURIRequest extends HTTP::Client::Request::Range {
     or
     // Kernel.open("http://example.com").read
     // open("http://example.com").read
-    this instanceof KernelMethodCall and
+    request instanceof KernelMethodCall and
     this.getMethodName() = "open" and
     request.asExpr().getExpr() = this and
     responseBody.asExpr().getExpr().(MethodCall).getMethodName() in ["read", "readlines"] and

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,76 @@
+/**
+ * @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
+ *       external/cwe/cwe-073
+ */
+
+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 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" }
+}
+
+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)
+  }
+
+  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, DataFlow::CallNode call
+where
+  config.hasFlowPath(source, sink) and
+  sourceNode = source.getNode() and
+  call.asExpr().getExpr().(MethodCall).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