Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added models for python-sh #518

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

esiebomaj
Copy link

@esiebomaj esiebomaj commented Oct 25, 2021

Pre-submission checklist

  • I've run the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
    • pre-commit run

Summary

This PR adds pysa models for python-sh

Python-sh is a full-fledged replacement for subprocess in python. This might allow arbitrary command execution if user-controlled data is allowed to flow to some of this module's functions
therefore It is important to have pysa models for this library in other to prevent this occurrence

Test Plan

For testing, I have included some functions in documentation\deliberately_vulnerable_flask_app which uses python-sh and may potentially cause RCE

cd documentation\deliberately_vulnerable_flask_app
run pyre analyze --no-verify to see the vulnerabilities pysa has detected in the app.py

Related Issue: #MLH-Fellowship-issue-57

@esiebomaj
Copy link
Author

A few notes below

The python-sh library is included in the virtual enviroments site-packages as a single module (python file) sh.py
This makes it difficult for pysa to find it

To ensure pysa picks the module, you should create an sh directory within you site-packages and add the sh.py file also add __init__.py to this directory

Example

-site-packages
    - sh
        - __init__.py
	- sh.py

This affects the way I import sh in the documentation\deliberately_vulnerable_flask_app\app.py from sh import sh instead of just import sh
also the models are written as sh.sh.Command():... insead of just sh.Command():... etc

@esiebomaj esiebomaj marked this pull request as ready for review November 8, 2021 18:24
@0xedward
Copy link
Contributor

0xedward commented Nov 8, 2021

We're looking into the bug with Pyre not picking up sh.py in site-packages if sh.py is not included as a module in the venv. We'll try to fix the bug before landing this coverage improvement. Hopefully, we'll be able to fix this soon. Thanks for working on this and helping us find this bug @esiebomaj! :)

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@esiebomaj
Copy link
Author

esiebomaj commented Nov 8, 2021

okay
Thanks @0xedward

@0xedward
Copy link
Contributor

0xedward commented Dec 2, 2021

We're looking into the bug with Pyre not picking up sh.py in site-packages if sh.py is not included as a module in the venv.

Hey @esiebomaj! @shannonzhu landed a fix for this in aa1034d. Would you mind updating your local pyre config to match the following and let us know if Pyre is able to pick up sh? :)

{
  "taint_models_path": "../../stubs",
  "search_path": [
    "../../stubs",
    {
      "site-package": "sh",
      "is_toplevel_module": true
    },
    {
      "site-package": "flask"
    },
  ],
  "source_directories": [
    "."
  ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants