Merge pull request github#12856 from p-/p--non-constant-open-improvments

Ruby: Add additional sanitizers for Kernel.open or IO.read or similar sinks with a non-constant value

Author: erik-krogh

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

@@ -18,23 +18,41 @@
 import codeql.ruby.security.KernelOpenQuery
 import codeql.ruby.AST
 import codeql.ruby.ApiGraphs
+import codeql.ruby.DataFlow
 
 from AmbiguousPathCall call
 where
-  not hasConstantPrefix(call.getPathArgument().getALocalSource().asExpr().getExpr()) and
+  call.getNumberOfArguments() > 0 and
+  not hasConstantPrefix(call.getPathArgument()) 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) {
+predicate hasConstantPrefix(DataFlow::Node node) {
+  hasConstantPrefix(node.getALocalSource())
+  or
   // if it's a format string, then the first argument is not a constant string
-  e.(StringlikeLiteral).getComponent(0) instanceof StringTextComponent
+  node.asExpr().getExpr().(StringlikeLiteral).getComponent(0) instanceof StringTextComponent
   or
   // it is not a constant string argument
-  exists(e.getConstantValue())
+  exists(node.getConstantValue())
   or
   // not a concatenation that starts with a constant string
-  hasConstantPrefix(e.(AddExpr).getLeftOperand())
+  exists(DataFlow::ExprNode prefix |
+    node.asExpr().getExpr().(AddExpr).getLeftOperand() = prefix.asExpr().getExpr() and
+    hasConstantPrefix(prefix)
+  )
+  or
+  // is a .freeze call on a constant string
+  exists(DataFlow::CallNode call | node = call and call.getMethodName() = "freeze" |
+    hasConstantPrefix(call.getReceiver())
+  )
+  or
+  // is a constant read of a constant string
+  exists(DataFlow::Node constant |
+    constant.asExpr().getExpr() = node.asExpr().getExpr().(ConstantReadAccess).getValue() and
+    hasConstantPrefix(constant)
+  )
 }

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

@@ -1,11 +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: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. |
+| NonConstantKernelOpen.rb:7:5:7:14 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:8:5:8:17 | call to read | Call to IO.read with a non-constant value. Consider replacing it with File.read. |
+| NonConstantKernelOpen.rb:9:5:9:18 | call to write | Call to IO.write with a non-constant value. Consider replacing it with File.write. |
+| NonConstantKernelOpen.rb:10:5:10:20 | call to binread | Call to IO.binread with a non-constant value. Consider replacing it with File.binread. |
+| NonConstantKernelOpen.rb:11:5:11:21 | call to binwrite | Call to IO.binwrite with a non-constant value. Consider replacing it with File.binwrite. |
+| NonConstantKernelOpen.rb:12:5:12:20 | call to foreach | Call to IO.foreach with a non-constant value. Consider replacing it with File.foreach. |
+| NonConstantKernelOpen.rb:13:5:13:22 | call to readlines | Call to IO.readlines with a non-constant value. Consider replacing it with File.readlines. |
+| NonConstantKernelOpen.rb:14:5:14:18 | call to open | Call to URI.open with a non-constant value. Consider replacing it with URI(<uri>).open. |
+| NonConstantKernelOpen.rb:18:5:18:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:28:5:28:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
+| NonConstantKernelOpen.rb:46:5:46: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

@@ -1,4 +1,7 @@
 class UsersController < ActionController::Base
+  CONSTANT = "constant"
+  CONSTANT_WITH_FREEZE = "constant-with-freeze".freeze
+
   def create
     file = params[:file]
     open(file) # BAD
@@ -30,6 +33,16 @@ def create
 
     IO.write(File.join("foo", "bar.txt"), "bar") # GOOD
 
+    IO.read(CONSTANT) # GOOD
+
+    IO.read(CONSTANT + file) # GOOD
+
+    IO.read(CONSTANT_WITH_FREEZE) # GOOD
+
+    IO.read(CONSTANT_WITH_FREEZE + file) # GOOD
+    
+    open.where(external: false) # GOOD - an open method is called withoout arguments
+    
     open(file) # BAD - sanity check to verify that file was not mistakenly marked as sanitized
   end
 end