Merge pull request #11397 from erik-krogh/call-instanceof

Rb: use `instanceof` instead of `extends` on `DataFlow::CallNode` in some case

Author: erik-krogh

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

@@ -168,7 +168,7 @@ module ActiveStorage {
    * A call on an ActiveStorage object that results in an image transformation.
    * Arguments to these calls may be executed as system commands.
    */
-  private class ImageProcessingCall extends DataFlow::CallNode, SystemCommandExecution::Range {
+  private class ImageProcessingCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     ImageProcessingCall() {
       this.getReceiver() instanceof BlobInstance and
       this.getMethodName() = ["variant", "preview", "representation"]
@@ -209,7 +209,7 @@ module ActiveStorage {
       this = API::getTopLevelMember("ActiveStorage").getAMethodCall("video_preview_arguments=")
     }
 
-    override DataFlow::Node getAnArgument() { result = this.getArgument(0) }
+    override DataFlow::Node getAnArgument() { result = super.getArgument(0) }
   }
 
   /**

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

@@ -20,7 +20,7 @@ module PosixSpawn {
   /**
    * A call to `POSIX::Spawn::Child.new` or `POSIX::Spawn::Child.build`.
    */
-  class ChildCall extends SystemCommandExecution::Range, DataFlow::CallNode {
+  class ChildCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     ChildCall() {
       this =
         [
@@ -30,7 +30,7 @@ module PosixSpawn {
     }
 
     override DataFlow::Node getAnArgument() {
-      result = this.getArgument(_) and not result.asExpr() instanceof ExprNodes::PairCfgNode
+      result = super.getArgument(_) and not result.asExpr() instanceof ExprNodes::PairCfgNode
     }
 
     override predicate isShellInterpreted(DataFlow::Node arg) { none() }
@@ -39,7 +39,7 @@ module PosixSpawn {
   /**
    * A call to `POSIX::Spawn.spawn` or a related method.
    */
-  class SystemCall extends SystemCommandExecution::Range, DataFlow::CallNode {
+  class SystemCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     SystemCall() {
       this =
         posixSpawnModule()
@@ -71,7 +71,7 @@ module PosixSpawn {
     }
 
     private predicate argument(DataFlow::Node arg) {
-      arg = this.getArgument(_) and
+      arg = super.getArgument(_) and
       not arg.asExpr() instanceof ExprNodes::HashLiteralCfgNode and
       not arg.asExpr() instanceof ExprNodes::ArrayLiteralCfgNode and
       not arg.asExpr() instanceof ExprNodes::PairCfgNode

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

@@ -27,28 +27,28 @@ module Railties {
    * A call to `Rails::Generators::Actions#execute_command`.
    * This method concatenates its first and second arguments and executes the result as a shell command.
    */
-  private class ExecuteCommandCall extends SystemCommandExecution::Range, DataFlow::CallNode {
+  private class ExecuteCommandCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     ExecuteCommandCall() {
       this = generatorsActionsClass().getAnInstanceSelf().getAMethodCall("execute_command")
     }
 
-    override DataFlow::Node getAnArgument() { result = this.getArgument([0, 1]) }
+    override DataFlow::Node getAnArgument() { result = super.getArgument([0, 1]) }
 
     override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getAnArgument() }
   }
 
   /**
    * A call to a method in `Rails::Generators::Actions` which delegates to `execute_command`.
    */
-  private class ExecuteCommandWrapperCall extends SystemCommandExecution::Range, DataFlow::CallNode {
+  private class ExecuteCommandWrapperCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     ExecuteCommandWrapperCall() {
       this =
         generatorsActionsClass()
             .getAnInstanceSelf()
             .getAMethodCall(["rake", "rails_command", "git"])
     }
 
-    override DataFlow::Node getAnArgument() { result = this.getArgument(0) }
+    override DataFlow::Node getAnArgument() { result = super.getArgument(0) }
 
     override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getAnArgument() }
   }

ruby/ql/lib/codeql/ruby/frameworks/core/IO.qll

@@ -114,7 +114,7 @@ module IO {
    * ```
    * Ruby documentation: https://docs.ruby-lang.org/en/3.1/IO.html#method-c-popen
    */
-  class POpenCall extends SystemCommandExecution::Range, DataFlow::CallNode {
+  class POpenCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     POpenCall() { this = API::getTopLevelMember("IO").getAMethodCall("popen") }
 
     override DataFlow::Node getAnArgument() { this.argument(result, _) }
@@ -131,15 +131,15 @@ module IO {
         not n instanceof ExprNodes::ArrayLiteralCfgNode and
         (
           // IO.popen({var: "a"}, "cmd", {some: :opt})
-          arg = this.getArgument([0, 1]) and
+          arg = super.getArgument([0, 1]) and
           // We over-approximate by assuming a subshell if the argument isn't an array or "-".
           // This increases the sensitivity of the CommandInjection query at the risk of some FPs.
           if n.getConstantValue().getString() = "-" then shell = false else shell = true
           or
           // IO.popen([{var: "b"}, "cmd", "arg1", "arg2", {some: :opt}])
           // IO.popen({var: "a"}, ["cmd", "arg1", "arg2", {some: :opt}])
           shell = false and
-          exists(ExprNodes::ArrayLiteralCfgNode arr | this.getArgument([0, 1]).asExpr() = arr |
+          exists(ExprNodes::ArrayLiteralCfgNode arr | super.getArgument([0, 1]).asExpr() = arr |
             n = arr.getAnArgument()
             or
             // IO.popen([{var: "b"}, ["cmd", "argv0"], "arg1", "arg2", {some: :opt}])

ruby/ql/lib/codeql/ruby/frameworks/stdlib/Open3.qll

@@ -2,10 +2,8 @@
  * Provides modeling for the `Open3` library.
  */
 
-private import codeql.ruby.AST
-private import codeql.ruby.DataFlow
+private import ruby
 private import codeql.ruby.ApiGraphs
-private import codeql.ruby.frameworks.Stdlib
 private import codeql.ruby.Concepts
 
 /**
@@ -17,23 +15,19 @@ module Open3 {
    * These methods take the same argument forms as `Kernel.system`.
    * See `KernelSystemCall` for details.
    */
-  class Open3Call extends SystemCommandExecution::Range {
-    MethodCall methodCall;
-
+  class Open3Call extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     Open3Call() {
-      this.asExpr().getExpr() = methodCall and
       this =
         API::getTopLevelMember("Open3")
             .getAMethodCall(["popen3", "popen2", "popen2e", "capture3", "capture2", "capture2e"])
     }
 
-    override DataFlow::Node getAnArgument() {
-      result.asExpr().getExpr() = methodCall.getAnArgument()
-    }
+    override DataFlow::Node getAnArgument() { result = super.getArgument(_) }
 
     override predicate isShellInterpreted(DataFlow::Node arg) {
       // These Open3 methods invoke a subshell if you provide a single string as argument
-      methodCall.getNumberOfArguments() = 1 and arg.asExpr().getExpr() = methodCall.getAnArgument()
+      super.getNumberOfArguments() = 1 and
+      arg = this.getAnArgument()
     }
   }
 
@@ -47,26 +41,21 @@ module Open3 {
    * Open3.pipeline([{}, "cat", "foo.txt"], "tail")
    * Open3.pipeline([["cat", "cat"], "foo.txt"], "tail")
    */
-  class Open3PipelineCall extends SystemCommandExecution::Range {
-    MethodCall methodCall;
-
+  class Open3PipelineCall extends SystemCommandExecution::Range instanceof DataFlow::CallNode {
     Open3PipelineCall() {
-      this.asExpr().getExpr() = methodCall and
       this =
         API::getTopLevelMember("Open3")
             .getAMethodCall([
                 "pipeline_rw", "pipeline_r", "pipeline_w", "pipeline_start", "pipeline"
               ])
     }
 
-    override DataFlow::Node getAnArgument() {
-      result.asExpr().getExpr() = methodCall.getAnArgument()
-    }
+    override DataFlow::Node getAnArgument() { result = super.getArgument(_) }
 
     override predicate isShellInterpreted(DataFlow::Node arg) {
       // A command in the pipeline is executed in a subshell if it is given as a single string argument.
-      arg.asExpr().getExpr() instanceof StringlikeLiteral and
-      arg.asExpr().getExpr() = methodCall.getAnArgument()
+      arg.asExpr().getExpr() instanceof Ast::StringlikeLiteral and
+      arg = this.getAnArgument()
     }
   }
 }