add some python sinks for paramiko ssh clients#12220
Conversation
|
Hi, I forgot to put comments, if there is any need for comments please ping me about that. |
yoff
left a comment
There was a problem hiding this comment.
Sorry for the long wait, this sparked a surprisingly lengthy internal debate...the code is fine, though, things have been modelled as sinks and additional flow steps appropriately :-)
You give a straightforward implementation of some new sinks with clear examples.
In a full implementation, we should include sanitisers, but this will just be folded into an existing query, so need.
We are moving towards using inline test expectations, but again the tests here are so small, that they can easily be folded into the existing query's tests.
What we usually like is, as you point out, comments. In this case there is not much complexity to comment on, but it is always good to include references to the api that is being modelled, in this case the particular paramiko methods used as sinks and the method mentioned in the additional flow step.
I have approved the PR, since it is easy enough to take from here. But if you want to improve upon it, I am happy to review further iterations. Just let me know to merge it or to wait a bit :-)
| override predicate isSink(DataFlow::Node sink) { | ||
| sink = | ||
| [ | ||
| API::moduleImport("paramiko") | ||
| .getMember("SSHClient") | ||
| .getReturn() | ||
| .getMember("exec_command") | ||
| .getACall() | ||
| .getArgByName("command"), | ||
| API::moduleImport("paramiko") | ||
| .getMember("SSHClient") | ||
| .getReturn() | ||
| .getMember("exec_command") | ||
| .getACall() | ||
| .getArg(0) | ||
| ] | ||
| or | ||
| sink = | ||
| [ | ||
| API::moduleImport("paramiko") | ||
| .getMember("SSHClient") | ||
| .getReturn() | ||
| .getMember("connect") | ||
| .getACall() | ||
| .getArgByName("sock"), | ||
| API::moduleImport("paramiko") | ||
| .getMember("SSHClient") | ||
| .getReturn() | ||
| .getMember("connect") | ||
| .getACall() | ||
| .getArg(11) | ||
| ] | ||
| } |
There was a problem hiding this comment.
This code is fine as is, but I would refactor it through a private predicate to make it clearer what is happening.
Also, we can use the member predicate getParameter that takes both an index and a name:
| override predicate isSink(DataFlow::Node sink) { | |
| sink = | |
| [ | |
| API::moduleImport("paramiko") | |
| .getMember("SSHClient") | |
| .getReturn() | |
| .getMember("exec_command") | |
| .getACall() | |
| .getArgByName("command"), | |
| API::moduleImport("paramiko") | |
| .getMember("SSHClient") | |
| .getReturn() | |
| .getMember("exec_command") | |
| .getACall() | |
| .getArg(0) | |
| ] | |
| or | |
| sink = | |
| [ | |
| API::moduleImport("paramiko") | |
| .getMember("SSHClient") | |
| .getReturn() | |
| .getMember("connect") | |
| .getACall() | |
| .getArgByName("sock"), | |
| API::moduleImport("paramiko") | |
| .getMember("SSHClient") | |
| .getReturn() | |
| .getMember("connect") | |
| .getACall() | |
| .getArg(11) | |
| ] | |
| } | |
| private API::Node paramikoClient() { | |
| result = API::moduleImport("paramiko").getMember("SSHClient").getReturn() | |
| } | |
| override 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() | |
| } |
(you may need to run autoformat to break the lines.)
There was a problem hiding this comment.
I'll make the Improvements according to your suggestion. And I'd like to write some comments too.
|
I made some changes, I didn't understand this part of your comments
Could you please elaborate more? |
I see that I have a typo there, it should say "so no need". If we used the configuration as presented in this PR, it would likely give many false positives because it does not recognise that a lot of code is already protected as it validates the input in some way. However, the taint configuration, that we will add these sinks to, already includes sanitisers so you do not need to repeat those here :-) |
yoff
left a comment
There was a problem hiding this comment.
Thanks for the extra comment 👍
|
QHelp previews: python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.qhelpRCE with user provided command with paramiko ssh clientProcessing an unvalidated user input can allow an attacker to inject arbitrary command in your local and remote servers when creating a ssh connection. RecommendationThis vulnerability can be prevented by not allowing untrusted user input to be passed as ProxyCommand or exec_command. ExampleIn the example below, the ProxyCommand and exec_command are 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 bad1():
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()
|
No description provided.