Python: New command execution sinks

[am0o0]

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!

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

[tausbn]

This is not right. classRef should be a reference to the class, whereas Instance is for instances of said class. These are not the same thing, and this is important for this file, as it is explicitly about finding references to (subclasses of) known classes, not instances of those classes.

Please just put classRef() back in place in Fabric.qll.

(Also, don't forget to autoformat your code. This file and Torch.qll failed the autoformat check.)

[am0o0]

I think my last question is solved in c7adb32.
#15715 (comment)

[tausbn]

Thank you for your submission. Before we can merge this, there are a few comments that need to be addressed.

Once you've addressed the requested changes, I'll re-run the tests and also kick off a performance evaluation (which is needed because you've made changes to files outside the experimental subdirectory).

Finally, this would probably have been better as two separate PRs, as the command execution sinks are mostly disjoint from your work on secondary command injection.