Python: New command execution sinks

[am0o0]

trafficstars

What is new? JsonPickle library Code execution sinks Pytorch library Code execution sinks Pexpect library Command Execution and Secondary server cmd injection AsyncSsh library Secondary server cmd injection Netmiko library Secondary server cmd injection Scrapli library Secondary server cmd injection Twisted library Secondary server cmd injection Ssh2-python library Secondary server cmd injection pandas library DataFrame Code execution sinks

What has changed? Upgrade paramiko query to Secondary server command execution query which attackers can execute commands on other than the primary server. it is in the experimental directory. for the paramiko query, it has added proxyCommand as a SystemCommandExecution because it executes commands on the primary server. Upgraded Fabric framework and added proxy_command as a SystemCommandExecution, I didn't change the sinks of this framework to Secondary server command execution because it is not in an experimental library, otherwise the run and sudo functions are SecondaryCommandInjection and local function is SystemCommandExecution. I only simplified the framework structure with new higher-level APIs and added SystemCommandExecution new sinks.

Also, I tried my best to use inline tests everywhere so you can review this PR more easily. :)

[ghsecuritylab]

Hello am0o0 👋 You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

• the submission models widely-used frameworks/libraries
• the vulnerability modeled in the submission is impactful
• the submission finds new true positive vulnerabilities
• the submission finds very few false positives
• code in the submission is easy to read and will be easy to maintain
• documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
• the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

[github-actions[bot]]

QHelp previews:

python/ql/src/experimental/Security/CWE-074/remoteCommandExecution/RemoteCommandExecution.qhelp

Command execution on a secondary remote server

Allowing users to execute arbitrary commands using an SSH connection on a remote server can lead to security issues unless you implement proper authorization.

Assume that you connect to a remote system via SSH connection from your main or local server that accepts user-controlled data and has interaction with users that you don't trust, passing these data to SSH API as a part of a command that will be executed on a secondary remote server can lead to security issues. You should consider proper authorization rules very carefully.

Recommendation

This vulnerability can be prevented by implementing proper authorization rules for untrusted user input that can be passed to your secondary servers.

Example

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

#!/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 withoutAuthorization():
    user_cmd = request.args.get('command')
    stdin, stdout, stderr = paramiko_ssh_client.exec_command(user_cmd)
    return stdout

if __name__ == '__main__':
    app.debug = False
    app.run()

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

#!/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()

[tausbn]

Tests were failing to compile, so I rebased this on top of current main. I'm in the process of reviewing it now, but the fact that it targets our main libraries (and not experimental) means a greater level of scrutiny is required, and it'll therefore take a bit longer than expected.

[tausbn]

Urgh. Rebasing may have been a mistake (I ended up a co-author on all commits, which I wasn't expecting). Feel free to force push your original commits instead, and then merging in main.

[am0o0]

@tausbn I think I should add some predicates for classes that extend the SystemCommandExecution::Range like the SubprocessPopenCall class. do you think I should do it now? or should I wait for the second review round?

[am0o0]

@tausbn Also please let me know if you prefer I move most of the new non-experimental codeql library files under the experimental directory.

[tausbn]

@tausbn I think I should add some predicates for classes that extend the SystemCommandExecution::Range like the SubprocessPopenCall class. do you think I should do it now? or should I wait for the second review round?

Feel free to add them now, if you like. 👍

@tausbn Also please let me know if you prefer I move most of the new non-experimental codeql library files under the experimental directory.

No, I think it's fine the way it is. From what I've seen so far, your modelling is good, and only a few changes here and there should be needed to make it ready to merge. 🙂

One thing I do need to mention is this change: https://github.com/github/codeql/pull/15715/files#diff-950dae083553f4d1115143425b3e4816da96a333a4751463eda140c20156ae5cL97

The predicate in question is used elsewhere, and so its removal is causing the tests to fail (as we can't even compile all the queries). Specifically, It's this file that uses it: https://github.com/github/codeql/blob/main/python/ql/src/meta/ClassHierarchy/Find.ql#L327

[am0o0]

@tausbn I fixed the Fabric library issue that you mentioned( I didn't run a full test yet) you can see the changes in this commit: f93d4a0

the only problem is that I wanted to use the ClassInstantiation class for this part of the query: f93d4a0 but I couldn't do that because it needs the class instances without getReturn() and if I remove this getReturn() from then the ClassInstantiation then the instanceRunMethods won't work. the main reason is that instanceRunMethods cant work for all of Instances when we have something like:(.getReturn() is the problem)

API::CallNode instanceRunMethods() {
          result = any(Instance is).getReturn().getMember(["run", "sudo", "local"]).getACall()
        }

[am0o0]

I think my last question is solved in c7adb32. https://github.com/github/codeql/pull/15715#issuecomment-2103605867

[am0o0]

@tausbn I'm not really good at working with Git :) IDK if it is solved or not.

[tausbn]

I think you solved the merge conflict (at least the tests can run now), but I'm not too sure about the revert in the latest commit.

Also:

python/ql/src/experimental/semmle/python/security/RemoteCommandExecution.qll would change by autoformatting.

(So many hoops to jump through...)