Ruby: improve non-constant-kernel-open, freeze called on constant

Author: p-

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

@@ -18,24 +18,33 @@
 import codeql.ruby.security.KernelOpenQuery
 import codeql.ruby.AST
 import codeql.ruby.ApiGraphs
+import codeql.ruby.DataFlow
 
 from AmbiguousPathCall call
 where
   call.getNumberOfArguments() > 0 and
-  not hasConstantPrefix(call.getPathArgument().getALocalSource().asExpr().getExpr()) 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.asExpr().getExpr().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
+  node.asExpr().getExpr().(ConstantReadAccess).getValue().(MethodCall).getMethodName() = "freeze"
 }

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

@@ -1,11 +1,11 @@
-| NonConstantKernelOpen.rb:6:5:6:14 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
-| NonConstantKernelOpen.rb:7:5:7:17 | call to read | Call to IO.read with a non-constant value. Consider replacing it with File.read. |
-| NonConstantKernelOpen.rb:8:5:8:18 | call to write | Call to IO.write with a non-constant value. Consider replacing it with File.write. |
-| NonConstantKernelOpen.rb:9:5:9:20 | call to binread | Call to IO.binread with a non-constant value. Consider replacing it with File.binread. |
-| NonConstantKernelOpen.rb:10:5:10:21 | call to binwrite | Call to IO.binwrite with a non-constant value. Consider replacing it with File.binwrite. |
-| NonConstantKernelOpen.rb:11:5:11:20 | call to foreach | Call to IO.foreach with a non-constant value. Consider replacing it with File.foreach. |
-| NonConstantKernelOpen.rb:12:5:12:22 | call to readlines | Call to IO.readlines with a non-constant value. Consider replacing it with File.readlines. |
-| NonConstantKernelOpen.rb:13:5:13:18 | call to open | Call to URI.open with a non-constant value. Consider replacing it with URI(<uri>).open. |
-| NonConstantKernelOpen.rb:17:5:17:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
-| NonConstantKernelOpen.rb:27:5:27:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. |
-| NonConstantKernelOpen.rb:41:5:41: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,5 +1,6 @@
 class UsersController < ActionController::Base
   CONSTANT = "constant"
+  CONSTANT_WITH_FREEZE = "constant-with-freeze".freeze
 
   def create
     file = params[:file]
@@ -36,6 +37,10 @@ def create
 
     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