From d234a53c501470e52c7af4382cf44fc0eaf40d0c Mon Sep 17 00:00:00 2001 From: amammad <77095239+amammad@users.noreply.github.com> Date: Sat, 24 Feb 2024 17:43:51 +0400 Subject: [PATCH] update Fabric models, add new sink to Fabric, add proper test cases --- .../lib/semmle/python/frameworks/Fabric.qll | 103 ++++++++---------- .../frameworks/fabric/fabric_v2_test.py | 31 +++++- 2 files changed, 77 insertions(+), 57 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Fabric.qll b/python/ql/lib/semmle/python/frameworks/Fabric.qll index 06d1de6590d..4562327a24c 100644 --- a/python/ql/lib/semmle/python/frameworks/Fabric.qll +++ b/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() } } diff --git a/python/ql/test/library-tests/frameworks/fabric/fabric_v2_test.py b/python/ql/test/library-tests/frameworks/fabric/fabric_v2_test.py index 75abc2ce715..0334458273f 100644 --- a/python/ql/test/library-tests/frameworks/fabric/fabric_v2_test.py +++ b/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):