Add suggestions to fix FileJoinSanitizer

Author: p-

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,23 +17,12 @@ class AmbiguousPathCall extends DataFlow::CallNode {
     this.(KernelMethodCall).getMethodName() = "open" and
     name = "Kernel.open"
     or
-    methodCallOnlyOnIO(this, "read") and
-    name = "IO.read"
-    or
-    methodCallOnlyOnIO(this, "write") and
-    name = "IO.write"
-    or
-    methodCallOnlyOnIO(this, "binread") and
-    name = "IO.binread"
-    or
-    methodCallOnlyOnIO(this, "binwrite") and
-    name = "IO.binwrite"
-    or
-    methodCallOnlyOnIO(this, "foreach") and
-    name = "IO.foreach"
-    or
-    methodCallOnlyOnIO(this, "readlines") and
-    name = "IO.readlines"
+    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"
@@ -75,11 +65,9 @@ private predicate methodCallOnlyOnIO(DataFlow::CallNode node, string methodName)
 abstract class Sanitizer extends DataFlow::Node { }
 
 /**
- * If a File.join is performed the resulting string will not start with a pipe |
+ * 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 FileJoinCall extends Sanitizer, DataFlow::ExprNode {
-  FileJoinCall() {
-    this = API::getTopLevelMember("File").getAMethodCall("join").getArgument(any(int i | i > 0))
-  }
+private class FileJoinSanitizer extends Sanitizer {
+  FileJoinSanitizer() { this = any(File::FileJoinSummary s).getParameter("1..") }
 }

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

@@ -9,6 +9,7 @@ edges
 | 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 :  |
@@ -23,6 +24,7 @@ nodes
 | 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 |
@@ -34,3 +36,4 @@ subpaths
 | 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

@@ -10,7 +10,7 @@ def create
     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, "")) # 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
@@ -22,5 +22,7 @@ def create
     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

@@ -8,3 +8,4 @@
 | 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

@@ -29,5 +29,7 @@ def create
     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