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

Add FuzzXxx tests to cipher algorithms #480

Open
raklaptudirm opened this issue Mar 24, 2022 · 3 comments
Open

Add FuzzXxx tests to cipher algorithms #480

raklaptudirm opened this issue Mar 24, 2022 · 3 comments
Labels
dont-close enhancement Good First Issue Good issue to get started with for new comers. help wanted

Comments

@raklaptudirm
Copy link
Member

Description
Use the new go1.18 FuzzXxx tests to test cipher algorithm encryption and decryption.
The tests should accept fuzzed strings, encrypt then decrypt them, and check for equality with the original. This can help catch more elusive bugs in the code.

@HectorMalot
Copy link

I looked at this for a bit, but have a few questions for the fuzzing. As I'm writing the tests, I'm finding some edge cases and wanted to ask first if/how you want algorithms to change

  1. For rot13/caesar encryption: the current algorithms only expect a-zA-Z as input, but don't validate this. This can be either be solved on the fuzzer (only allowing valid inputs), or on the algorithm (providing a helpful error is an input is outside of the expected range. Any preference or other ideas?

  2. Similarly, RSA encrypt/decrypt as currently tested fails on certain inputs. Specifically, in the test p and q are set to 61 and 53, leading to a modulus of 3233. e.g. try encrypting and then decrypting string("\x82") with these exponents. This fails because the expected output is int64(65533) but the actual output is int64(873), which is equal to 65533%3233. Notably, every input above rune(3232) will fail because the modulus is not large enough.

    • Option 1: Encrypt byte by byte (the current string-rune conversion can sometimes return a different output than expected)
    • Option 2: invalidate modulus smaller than MaxInt64
    • Option 3: invalidate inputs with runes larger than the provided modulus
    • Option 4: leave as is, update test to larger modulus (I'll share a PR that does this a bit later)

Any preference / ideas on the above?

@siriak
Copy link
Member

siriak commented Oct 14, 2022

  1. Validate input in the algorithm
  2. Option 1 or 2 whichever is simpler

@mcaci
Copy link
Contributor

mcaci commented Oct 31, 2022

Hi everyone, I have been adding some fuzz tests lately to contribute to this issue. I think now once these PRs are good on your side and merged there should be no more work to do on this issue. Let me know if you have any feedbacks about this. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-close enhancement Good First Issue Good issue to get started with for new comers. help wanted
Projects
None yet
Development

No branches or pull requests

5 participants