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

Author: am0o0

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

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() }
+  }
+}

python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/SecondaryServerCmdInjection.qhelp

@@ -2,16 +2,18 @@
 <qhelp>
     <overview>
         <p>
-            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.
         </p>
     </overview>
     <recommendation>
         <p>
-            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.
         </p>
     </recommendation>
     <example>
-        <p>In the example below, the ProxyCommand and exec_command are controlled by the user and hence leads to a vulnerability.</p>
+        <p>In the example below, the exec_command is controlled by the user and hence leads to a vulnerability.</p>
         <sample src="paramikoBad.py" />
+        <p>In the example below, the exec_command is controlled by the an Authorized user and hence it is safe.</p>
+        <sample src="paramikoGood.py" />
     </example>
 </qhelp>

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"

python/ql/src/experimental/Security/CWE-074/secondaryCommandInjection/paramikoBad.py

@@ -12,24 +12,11 @@
 
 
 @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()

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()
+

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<ParamikoConfig>;
+module SecondaryCommandInjectionFlow = TaintTracking::Global<SecondaryCommandInjectionConfig>;

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<ParamikoConfig>
+import FromTaintTrackingConfig<SecondaryCommandInjectionConfig>

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"}

python/ql/test/library-tests/frameworks/paramiko/ConceptsTest.expected

@@ -0,0 +1,2 @@
+testFailures
+failures