update Fabric models, add new sink to Fabric, add proper test cases

python/ql/lib/semmle/python/frameworks/Fabric.qll

@@ -71,7 +71,7 @@ private module FabricV1 {
  * Provides classes modeling security-relevant aspects of the `fabric` PyPI package, for
  * version 2.x.
  *
- * See http://docs.fabfile.org/en/2.5/getting-st  arted.html.
+ * See http://docs.fabfile.org/en/2.5/getting-started.html.
  */
 module FabricV2 {
   /** Gets a reference to the `fabric` module. */
@@ -93,56 +93,26 @@ module FabricV2 {
        * See https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.
        */
       module ConnectionClass {
-        /** Gets a reference to the `fabric.connection.Connection` class. */
-        API::Node classRef() {
-          result = fabric().getMember("Connection")
-          or
-          result = connection().getMember("Connection")
-          or
-          result =
-            ModelOutput::getATypeNode("fabric.connection.Connection~Subclass").getASubclass*()
-        }
-
         /**
          * A source of instances of `fabric.connection.Connection`, extend this class to model new instances.
          *
          * This can include instantiations of the class, return values from function
          * calls, or a special parameter that will be set when functions are called by an external
          * library.
-         *
-         * Use the predicate `Connection::instance()` to get references to instances of `fabric.connection.Connection`.
          */
-        abstract class InstanceSource extends DataFlow::LocalSourceNode { }
-
-        private class ClassInstantiation extends InstanceSource, DataFlow::CallCfgNode {
-          ClassInstantiation() { this = classRef().getACall() }
-        }
-
-        /** Gets a reference to an instance of `fabric.connection.Connection`. */
-        private DataFlow::TypeTrackingNode instance(DataFlow::TypeTracker t) {
-          t.start() and
-          result instanceof InstanceSource
-          or
-          exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
-        }
-
-        /** Gets a reference to an instance of `fabric.connection.Connection`. */
-        DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }
+        abstract class Instance extends DataFlow::LocalSourceNode { }
 
         /**
-         * Gets a reference to either `run`, `sudo`, or `local` method on a
-         * `fabric.connection.Connection` instance.
-         *
-         * See
-         * - https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.run
-         * - https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.sudo
-         * - https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.local
+         *  A reference to the `fabric.connection.Connection` class.
          */
-        private DataFlow::TypeTrackingNode instanceRunMethods(DataFlow::TypeTracker t) {
-          t.startInAttr(["run", "sudo", "local"]) and
-          result = instance()
-          or
-          exists(DataFlow::TypeTracker t2 | result = instanceRunMethods(t2).track(t2, t))
+        class ClassInstantiation extends Instance {
+          ClassInstantiation() {
+            this =
+              [
+                fabric().getMember("Connection"), connection().getMember("Connection"),
+                ModelOutput::getATypeNode("fabric.connection.Connection~Subclass").getASubclass*()
+              ].getACall()
+          }
         }
 
         /**
@@ -154,8 +124,8 @@ module FabricV2 {
          * - https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.sudo
          * - https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.local
          */
-        DataFlow::Node instanceRunMethods() {
-          instanceRunMethods(DataFlow::TypeTracker::end()).flowsTo(result)
+        API::CallNode instanceRunMethods() {
+          result = any(Instance is).getAMethodCall(["run", "sudo", "local"])
         }
       }
     }
@@ -168,19 +138,38 @@ module FabricV2 {
      * - https://docs.fabfile.org/en/2.5/api/connection.html#fabric.connection.Connection.local
      */
     private class FabricConnectionRunSudoLocalCall extends SystemCommandExecution::Range,
-      DataFlow::CallCfgNode
+      API::CallNode
     {
       FabricConnectionRunSudoLocalCall() {
-        this.getFunction() = Fabric::Connection::ConnectionClass::instanceRunMethods()
+        this = Fabric::Connection::ConnectionClass::instanceRunMethods()
       }
 
-      override DataFlow::Node getCommand() {
-        result = [this.getArg(0), this.getArgByName("command")]
-      }
+      override DataFlow::Node getCommand() { result = this.getParameter(0, "command").asSink() }
 
       override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
     }
 
+    /**
+     * A `gateway` parameter of `fabric.connection.Connection` instance is considered as ssh proxy_command option and can execute command.
+     * See https://docs.fabfile.org/en/latest/api/connection.html#fabric.connection.Connection
+     */
+    private class FabricConnectionProxyCommand extends SystemCommandExecution::Range, API::CallNode {
+      FabricConnectionProxyCommand() {
+        this instanceof Fabric::Connection::ConnectionClass::Instance and
+        // we want to make sure that the connection is established otherwise the command of proxy_command won't run.
+        exists(
+          this.getAMethodCall([
+              "run", "get", "sudo", "open_gateway", "open", "create_session", "forward_local",
+              "forward_remote", "put", "shell", "sftp"
+            ])
+        )
+      }
+
+      override DataFlow::Node getCommand() { result = this.getParameter(4, "gateway").asSink() }
+
+      override predicate isShellInterpreted(DataFlow::Node arg) { none() }
+    }
+
     // -------------------------------------------------------------------------
     // fabric.tasks
     // -------------------------------------------------------------------------
@@ -191,9 +180,12 @@ module FabricV2 {
     module Tasks {
       /** Gets a reference to the `fabric.tasks.task` decorator. */
       API::Node task() { result in [tasks().getMember("task"), fabric().getMember("task")] }
+
+      /** Gets a reference to the `fabric.tasks.task` decorator. */
+      API::Node test() { result in [tasks().getMember("task"), fabric().getMember("task")] }
     }
 
-    class FabricTaskFirstParamConnectionInstance extends Fabric::Connection::ConnectionClass::InstanceSource,
+    class FabricTaskFirstParamConnectionInstance extends Fabric::Connection::ConnectionClass::Instance,
       DataFlow::ParameterNode
     {
       FabricTaskFirstParamConnectionInstance() {
@@ -242,11 +234,14 @@ module FabricV2 {
         API::Node subclassInstance() { result = any(ModeledSubclass m).getASubclass*().getReturn() }
 
         /**
-         * Gets a reference to the `run` method on an instance of a subclass of `fabric.group.Group`.
+         * Gets a reference to the `run` and `sudo` methods on an instance of a subclass of `fabric.group.Group`.
          *
          * See https://docs.fabfile.org/en/2.5/api/group.html#fabric.group.Group.run
+         * See https://docs.fabfile.org/en/latest/api/group.html#fabric.group.Group.sudo
          */
-        API::Node subclassInstanceRunMethod() { result = subclassInstance().getMember("run") }
+        API::Node subclassInstanceRunMethod() {
+          result = subclassInstance().getMember(["run", "sudo"])
+        }
       }
 
       /**
@@ -254,14 +249,12 @@ module FabricV2 {
        *
        * See https://docs.fabfile.org/en/2.5/api/group.html#fabric.group.Group.run
        */
-      private class FabricGroupRunCall extends SystemCommandExecution::Range, DataFlow::CallCfgNode {
+      private class FabricGroupRunCall extends SystemCommandExecution::Range, API::CallNode {
         FabricGroupRunCall() {
           this = Fabric::Group::GroupClass::subclassInstanceRunMethod().getACall()
         }
 
-        override DataFlow::Node getCommand() {
-          result = [this.getArg(0), this.getArgByName("command")]
-        }
+        override DataFlow::Node getCommand() { result = this.getParameter(0, "command").asSink() }
 
         override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
       }

python/ql/test/library-tests/frameworks/fabric/fabric_v2_test.py

@@ -5,7 +5,6 @@ Loosely inspired by http://docs.fabfile.org/en/2.5/getting-started.html
 
 from fabric import connection, Connection, group, SerialGroup, ThreadingGroup, tasks, task
 
-
 ################################################################################
 # Connection
 ################################################################################
@@ -22,6 +21,32 @@ c.sudo(command="cmd1; cmd2")  # $getCommand="cmd1; cmd2"
 c2 = connection.Connection("web2")
 c2.run("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
 
+# ssh proxy_command command injection with gateway parameter,
+# we need to call some of the following functions to run the proxy command
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.run(command="cmd1; cmd2")  # $getCommand="cmd1; cmd2"
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.get("afs")
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.sudo(command="cmd1; cmd2")  # $getCommand="cmd1; cmd2"
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.open_gateway()
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.open()
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.create_session()
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.forward_local("80")
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.forward_remote("80")
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.put(local="local")
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.shell()
+c = Connection("web1", gateway="cmd")  # $getCommand="cmd"
+c.sftp()
+# no call to desired methods so it is safe
+c = Connection("web1", gateway="cmd")
 
 ################################################################################
 # SerialGroup
@@ -30,11 +55,11 @@ results = SerialGroup("web1", "web2", "mac1").run("cmd1; cmd2")  # $getCommand="
 
 pool = SerialGroup("web1", "web2", "web3")
 pool.run("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
+pool.sudo("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
 
 # fully qualified usage
 group.SerialGroup("web1", "web2", "mac1").run("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
 
-
 ################################################################################
 # ThreadingGroup
 ################################################################################
@@ -42,6 +67,7 @@ results = ThreadingGroup("web1", "web2", "mac1").run("cmd1; cmd2")  # $getComman
 
 pool = ThreadingGroup("web1", "web2", "web3")
 pool.run("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
+pool.sudo("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
 
 # fully qualified usage
 group.ThreadingGroup("web1", "web2", "mac1").run("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
@@ -57,6 +83,7 @@ def foo(c):
     # 'c' is a fabric.connection.Connection
     c.run("cmd1; cmd2")  # $getCommand="cmd1; cmd2"
 
+
 # fully qualified usage
 @tasks.task
 def bar(c):