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

Adds verify taint config only option #740

Conversation

abishekvashok
Copy link
Contributor

@abishekvashok abishekvashok commented May 31, 2023

Adds verify_taint_config_only option to the analyze command that just verifies taint.config files and skips the analysis. This can be useful in the future when we want to verify taint config files via the vs code extension without performing analysis or even any other sort of preprocessing.

Modifies the ocaml server and the python client to pass options for the same.

Pre-submission checklist

  • I've ran 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

Test Plan

  • Ran pysa with the default taint.config on documentation/pysa_tutorial/exercise1/:
Screenshot 2023-05-31 at 3 48 17 PM
  • Command: python3 -m pyre-check.client.pyre -n analyze --verify-taint-config-only
Screenshot 2023-05-31 at 3 41 33 PM
  • Modify taint.config to induce a TaintConfigurationError:
{
  "sources": [
    {
      "name": "CustomUserControlled",
      "comment": "use to annotate user input"
    }
  ],

  "sinks": [
    {
      "name": "CodeExecution",
      "comment": "use to annotate execution of python code"
    }
  ],

  "features": [],

  "rules": [
    {
      "name": "Possible RCE:",
      "code": 5001,
      "sources": [ "CustomUserControlled" ],
      "sinks": [ "CodeExecution" ],
      "message_format": "User specified data may reach a code execution sink"
    },
    {
      "name": "test-duplicate",
      "code": 5001,
      "sources": [ "CustomUserControlled" ],
      "sinks": [ "CodeExecution" ],
      "message_format": "duplicate"
    }
  ]
}

Command: python3 -m pyre-check.client.pyre -n analyze --verify-taint-config-only

Screenshot 2023-05-31 at 3 42 49 PM
  • Github actions. (pysa action was failing before this PR)

Fixes part of MLH-Fellowship#82
Signed-off-by: Abishek V Ashok [email protected]

Copy link

@tianhan0 tianhan0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for proposing this! I just noted a few minor things

Comment on lines 321 to 325
Log.log_exception "Taint analysis failed." exn (Worker.exception_backtrace exn);
raise exn

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arthaud Do you have a second opinion if we can do better for the logging?

source/command/analyzeCommand.ml Outdated Show resolved Hide resolved
source/interprocedural_analyses/taint/taintAnalysis.ml Outdated Show resolved Hide resolved
@abishekvashok abishekvashok force-pushed the taint-config-validate-command branch from cf8f7ae to 56f3baa Compare June 1, 2023 06:29
@abishekvashok
Copy link
Contributor Author

abishekvashok commented Jun 1, 2023

@tianhan0 thanks for the review. Made the changes, hopefully, as suggested. Back to you if it pleases you :)

Copy link

@tianhan0 tianhan0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just left a nitpick that hope you can address.

Also, do you know why this test failed: https://github.com/facebook/pyre-check/actions/runs/5141096156/jobs/9253212045?pr=740? Is this expected?

source/command/analyzeCommand.ml Outdated Show resolved Hide resolved
@abishekvashok
Copy link
Contributor Author

@tianhan0

Also, do you know why this test failed: https://github.com/facebook/pyre-check/actions/runs/5141096156/jobs/9253212045?pr=740? Is this expected?

That test has been failing for a long time and is unrelated to this PR. I think, from some tests that I did that the setup ocaml action actually uses caches which need to be invalidated now. I tried to run the action by manually setting up opam with a package manager instead of the actions and the build doesn't error out as it does in the main repo. But then without the action, setting up the switch takes a large chunk of time. I think there is a way to specify a cache key that is invalidated for a specific amount of time. I can look into this further if you want.

Just left a nitpick that hope you can address.

Ofcourse, I can address it. Thanks for the valuable suggestions and reviews. Made the change!

@abishekvashok abishekvashok force-pushed the taint-config-validate-command branch from 56f3baa to 3702746 Compare June 2, 2023 06:21
source/command/analyzeCommand.ml Outdated Show resolved Hide resolved
source/command/analyzeCommand.ml Outdated Show resolved Hide resolved
source/interprocedural_analyses/taint/taintAnalysis.ml Outdated Show resolved Hide resolved
Adds verify_taint_config_only option to the analyze command that just
verifies taint.config files and skips the analysis. This can be useful
in the future when we want to verify taint config files via the vs code
extension without performing analysis or even any other sort of
preprocessing.

Modifies the ocaml server and the python client to pass options for the
same.

Signed-off-by: Abishek V Ashok <[email protected]>
@arthaud
Copy link
Contributor

arthaud commented Jun 5, 2023

LGTM!

@facebook-github-bot
Copy link
Contributor

@arthaud has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@arthaud merged this pull request in 0877a76.

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.

4 participants