Merge pull request #11366 from p-/p--ruby-kernel-open-addition

Ruby: Add additional sinks to the `rb/kernel-open` query

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

@@ -133,7 +133,7 @@ module File {
     }
 
     override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
-      input = "Argument[0..]" and
+      input = "Argument[0,1..]" and
       output = "ReturnValue" and
       preservesValue = false
     }

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

@@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow
 private import codeql.ruby.AST
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.frameworks.core.Kernel::Kernel
+private import codeql.ruby.frameworks.Files
 
 /** A call to a method that might access a file or start a process. */
 class AmbiguousPathCall extends DataFlow::CallNode {
@@ -16,9 +17,15 @@ class AmbiguousPathCall extends DataFlow::CallNode {
     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"
+    exists(string methodName |
+      methodName = ["read", "write", "binread", "binwrite", "foreach", "readlines"]
+    |
+      methodCallOnlyOnIO(this, methodName) and
+      name = "IO." + methodName
+    )
+    or
+    this = API::getTopLevelMember("URI").getAMethodCall("open") and
+    name = "URI.open"
   }
 
   /** Gets the name for the method being called. */
@@ -26,11 +33,41 @@ class AmbiguousPathCall extends DataFlow::CallNode {
 
   /** Gets the name for a safer method that can be used instead. */
   string getReplacement() {
+    result = "File.open" and name = "Kernel.open"
+    or
     result = "File.read" and name = "IO.read"
     or
-    result = "File.open" and name = "Kernel.open"
+    result = "File.write" and name = "IO.write"
+    or
+    result = "File.binread" and name = "IO.binread"
+    or
+    result = "File.binwrite" and name = "IO.binwrite"
+    or
+    result = "File.foreach" and name = "IO.foreach"
+    or
+    result = "File.readlines" and name = "IO.readlines"
+    or
+    result = "URI(<uri>).open" and name = "URI.open"
   }
 
   /** Gets the argument that specifies the path to be accessed. */
   DataFlow::Node getPathArgument() { result = this.getArgument(0) }
 }
+
+private predicate methodCallOnlyOnIO(DataFlow::CallNode node, string methodName) {
+  node = API::getTopLevelMember("IO").getAMethodCall(methodName) and
+  not node = API::getTopLevelMember("File").getAMethodCall(methodName) // needed in e.g. opal/opal, where some calls have both paths (opal implements an own corelib)
+}
+
+/**
+ * A sanitizer for kernel open vulnerabilities.
+ */
+abstract class Sanitizer extends DataFlow::Node { }
+
+/**
+ * If a `File.join` is performed the resulting string will not start with a pipe `|`.
+ * This is true as long the tainted data doesn't flow into the first argument.
+ */
+private class FileJoinSanitizer extends Sanitizer {
+  FileJoinSanitizer() { this = any(File::FileJoinSummary s).getParameter("1..") }
+}

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

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

@@ -6,15 +6,19 @@
 <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>.
+The same vulnerability applies to <code>IO.read</code>, <code>IO.write</code>,
+<code>IO.binread</code>, <code>IO.binwrite</code>, <code>IO.foreach</code>,
+<code>IO.readlines</code> and <code>URI.open</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
+does not have this vulnerability. Similarly, use the methods from the <code>File</code>
+class instead of the <code>IO</code> class e.g. <code>File.read</code> instead
 of <code>IO.read</code>.</p>
+<p>Instead of <code>URI.open</code> use <code>URI(..).open</code> or an HTTP Client.</p>
 
 </recommendation>
 <example>
@@ -36,6 +40,7 @@ user-supplied file path.
 <li>
 OWASP:
 <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Ruby_on_Rails_Cheat_Sheet.html#command-injection">Ruby on Rails Cheat Sheet: Command Injection</a>.
 </li>
 
 <li>

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

@@ -1,6 +1,7 @@
 /**
- * @name Use of `Kernel.open` or `IO.read` with user-controlled input
- * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ * @name Use of `Kernel.open`, `IO.read` or similar sinks with user-controlled input
+ * @description Using `Kernel.open`, `IO.read`, `IO.write`, `IO.binread`, `IO.binwrite`,
+ *              `IO.foreach`, `IO.readlines`, or `URI.open` may allow a malicious
  *              user to execute arbitrary system commands.
  * @kind path-problem
  * @problem.severity error
@@ -32,7 +33,8 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) {
     node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier
+    node instanceof StringConstArrayInclusionCallBarrier or
+    node instanceof Sanitizer
   }
 }
 

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

@@ -1,6 +1,7 @@
 /**
- * @name Use of `Kernel.open` or `IO.read` with a non-constant value
- * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ * @name Use of `Kernel.open` or `IO.read` or similar sinks with a non-constant value
+ * @description Using `Kernel.open`, `IO.read`, `IO.write`, `IO.binread`, `IO.binwrite`,
+ *              `IO.foreach`, `IO.readlines`, or `URI.open` may allow a malicious
  *              user to execute arbitrary system commands.
  * @kind problem
  * @problem.severity warning
@@ -15,15 +16,25 @@
  */
 
 import codeql.ruby.security.KernelOpenQuery
-import codeql.ruby.ast.Literal
+import codeql.ruby.AST
+import codeql.ruby.ApiGraphs
 
 from AmbiguousPathCall call
 where
-  // there is not a constant string argument
-  not exists(call.getPathArgument().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
+  not hasConstantPrefix(call.getPathArgument().getALocalSource().asExpr().getExpr()) and
+  not call.getPathArgument().getALocalSource() =
+    API::getTopLevelMember("File").getAMethodCall("join")
 select call,
   "Call to " + call.getName() + " with a non-constant value. Consider replacing it with " +
     call.getReplacement() + "."
+
+predicate hasConstantPrefix(Expr e) {
+  // if it's a format string, then the first argument is not a constant string
+  e.(StringlikeLiteral).getComponent(0) instanceof StringTextComponent
+  or
+  // it is not a constant string argument
+  exists(e.getConstantValue())
+  or
+  // not a concatenation that starts with a constant string
+  hasConstantPrefix(e.(AddExpr).getLeftOperand())
+}

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

@@ -2807,13 +2807,14 @@
 | 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 |
 | file://:0:0:0:0 | parameter position 0 of File.dirname | file://:0:0:0:0 | [summary] to write: return (return) in File.dirname |
 | 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 |
+| file://:0:0:0:0 | parameter position 0 of File.join | file://:0:0:0:0 | [summary] to write: return (return) in File.join |
 | file://:0:0:0:0 | parameter position 0 of File.path | file://:0:0:0:0 | [summary] to write: return (return) in File.path |
 | file://:0:0:0:0 | parameter position 0 of File.realdirpath | file://:0:0:0:0 | [summary] to write: return (return) in File.realdirpath |
 | file://:0:0:0:0 | parameter position 0 of File.realpath | file://:0:0:0:0 | [summary] to write: return (return) in File.realpath |
 | file://:0:0:0:0 | parameter position 0 of Hash[] | file://:0:0:0:0 | [summary] read: argument position 0.any element in Hash[] |
 | 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 |
 | file://:0:0:0:0 | parameter position 0 of \| | file://:0:0:0:0 | [summary] read: argument position 0.any element in \| |
-| file://:0:0:0:0 | parameter position 0.. of File.join | file://:0:0:0:0 | [summary] to write: return (return) in File.join |
+| file://:0:0:0:0 | parameter position 1.. of File.join | file://:0:0:0:0 | [summary] to write: return (return) in File.join |
 | file://:0:0:0:0 | parameter self of & | file://:0:0:0:0 | [summary] read: argument self.any element in & |
 | file://:0:0:0:0 | parameter self of * | file://:0:0:0:0 | [summary] read: argument self.any element in * |
 | 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

@@ -2,12 +2,38 @@ edges
 | KernelOpen.rb:3:12:3:17 | call to params :  | KernelOpen.rb:3:12:3:24 | ...[...] :  |
 | KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:4:10:4:13 | file |
 | KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:5:13:5:16 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:6:14:6:17 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:7:16:7:19 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:8:17:8:20 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:9:16:9:19 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:10:18:10:21 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:11:14:11:17 | file |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:13:23:13:26 | file :  |
+| KernelOpen.rb:3:12:3:24 | ...[...] :  | KernelOpen.rb:26:10:26:13 | file |
+| KernelOpen.rb:13:23:13:26 | file :  | KernelOpen.rb:13:13:13:31 | call to join |
 nodes
 | KernelOpen.rb:3:12:3:17 | call to params :  | semmle.label | call to params :  |
 | KernelOpen.rb:3:12:3:24 | ...[...] :  | semmle.label | ...[...] :  |
 | KernelOpen.rb:4:10:4:13 | file | semmle.label | file |
 | KernelOpen.rb:5:13:5:16 | file | semmle.label | file |
+| KernelOpen.rb:6:14:6:17 | file | semmle.label | file |
+| KernelOpen.rb:7:16:7:19 | file | semmle.label | file |
+| KernelOpen.rb:8:17:8:20 | file | semmle.label | file |
+| KernelOpen.rb:9:16:9:19 | file | semmle.label | file |
+| KernelOpen.rb:10:18:10:21 | file | semmle.label | file |
+| KernelOpen.rb:11:14:11:17 | file | semmle.label | file |
+| KernelOpen.rb:13:13:13:31 | call to join | semmle.label | call to join |
+| KernelOpen.rb:13:23:13:26 | file :  | semmle.label | file :  |
+| KernelOpen.rb:26:10:26:13 | 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 $@. 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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

@@ -3,6 +3,15 @@ class UsersController < ActionController::Base
     file = params[:file]
     open(file) # BAD
     IO.read(file) # BAD
+    IO.write(file) # BAD
+    IO.binread(file) # BAD
+    IO.binwrite(file) # BAD
+    IO.foreach(file) # BAD
+    IO.readlines(file) # BAD
+    URI.open(file) # BAD
+
+    IO.read(File.join(file, "")) # BAD - file as first argument to File.join 
+    IO.read(File.join("", file)) # GOOD - file path is sanitised by guard
 
     File.open(file).read # GOOD
 
@@ -13,5 +22,7 @@ class UsersController < ActionController::Base
     if %w(some/const/1.txt some/const/2.txt).include? file
       IO.read(file) # GOOD - file path is sanitised by guard
     end
+
+    open(file) # BAD - sanity check to verify that file was not mistakenly marked as sanitized
   end
 end

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

@@ -1,4 +1,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. |
 | 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. |
+| NonConstantKernelOpen.rb:6:5:6:18 | call to write | Call to IO.write with a non-constant value. Consider replacing it with File.write. |
+| NonConstantKernelOpen.rb:7:5:7:20 | call to binread | Call to IO.binread with a non-constant value. Consider replacing it with File.binread. |
+| NonConstantKernelOpen.rb:8:5:8:21 | call to binwrite | Call to IO.binwrite with a non-constant value. Consider replacing it with File.binwrite. |
+| NonConstantKernelOpen.rb:9:5:9:20 | call to foreach | Call to IO.foreach with a non-constant value. Consider replacing it with File.foreach. |
+| NonConstantKernelOpen.rb:10:5:10:22 | call to readlines | Call to IO.readlines with a non-constant value. Consider replacing it with File.readlines. |
+| 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. |
+| NonConstantKernelOpen.rb:15:5:15:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:25:5:25:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:33:5:33:14 | 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.rb

@@ -3,6 +3,12 @@ class UsersController < ActionController::Base
     file = params[:file]
     open(file) # BAD
     IO.read(file) # BAD
+    IO.write(file) # BAD
+    IO.binread(file) # BAD
+    IO.binwrite(file) # BAD
+    IO.foreach(file) # BAD
+    IO.readlines(file) # BAD
+    URI.open(file) # BAD
 
     File.open(file).read # GOOD
 
@@ -19,5 +25,11 @@ class UsersController < ActionController::Base
     Kernel.open("#{this_is} bad") # BAD
 
     open("| #{this_is_an_explicit_command} foo bar") # GOOD
+
+    IO.foreach("|" + EnvUtil.rubybin + " -e 'puts :foo; puts :bar; puts :baz'") {|x| a << x } # GOOD
+
+    IO.write(File.join("foo", "bar.txt"), "bar") # GOOD
+
+    open(file) # BAD - sanity check to verify that file was not mistakenly marked as sanitized
   end
 end