Merge pull request github#11191 from erik-krogh/arrJoin

RB: add join(" ") calls as a sink for rb/shell-command-constructed-from-input

Author: erik-krogh

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

@@ -4,11 +4,11 @@
  * well as extension points for adding your own.
  */
 
+private import ruby
 private import codeql.ruby.DataFlow
 private import codeql.ruby.DataFlow2
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.frameworks.core.Gem::Gem as Gem
-private import codeql.ruby.AST as Ast
 private import codeql.ruby.Concepts as Concepts
 
 /**
@@ -81,6 +81,37 @@ module UnsafeShellCommandConstruction {
     override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit }
   }
 
+  /**
+   * A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
+   */
+  class ArrayJoin extends Sink {
+    Concepts::SystemCommandExecution s;
+    DataFlow::CallNode call;
+
+    ArrayJoin() {
+      call.getMethodName() = "join" and
+      call.getNumberOfArguments() = 1 and
+      call.getArgument(0).getConstantValue().getString() = " " and
+      isUsedAsShellCommand(call, s) and
+      (
+        this = call.getReceiver() and
+        not call.getReceiver().asExpr() instanceof Cfg::CfgNodes::ExprNodes::ArrayLiteralCfgNode
+        or
+        this.asExpr() =
+          call.getReceiver()
+              .asExpr()
+              .(Cfg::CfgNodes::ExprNodes::ArrayLiteralCfgNode)
+              .getAnArgument()
+      )
+    }
+
+    override string describe() { result = "array" }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override DataFlow::Node getStringConstruction() { result = call }
+  }
+
   import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
 
   /**

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

@@ -9,6 +9,8 @@ edges
 | impl/unsafeShell.rb:33:12:33:17 | target :  | impl/unsafeShell.rb:34:19:34:27 | #{...} |
 | impl/unsafeShell.rb:37:10:37:10 | x :  | impl/unsafeShell.rb:38:19:38:22 | #{...} |
 | impl/unsafeShell.rb:47:16:47:21 | target :  | impl/unsafeShell.rb:48:19:48:27 | #{...} |
+| impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:52:14:52:14 | x |
+| impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:54:29:54:29 | x |
 nodes
 | impl/sub/notImported.rb:2:12:2:17 | target :  | semmle.label | target :  |
 | impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
@@ -30,6 +32,9 @@ nodes
 | impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} |
 | impl/unsafeShell.rb:47:16:47:21 | target :  | semmle.label | target :  |
 | impl/unsafeShell.rb:48:19:48:27 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:51:17:51:17 | x :  | semmle.label | x :  |
+| impl/unsafeShell.rb:52:14:52:14 | x | semmle.label | x |
+| impl/unsafeShell.rb:54:29:54:29 | x | semmle.label | x |
 subpaths
 #select
 | impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target :  | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
@@ -42,3 +47,5 @@ subpaths
 | impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target :  | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command |
 | impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x :  | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command |
 | impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target :  | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command |
+| impl/unsafeShell.rb:52:14:52:24 | call to join | impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:52:14:52:14 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:52:5:52:30 | call to popen | shell command |
+| impl/unsafeShell.rb:54:14:54:40 | call to join | impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:54:29:54:29 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:54:5:54:46 | call to popen | shell command |

ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb

@@ -47,4 +47,10 @@ def thisIsSafe()
   def self.foo(target)
     IO.popen("cat #{target}", "w") # NOT OK
   end
+
+  def arrayJoin(x)
+    IO.popen(x.join(' '), "w") # NOT OK
+
+    IO.popen(["foo", "bar", x].join(' '), "w") # NOT OK
+  end
 end