Merge pull request #15524 from hmac/hmac-process-spawn

Ruby: Add some more command injection sinks
2026-04-22 23:35:14 +02:00 · 2024-03-13 09:53:10 +00:00
parent 2fd2b4c874 148241183a
commit dd5eb982ec
13 changed files with 214 additions and 3 deletions
--- a/ruby/ql/lib/change-notes/2024-02-27-process-spawn.md
+++ b/ruby/ql/lib/change-notes/2024-02-27-process-spawn.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* New command injection sinks have been added, including `Process.spawn`, `Process.exec`, `Terrapin::CommandLine` and the `open4` gem.
--- a/ruby/ql/lib/codeql/ruby/frameworks/Stdlib.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/Stdlib.qll
@@ -5,3 +5,4 @@
 import stdlib.Open3
 import stdlib.Logger
 import stdlib.Pathname
+import stdlib.Process
--- a/ruby/ql/lib/codeql/ruby/frameworks/stdlib/Open3.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/stdlib/Open3.qll
@@ -7,7 +7,7 @@ private import codeql.ruby.ApiGraphs
 private import codeql.ruby.Concepts

 /**
- * Provides modeling for the `Open3` library.
+ * Provides modeling for the `Open3` and `Open4` libraries.
 */
 module Open3 {
  /**
@@ -31,6 +31,36 @@ module Open3 {
    }
  }

+  /**
+   * A system command executed via one of the `Open4` methods.
+   * These methods take the same argument forms as `Kernel.system`.
+   * See `KernelSystemCall` for details.
+   */
+  class Open4Call extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
+    Open4Call() {
+      this =
+        API::getTopLevelMember("Open4").getAMethodCall(["open4", "popen4", "spawn", "popen4ext"])
+    }
+
+    override DataFlow::Node getAnArgument() {
+      // `popen4ext` takes an optional boolean as its first argument, but it is unlikely that we will be
+      // tracking flow into a boolean value so it doesn't seem worth modeling that special case here.
+      result = super.getArgument(_)
+    }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      super.getNumberOfArguments() = 1 and
+      arg = this.getAnArgument()
+      or
+      // ```rb
+      // Open4.popen4ext(true, "some cmd")
+      // ```
+      super.getNumberOfArguments() = 2 and
+      super.getArgument(0).getConstantValue().isBoolean(_) and
+      arg = super.getArgument(1)
+    }
+  }
+
  /**
   * A pipeline of system commands constructed via one of the `Open3` methods.
   * These methods accept a variable argument list of commands.
--- a/ruby/ql/lib/codeql/ruby/frameworks/stdlib/Process.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/stdlib/Process.qll
@@ -0,0 +1,49 @@
+/**
+ * Provides modeling for the `Process` library.
+ */
+
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.controlflow.CfgNodes
+private import codeql.ruby.frameworks.core.Kernel
+
+/**
+ * Provides modeling for the `Process` library.
+ */
+module Process {
+  /**
+   * A call to `Process.spawn`.
+   * ```rb
+   * Process.spawn("tar xf ruby-2.0.0-p195.tar.bz2")
+   * Process.spawn({"ENV" => "VAR"}, "echo",  "hi")
+   * ```
+   */
+  class SpawnCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
+    SpawnCall() { this = DataFlow::getConstant(["Process", "PTY"]).getAMethodCall("spawn") }
+
+    // The command can be argument 0 or 1
+    // Options can be specified after the command, and we want to exclude those.
+    override DataFlow::Node getAnArgument() {
+      result = super.getArgument([0, 1]) and not result.asExpr() instanceof ExprNodes::PairCfgNode
+    }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      // Process.spawn invokes a subshell if you provide a single string as argument
+      super.getNumberOfArguments() = 1 and arg = this.getAnArgument()
+    }
+  }
+
+  /**
+   * A system command executed via the `Process.exec` method.
+   */
+  class ExecCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
+    ExecCall() { this = DataFlow::getConstant("Process").getAMethodCall("exec") }
+
+    override DataFlow::Node getAnArgument() { result = super.getArgument(_) }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      // Process.exec invokes a subshell if you provide a single string as argument
+      super.getNumberOfArguments() = 1 and arg = this.getAnArgument()
+    }
+  }
+}
--- a/ruby/ql/lib/codeql/ruby/frameworks/terrapin/model.yml
+++ b/ruby/ql/lib/codeql/ruby/frameworks/terrapin/model.yml
@@ -0,0 +1,41 @@
+extensions:
+  - addsTo:
+      pack: codeql/ruby-all
+      extensible: sourceModel
+    data: []
+
+  - addsTo:
+      pack: codeql/ruby-all
+      extensible: sinkModel
+    data:
+      - ["Terrapin::CommandLine!","Method[new].Argument[0]","command-injection"]
+      - ["Terrapin::CommandLine!","Method[new].Argument[1]","command-injection"]
+
+  - addsTo:
+      pack: codeql/ruby-all
+      extensible: summaryModel
+    data:
+      - ["Terrapin::CommandLine::Output!","Method[new]","Argument[1]","ReturnValue","value"]
+      - ["Terrapin::CommandLine!","Method[path=]","Argument[0]","ReturnValue","taint"]
+      - ["Terrapin::CommandLine!","Method[new]","Argument[2]","ReturnValue","taint"]
+
+  - addsTo:
+      pack: codeql/ruby-all
+      extensible: neutralModel
+    data: []
+
+  - addsTo:
+      pack: codeql/ruby-all
+      extensible: typeModel
+    data:
+      - ["Terrapin::CommandLine::Output","Terrapin::CommandLine::MultiPipe","Method[output].ReturnValue"]
+      - ["Terrapin::CommandLine::Output","Terrapin::CommandLine::FakeRunner","Method[call].ReturnValue"]
+      - ["Terrapin::CommandLine::Output","Terrapin::CommandLine::ProcessRunner","Method[call].ReturnValue"]
+      - ["Terrapin::CommandLine::Output","Terrapin::CommandLine!","Method[runner].ReturnValue.ReturnValue"]
+      - ["Terrapin::CommandLine::FakeRunner","Terrapin::CommandLine!","Method[runner].ReturnValue"]
+      - ["Terrapin::CommandLine::Output","Terrapin::CommandLine!","Method[fake!].ReturnValue.ReturnValue"]
+      - ["Terrapin::CommandLine::FakeRunner","Terrapin::CommandLine!","Method[fake!].ReturnValue"]
+      - ["Terrapin::CommandLine::Output","Terrapin::CommandLine","Method[output].ReturnValue"]
+      - ["Terrapin::CommandLineError","Terrapin::CommandNotFoundError",""]
+      - ["Terrapin::CommandLineError","Terrapin::ExitStatusError",""]
+      - ["Terrapin::CommandLineError","Terrapin::InterpolationError",""]
--- a/ruby/ql/test/library-tests/frameworks/stdlib/CommandExecution.expected
+++ b/ruby/ql/test/library-tests/frameworks/stdlib/CommandExecution.expected
@@ -0,0 +1,32 @@
+| Open3.rb:1:1:1:24 | call to popen3 | Open3.rb:1:14:1:23 | "echo foo" | true |
+| Open3.rb:2:1:2:24 | call to popen2 | Open3.rb:2:14:2:23 | "echo foo" | true |
+| Open3.rb:3:1:3:25 | call to popen2e | Open3.rb:3:15:3:24 | "echo foo" | true |
+| Open3.rb:4:1:4:26 | call to capture3 | Open3.rb:4:16:4:25 | "echo foo" | true |
+| Open3.rb:5:1:5:26 | call to capture2 | Open3.rb:5:16:5:25 | "echo foo" | true |
+| Open3.rb:6:1:6:27 | call to capture2e | Open3.rb:6:17:6:26 | "echo foo" | true |
+| Open3.rb:7:1:7:41 | call to pipeline_rw | Open3.rb:7:19:7:28 | "echo foo" | true |
+| Open3.rb:7:1:7:41 | call to pipeline_rw | Open3.rb:7:31:7:40 | "grep bar" | true |
+| Open3.rb:8:1:8:40 | call to pipeline_r | Open3.rb:8:18:8:27 | "echo foo" | true |
+| Open3.rb:8:1:8:40 | call to pipeline_r | Open3.rb:8:30:8:39 | "grep bar" | true |
+| Open3.rb:9:1:9:40 | call to pipeline_w | Open3.rb:9:18:9:27 | "echo foo" | true |
+| Open3.rb:9:1:9:40 | call to pipeline_w | Open3.rb:9:30:9:39 | "grep bar" | true |
+| Open3.rb:10:1:10:44 | call to pipeline_start | Open3.rb:10:22:10:31 | "echo foo" | true |
+| Open3.rb:10:1:10:44 | call to pipeline_start | Open3.rb:10:34:10:43 | "grep bar" | true |
+| Open3.rb:11:1:11:38 | call to pipeline | Open3.rb:11:16:11:25 | "echo foo" | true |
+| Open3.rb:11:1:11:38 | call to pipeline | Open3.rb:11:28:11:37 | "grep bar" | true |
+| Open3.rb:13:1:13:24 | call to open4 | Open3.rb:13:14:13:23 | "echo foo" | true |
+| Open3.rb:14:1:14:25 | call to popen4 | Open3.rb:14:15:14:24 | "echo foo" | true |
+| Open3.rb:15:1:15:23 | call to spawn | Open3.rb:15:13:15:22 | "echo bar" | true |
+| Open3.rb:16:1:16:27 | call to popen4ext | Open3.rb:16:17:16:26 | "echo foo" | true |
+| Open3.rb:17:1:17:30 | call to popen4ext | Open3.rb:17:17:17:22 | "echo" | false |
+| Open3.rb:17:1:17:30 | call to popen4ext | Open3.rb:17:25:17:29 | "foo" | false |
+| Open3.rb:18:1:18:33 | call to popen4ext | Open3.rb:18:17:18:20 | true | false |
+| Open3.rb:18:1:18:33 | call to popen4ext | Open3.rb:18:23:18:32 | "echo foo" | true |
+| Open3.rb:19:1:19:36 | call to popen4ext | Open3.rb:19:17:19:20 | true | false |
+| Open3.rb:19:1:19:36 | call to popen4ext | Open3.rb:19:23:19:28 | "echo" | false |
+| Open3.rb:19:1:19:36 | call to popen4ext | Open3.rb:19:31:19:35 | "foo" | false |
+| process.rb:1:1:1:25 | call to spawn | process.rb:1:15:1:24 | "echo foo" | true |
+| process.rb:2:1:2:30 | call to spawn | process.rb:2:15:2:29 | call to [] | true |
+| process.rb:3:1:3:24 | call to exec | process.rb:3:14:3:23 | "echo foo" | true |
+| process.rb:4:1:4:29 | call to exec | process.rb:4:14:4:28 | call to [] | true |
+| process.rb:5:1:5:21 | call to spawn | process.rb:5:11:5:20 | "echo foo" | true |
--- a/ruby/ql/test/library-tests/frameworks/stdlib/CommandExecution.ql
+++ b/ruby/ql/test/library-tests/frameworks/stdlib/CommandExecution.ql
@@ -0,0 +1,12 @@
+import codeql.ruby.Frameworks
+import codeql.ruby.Concepts
+import codeql.ruby.DataFlow
+
+query predicate commandExecutions(
+  SystemCommandExecution execution, DataFlow::Node arg, boolean isShellInterpreted
+) {
+  arg = execution.getAnArgument() and
+  if execution.isShellInterpreted(arg)
+  then isShellInterpreted = true
+  else isShellInterpreted = false
+}
--- a/ruby/ql/test/library-tests/frameworks/stdlib/Open3.expected
+++ b/ruby/ql/test/library-tests/frameworks/stdlib/Open3.expected
@@ -11,3 +11,11 @@ open3PipelineCallExecutions
 | Open3.rb:9:1:9:40 | call to pipeline_w |
 | Open3.rb:10:1:10:44 | call to pipeline_start |
 | Open3.rb:11:1:11:38 | call to pipeline |
+open4CallExecutions
+| Open3.rb:13:1:13:24 | call to open4 |
+| Open3.rb:14:1:14:25 | call to popen4 |
+| Open3.rb:15:1:15:23 | call to spawn |
+| Open3.rb:16:1:16:27 | call to popen4ext |
+| Open3.rb:17:1:17:30 | call to popen4ext |
+| Open3.rb:18:1:18:33 | call to popen4ext |
+| Open3.rb:19:1:19:36 | call to popen4ext |
--- a/ruby/ql/test/library-tests/frameworks/stdlib/Open3.ql
+++ b/ruby/ql/test/library-tests/frameworks/stdlib/Open3.ql
@@ -4,3 +4,5 @@ import codeql.ruby.DataFlow
 query predicate open3CallExecutions(Open3Call c) { any() }

 query predicate open3PipelineCallExecutions(Open3PipelineCall c) { any() }
+
+query predicate open4CallExecutions(Open4Call c) { any() }
--- a/ruby/ql/test/library-tests/frameworks/stdlib/Open3.rb
+++ b/ruby/ql/test/library-tests/frameworks/stdlib/Open3.rb
@@ -8,4 +8,12 @@ Open3.pipeline_rw("echo foo", "grep bar")
 Open3.pipeline_r("echo foo", "grep bar")
 Open3.pipeline_w("echo foo", "grep bar")
 Open3.pipeline_start("echo foo", "grep bar")
-Open3.pipeline("echo foo", "grep bar")
+Open3.pipeline("echo foo", "grep bar")
+
+Open4::open4("echo foo")
+Open4::popen4("echo foo")
+Open4.spawn("echo bar")
+Open4.popen4ext("echo foo")
+Open4.popen4ext("echo", "foo")
+Open4.popen4ext(true, "echo foo")
+Open4.popen4ext(true, "echo", "foo")
--- a/ruby/ql/test/library-tests/frameworks/stdlib/process.rb
+++ b/ruby/ql/test/library-tests/frameworks/stdlib/process.rb
@@ -0,0 +1,5 @@
+Process.spawn("echo foo")
+Process.spawn(["echo", "foo"])
+Process.exec("echo foo")
+Process.exec(["echo", "foo"])
+PTY.spawn("echo foo")
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
@@ -21,6 +21,9 @@ edges
 | CommandInjection.rb:103:9:103:12 | file | CommandInjection.rb:104:16:104:28 | "cat #{...}" | provenance |  |
 | CommandInjection.rb:103:16:103:21 | call to params | CommandInjection.rb:103:16:103:28 | ...[...] | provenance |  |
 | CommandInjection.rb:103:16:103:28 | ...[...] | CommandInjection.rb:103:9:103:12 | file | provenance |  |
+| CommandInjection.rb:111:33:111:38 | call to params | CommandInjection.rb:111:33:111:44 | ...[...] | provenance |  |
+| CommandInjection.rb:113:44:113:49 | call to params | CommandInjection.rb:113:44:113:54 | ...[...] | provenance |  |
+| CommandInjection.rb:113:44:113:54 | ...[...] | CommandInjection.rb:113:41:113:56 | "#{...}" | provenance |  |
 nodes
 | CommandInjection.rb:6:9:6:11 | cmd | semmle.label | cmd |
 | CommandInjection.rb:6:15:6:20 | call to params | semmle.label | call to params |
@@ -51,6 +54,11 @@ nodes
 | CommandInjection.rb:103:16:103:21 | call to params | semmle.label | call to params |
 | CommandInjection.rb:103:16:103:28 | ...[...] | semmle.label | ...[...] |
 | CommandInjection.rb:104:16:104:28 | "cat #{...}" | semmle.label | "cat #{...}" |
+| CommandInjection.rb:111:33:111:38 | call to params | semmle.label | call to params |
+| CommandInjection.rb:111:33:111:44 | ...[...] | semmle.label | ...[...] |
+| CommandInjection.rb:113:41:113:56 | "#{...}" | semmle.label | "#{...}" |
+| CommandInjection.rb:113:44:113:49 | call to params | semmle.label | call to params |
+| CommandInjection.rb:113:44:113:54 | ...[...] | semmle.label | ...[...] |
 subpaths
 #select
 | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
@@ -67,3 +75,5 @@ subpaths
 | CommandInjection.rb:82:14:82:34 | "echo #{...}" | CommandInjection.rb:81:23:81:33 | blah_number | CommandInjection.rb:82:14:82:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:23:81:33 | blah_number | user-provided value |
 | CommandInjection.rb:91:14:91:39 | "echo #{...}" | CommandInjection.rb:91:22:91:37 | ...[...] | CommandInjection.rb:91:14:91:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:91:22:91:37 | ...[...] | user-provided value |
 | CommandInjection.rb:104:16:104:28 | "cat #{...}" | CommandInjection.rb:103:16:103:21 | call to params | CommandInjection.rb:104:16:104:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:103:16:103:21 | call to params | user-provided value |
+| CommandInjection.rb:111:33:111:44 | ...[...] | CommandInjection.rb:111:33:111:38 | call to params | CommandInjection.rb:111:33:111:44 | ...[...] | This command depends on a $@. | CommandInjection.rb:111:33:111:38 | call to params | user-provided value |
+| CommandInjection.rb:113:41:113:56 | "#{...}" | CommandInjection.rb:113:44:113:49 | call to params | CommandInjection.rb:113:41:113:56 | "#{...}" | This command depends on a $@. | CommandInjection.rb:113:44:113:49 | call to params | user-provided value |
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
@@ -106,4 +106,13 @@ class Foo < ActionController::Base
        system("cat #{file.shellescape}") # OK, because file is shell escaped
        
    end
-end
+
+    def index
+      Terrapin::CommandLine.new(params[:foo], "bar") # BAD
+
+      Terrapin::CommandLine.new("echo", "#{params[foo]}") # BAD
+
+      cmd = Terrapin::CommandLine.new("echo", ":msg")
+      cmd.run(msg: params[:foo]) # GOOD
+    end
+end