search

sybrenstuvel / python-rsa

Python-RSA is a pure-Python RSA implementation.
https://stuvel.eu/rsa
Other
488 stars 114 forks source link

Add initial fuzz test #196

Open DavidKorczynski opened 2 years ago

DavidKorczynski commented 2 years ago

Hi,

I was wondering if you would like to integrate continuous fuzzing of python-rsa by way of OSS-Fuzz? In this PR https://github.com/google/oss-fuzz/pull/7516 I do exactly that, namely created the necessary logic from an OSS-Fuzz perspective to integrate python-rsa.

This includes developing initial fuzzers as well as integrating into OSS-Fuzz, however, it is preferable to have the fuzzers upstream so I included it in this PR - if you are happy with having the fuzzers here then I will remove them from the OSS-Fuzz repository.

Essentially, OSS-Fuzz is a free service run by Google that performs continuous fuzzing of important open source projects. The only expectation of integrating into OSS-Fuzz is that bugs will be fixed. This is not a "hard" requirement in that no one enforces this and the main point is if bugs are not fixed then it is a waste of resources to run the fuzzers, which we would like to avoid.

If you would like to integrate, the only thing I need is as list of email(s) that will get access to the data produced by OSS-Fuzz, such as bug reports, coverage reports and more stats. Notice the emails affiliated with the project will be public in the OSS-Fuzz repo, as they will be part of a configuration file.

sybrenstuvel commented 2 years ago

Thanks for the offer! I'm quite curious to see what the fuzz tests can expose.

As for the pull request, I can't accept it as-is. The atheris module should be listed as optional development dependency, and the test should be gracefully skipped when it cannot be imported.

I'm also curious why the tests simply return on a ValueError or OverflowError. IMO this should be documented in a comment above the respective return statements, so that it's clear for anyone reading the code.

DavidKorczynski commented 2 years ago

Thanks for letting me know you're interested in fuzzing @sybrenstuvel -- I will look to address the issues you mention!

DavidKorczynski commented 2 years ago

The atheris module should be listed as optional development dependency, and the test should be gracefully skipped when it cannot be imported.

Am not sure how you would prefer this. The fuzzer is not meant to be run similar to the other tests in that a fuzzer never really terminates but is meant to be run continuously. I would advice to have the continuous running of it handled by oss-fuzz, and as such it may be better to move it to another directory than tests/? In that case, where would you prefer to have the dependency atheris listed? Is it here https://github.com/sybrenstuvel/python-rsa/blob/76c0e6901cde36743fd6cbb5251a91bfb3a3352d/pyproject.toml#L38 ?

I'm also curious why the tests simply return on a ValueError or OverflowError. IMO this should be documented in a comment above the respective return statements, so that it's clear for anyone reading the code.

I put in comments in the exception handling now, let me know what you think.