From 385c3ba7ffd4ad0668e9db9ccc9e5f88e498ae6c Mon Sep 17 00:00:00 2001 From: amammad <77095239+amammad@users.noreply.github.com> Date: Sat, 24 Feb 2024 18:30:50 +0400 Subject: [PATCH] continue to convert paramiko query to a more general query, the proxy command is not a secondary command execution so we can add proxy command to SystemCommandExecution::Range, update QLDocs, add a proper Paramiko test case fix a typo --- python/ql/lib/semmle/python/Frameworks.qll | 1 + .../lib/semmle/python/frameworks/Paramiko.qll | 35 +++++++++++++++++ .../SecondaryServerCmdInjection.qhelp | 8 ++-- .../SecondaryServerCmdInjection.ql | 8 ++-- .../secondaryCommandInjection/paramikoBad.py | 15 +------ .../secondaryCommandInjection/paramikoGood.py | 27 +++++++++++++ .../security/SecondaryServerCmdInjection.qll | 39 +++++++------------ .../DataflowQueryTest.ql | 2 +- .../paramiko.py | 2 +- .../frameworks/paramiko/ConceptsTest.expected | 2 + .../frameworks/paramiko/ConceptsTest.ql | 2 + .../frameworks/paramiko/proxy_command.py | 11 ++++++ 12 files changed, 105 insertions(+), 47 deletions(-) create mode 100644 python/ql/lib/semmle/python/frameworks/Paramiko.qll create mode 100644 python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoGood.py create mode 100644 python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.expected create mode 100644 python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.ql create mode 100644 python/ql/test/library-tests/frameworks/paramiko/proxy_command.py diff --git a/python/ql/lib/semmle/python/Frameworks.qll b/python/ql/lib/semmle/python/Frameworks.qll index 85f2ef45060..f8298ffc7b7 100644 --- a/python/ql/lib/semmle/python/Frameworks.qll +++ b/python/ql/lib/semmle/python/Frameworks.qll @@ -46,6 +46,7 @@ private import semmle.python.frameworks.MySQLdb private import semmle.python.frameworks.Numpy private import semmle.python.frameworks.Oracledb private import semmle.python.frameworks.Pandas +private import semmle.python.frameworks.Paramiko private import semmle.python.frameworks.Peewee private import semmle.python.frameworks.Phoenixdb private import semmle.python.frameworks.Psycopg diff --git a/python/ql/lib/semmle/python/frameworks/Paramiko.qll b/python/ql/lib/semmle/python/frameworks/Paramiko.qll new file mode 100644 index 00000000000..e41d4b5ad55 --- /dev/null +++ b/python/ql/lib/semmle/python/frameworks/Paramiko.qll @@ -0,0 +1,35 @@ +/** + * Provides classes modeling security-relevant aspects of the `paramiko` PyPI package. + * See https://pypi.org/project/paramiko/. + */ + +private import python +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.dataflow.new.RemoteFlowSources +private import semmle.python.Concepts +private import semmle.python.ApiGraphs + +/** + * Provides models for the `paramiko` PyPI package. + * See https://pypi.org/project/paramiko/. + */ +private module Paramiko { + /* + * The first argument of `paramiko.ProxyCommand`. + * + * the `paramiko.ProxyCommand` is equivalent of `ssh -o ProxyCommand="CMD"` + * and it run CMD on current system that running the ssh command + * + * See https://paramiko.pydata.org/docs/reference/api/paramiko.eval.html + */ + + class ParamikoProxyCommand extends SystemCommandExecution::Range, API::CallNode { + ParamikoProxyCommand() { + this = API::moduleImport("paramiko").getMember("ProxyCommand").getACall() + } + + override DataFlow::Node getCommand() { result = this.getParameter(0, "command_line").asSink() } + + override predicate isShellInterpreted(DataFlow::Node arg) { none() } + } +} diff --git a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.qhelp b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.qhelp index 98cc0e1e4de..002ab280729 100644 --- a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.qhelp +++ b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.qhelp @@ -2,16 +2,18 @@

- Processing an unvalidated user input can allow an attacker to inject arbitrary command in your local and remote servers when creating a ssh connection. + Running user-controlled values into a secondary remote servers without proper authorization can allow an attacker to inject arbitrary command in the secondary remote servers from within your primary remote servers.

- This vulnerability can be prevented by not allowing untrusted user input to be passed as ProxyCommand or exec_command. + This vulnerability can be prevented by implementing proper authorization rules for untrusted user input that can be passed to your secondary servers.

-

In the example below, the ProxyCommand and exec_command are controlled by the user and hence leads to a vulnerability.

+

In the example below, the exec_command is controlled by the user and hence leads to a vulnerability.

+

In the example below, the exec_command is controlled by the an Authorized user and hence it is safe.

+ diff --git a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.ql b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.ql index fa444f8bdff..bbec313049f 100644 --- a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.ql +++ b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.ql @@ -1,5 +1,5 @@ /** - * @name RCE with user provided command with paramiko ssh client + * @name Command execution on a secondary remote server * @description user provided command can lead to execute code on a external server that can be belong to other users or admins * @kind path-problem * @problem.severity error @@ -13,9 +13,9 @@ import python import experimental.semmle.python.security.SecondaryServerCmdInjection -import ParamikoFlow::PathGraph +import SecondaryCommandInjectionFlow::PathGraph -from ParamikoFlow::PathNode source, ParamikoFlow::PathNode sink -where ParamikoFlow::flowPath(source, sink) +from SecondaryCommandInjectionFlow::PathNode source, SecondaryCommandInjectionFlow::PathNode sink +where SecondaryCommandInjectionFlow::flowPath(source, sink) select sink.getNode(), source, sink, "This code execution depends on a $@.", source.getNode(), "a user-provided value" diff --git a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoBad.py b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoBad.py index b54a88f2e4a..ca32aea9eef 100644 --- a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoBad.py +++ b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoBad.py @@ -12,24 +12,11 @@ paramiko_ssh_client.connect(hostname="127.0.0.1", port="22", username="ssh_user_ @app.route('/external_exec_command_1') -def bad1(): +def withoutAuthorization(): user_cmd = request.args.get('command') stdin, stdout, stderr = paramiko_ssh_client.exec_command(user_cmd) return stdout -@app.route('/external_exec_command_2') -def bad2(): - user_cmd = request.args.get('command') - stdin, stdout, stderr = paramiko_ssh_client.exec_command(command=user_cmd) - return stdout - - -@app.route('/proxycommand') -def bad2(): - user_cmd = request.args.get('command') - stdin, stdout, stderr = paramiko_ssh_client.connect('hostname', username='user',password='yourpassword',sock=paramiko.ProxyCommand(user_cmd)) - return stdout - if __name__ == '__main__': app.debug = False app.run() diff --git a/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoGood.py b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoGood.py new file mode 100644 index 00000000000..b7ce5bfaf7f --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoGood.py @@ -0,0 +1,27 @@ +#!/usr/bin/env python + +from flask import request, Flask +import paramiko +from paramiko import SSHClient + +app = Flask(__name__) +paramiko_ssh_client = SSHClient() +paramiko_ssh_client.load_system_host_keys() +paramiko_ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) +paramiko_ssh_client.connect(hostname="127.0.0.1", port="22", username="ssh_user_name", pkey="k", timeout=11, banner_timeout=200) + + +@app.route('/external_exec_command_1') +def withAuthorization(): + user_cmd = request.args.get('command') + auth_jwt = request.args.get('Auth') + # validating jwt token first + # .... then continue to run the command + stdin, stdout, stderr = paramiko_ssh_client.exec_command(user_cmd) + return stdout + + +if __name__ == '__main__': + app.debug = False + app.run() + diff --git a/python/ql/src/experimental/semmle/python/security/SecondaryServerCmdInjection.qll b/python/ql/src/experimental/semmle/python/security/SecondaryServerCmdInjection.qll index 6926e09a684..7f3b29e7de7 100644 --- a/python/ql/src/experimental/semmle/python/security/SecondaryServerCmdInjection.qll +++ b/python/ql/src/experimental/semmle/python/security/SecondaryServerCmdInjection.qll @@ -5,6 +5,9 @@ import semmle.python.ApiGraphs import semmle.python.dataflow.new.internal.DataFlowPublic import codeql.util.Unit +/** + * Provides sinks and additional taint steps for the secondary command injection configuration + */ module SecondaryCommandInjection { /** * The additional taint steps that need for creating taint tracking or dataflow. @@ -22,36 +25,24 @@ module SecondaryCommandInjection { abstract class Sink extends DataFlow::Node { } } +/** + * The exec_command of `paramiko.SSHClient` class execute command on ssh target server + */ +class ParamikoExecCommand extends SecondaryCommandInjection::Sink { + ParamikoExecCommand() { + this = paramikoClient().getMember("exec_command").getACall().getParameter(0, "command").asSink() + } +} + private API::Node paramikoClient() { result = API::moduleImport("paramiko").getMember("SSHClient").getReturn() } -module ParamikoConfig implements DataFlow::ConfigSig { +module SecondaryCommandInjectionConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - /** - * exec_command of `paramiko.SSHClient` class execute command on ssh target server - * the `paramiko.ProxyCommand` is equivalent of `ssh -o ProxyCommand="CMD"` - * and it run CMD on current system that running the ssh command - * the Sink related to proxy command is the `connect` method of `paramiko.SSHClient` class - */ - predicate isSink(DataFlow::Node sink) { - sink = paramikoClient().getMember("exec_command").getACall().getParameter(0, "command").asSink() - or - sink = paramikoClient().getMember("connect").getACall().getParameter(11, "sock").asSink() - } - - /** - * this additional taint step help taint tracking to find the vulnerable `connect` method of `paramiko.SSHClient` class - */ - predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - exists(API::CallNode call | - call = API::moduleImport("paramiko").getMember("ProxyCommand").getACall() and - nodeFrom = call.getParameter(0, "command_line").asSink() and - nodeTo = call - ) - } + predicate isSink(DataFlow::Node sink) { sink instanceof SecondaryCommandInjection::Sink } } /** Global taint-tracking for detecting "paramiko command injection" vulnerabilities. */ -module ParamikoFlow = TaintTracking::Global; +module SecondaryCommandInjectionFlow = TaintTracking::Global; diff --git a/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/DataflowQueryTest.ql b/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/DataflowQueryTest.ql index 88dd0ffb6f1..f4773e0e3af 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/DataflowQueryTest.ql +++ b/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/DataflowQueryTest.ql @@ -1,4 +1,4 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest import experimental.semmle.python.security.SecondaryServerCmdInjection -import FromTaintTrackingConfig +import FromTaintTrackingConfig diff --git a/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/paramiko.py b/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/paramiko.py index dd873fb27e6..2ffa4ab2d36 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/paramiko.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-074-SecondaryServerCmdInjection/paramiko.py @@ -23,5 +23,5 @@ async def read_item(cmd: str): @app.get("/bad3") async def read_item(cmd: str): - stdin, stdout, stderr = paramiko_ssh_client.connect('hostname', username='user',password='yourpassword',sock=paramiko.ProxyCommand(cmd)) # $ result=BAD + paramiko_ssh_client.connect('hostname', username='user',password='yourpassword',sock=paramiko.ProxyCommand(cmd)) # $ result=BAD return {"success": "OK"} diff --git a/python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.expected b/python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.expected new file mode 100644 index 00000000000..8ec8033d086 --- /dev/null +++ b/python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.expected @@ -0,0 +1,2 @@ +testFailures +failures diff --git a/python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.ql b/python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.ql new file mode 100644 index 00000000000..b557a0bccb6 --- /dev/null +++ b/python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.ql @@ -0,0 +1,2 @@ +import python +import experimental.meta.ConceptsTest diff --git a/python/ql/test/library-tests/frameworks/paramiko/proxy_command.py b/python/ql/test/library-tests/frameworks/paramiko/proxy_command.py new file mode 100644 index 00000000000..a43cee59636 --- /dev/null +++ b/python/ql/test/library-tests/frameworks/paramiko/proxy_command.py @@ -0,0 +1,11 @@ +#!/usr/bin/env python + +import paramiko +from paramiko import SSHClient +paramiko_ssh_client = SSHClient() +paramiko_ssh_client.load_system_host_keys() +paramiko_ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) +paramiko_ssh_client.connect(hostname="127.0.0.1", port="22", username="ssh_user_name", pkey="k", timeout=11, banner_timeout=200) + +cmd = "cmd" +paramiko_ssh_client.connect('hostname', username='user', password='yourpassword', sock=paramiko.ProxyCommand(cmd)) # $getCommand=cmd