pyre-check icon indicating copy to clipboard operation
pyre-check copied to clipboard

added models for python-sh

Open esiebomaj opened this issue 3 years ago • 5 comments

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 avatar Oct 25 '21 20:10 esiebomaj

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 avatar Nov 08 '21 18:11 esiebomaj

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! :)

0xedward avatar Nov 08 '21 19:11 0xedward

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

facebook-github-bot avatar Nov 08 '21 19:11 facebook-github-bot

okay Thanks @0xedward

esiebomaj avatar Nov 08 '21 19:11 esiebomaj

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 https://github.com/facebook/pyre-check/commit/aa1034dcfdb04c99450999dfcae8cc42fc079c4e. 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": [
    "."
  ]
}

0xedward avatar Dec 02 '21 06:12 0xedward