Merge pull request github#9797 from github/nickrolfe/railties_fix

Ruby: fix defining every dataflow node as a command execution sink

Author: nickrolfe

ruby/ql/lib/change-notes/2022-07-11-command-execution.md

@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* Fixed a bug causing every expression in the database to be considered a system-command execution sink when calls to any of the following methods exist:
+  * The `spawn`, `fspawn`, `popen4`, `pspawn`, `system`, `_pspawn` methods and the backtick operator from the `POSIX::spawn` gem.
+  * The `execute_command`, `rake`, `rails_command`, and `git` methods in `Rails::Generation::Actions`.

ruby/ql/lib/codeql/ruby/frameworks/PosixSpawn.qll

@@ -62,9 +62,9 @@ module PosixSpawn {
     // is shell interpreted unless there is another argument with a string
     // constant value.
     override predicate isShellInterpreted(DataFlow::Node arg) {
+      this.argument(arg) and
       not exists(DataFlow::Node otherArg |
         otherArg != arg and
-        this.argument(arg) and
         this.argument(otherArg) and
         otherArg.asExpr().getConstantValue().isString(_)
       )

ruby/ql/lib/codeql/ruby/frameworks/Railties.qll

@@ -43,7 +43,7 @@ module Railties {
 
     override DataFlow::Node getAnArgument() { result = this.getArgument([0, 1]) }
 
-    override predicate isShellInterpreted(DataFlow::Node arg) { any() }
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getAnArgument() }
   }
 
   /**
@@ -57,6 +57,6 @@ module Railties {
 
     override DataFlow::Node getAnArgument() { result = this.getArgument(0) }
 
-    override predicate isShellInterpreted(DataFlow::Node arg) { any() }
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getAnArgument() }
   }
 }

ruby/ql/test/library-tests/frameworks/PosixSpawn.ql

@@ -5,11 +5,13 @@ import codeql.ruby.DataFlow
 query predicate systemCalls(
   PosixSpawn::SystemCall call, DataFlow::Node arg, boolean shellInterpreted
 ) {
-  arg = call.getAnArgument() and
-  if call.isShellInterpreted(arg) then shellInterpreted = true else shellInterpreted = false
+  call.isShellInterpreted(arg) and shellInterpreted = true
+  or
+  not call.isShellInterpreted(arg) and arg = call.getAnArgument() and shellInterpreted = false
 }
 
 query predicate childCalls(PosixSpawn::ChildCall call, DataFlow::Node arg, boolean shellInterpreted) {
-  arg = call.getAnArgument() and
-  if call.isShellInterpreted(arg) then shellInterpreted = true else shellInterpreted = false
+  call.isShellInterpreted(arg) and shellInterpreted = true
+  or
+  not call.isShellInterpreted(arg) and arg = call.getAnArgument() and shellInterpreted = false
 }

ruby/ql/test/library-tests/frameworks/railties/Railties.expected

@@ -1,5 +1,14 @@
+systemCommandExecutions
 | Railties.rb:5:5:5:34 | call to execute_command |
 | Railties.rb:6:5:6:37 | call to execute_command |
 | Railties.rb:8:5:8:16 | call to rake |
 | Railties.rb:10:5:10:27 | call to rails_command |
 | Railties.rb:12:5:12:17 | call to git |
+shellInterpretedArguments
+| Railties.rb:5:5:5:34 | call to execute_command | Railties.rb:5:21:5:25 | :rake |
+| Railties.rb:5:5:5:34 | call to execute_command | Railties.rb:5:28:5:33 | "test" |
+| Railties.rb:6:5:6:37 | call to execute_command | Railties.rb:6:21:6:26 | :rails |
+| Railties.rb:6:5:6:37 | call to execute_command | Railties.rb:6:29:6:36 | "server" |
+| Railties.rb:8:5:8:16 | call to rake | Railties.rb:8:10:8:15 | "test" |
+| Railties.rb:10:5:10:27 | call to rails_command | Railties.rb:10:19:10:26 | "server" |
+| Railties.rb:12:5:12:17 | call to git | Railties.rb:12:9:12:16 | "status" |

ruby/ql/test/library-tests/frameworks/railties/Railties.ql

@@ -1,5 +1,10 @@
 private import ruby
 private import codeql.ruby.Concepts
 private import codeql.ruby.frameworks.Railties
+private import codeql.ruby.DataFlow
 
 query predicate systemCommandExecutions(SystemCommandExecution e) { any() }
+
+query predicate shellInterpretedArguments(SystemCommandExecution e, DataFlow::Node arg) {
+  e.isShellInterpreted(arg)
+}