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

OSS-Fuzz: OSS-Fuzz fuzzing integration #534

Merged
merged 13 commits into from
Oct 23, 2024

Conversation

arthurscchan
Copy link
Contributor

Hi! Would you be interested in setting up fuzzing for the Scrypt module via OSS-Fuzz?

Fuzzing is essentially a stress-testing approach used to find bugs in software, and OSS-Fuzz is a free service run by Google that continuously fuzzes important open-source projects. Integrating your module with OSS-Fuzz could help uncover memory corruption issues that may exist.

This PR adds a Cargo fuzz configuration along with a fuzzer for the Scrypt module. In combination with an initial attempt in OSS-Fuzz (google/oss-fuzz#12629), it enables OSS-Fuzz to fuzz the Scrypt module while keeping the fuzzers upstream for further modification and expansion. If you're happy to proceed with the integration and store the fuzzers upstream, please let me know, and I'd be glad to provide more details if needed.

The only thing required at this point is an email associated with a Google account, which will be used to receive notifications when bugs are found.

Signed-off-by: Arthur Chan <[email protected]>
@arthurscchan arthurscchan marked this pull request as ready for review October 21, 2024 17:01
@newpavlov
Copy link
Member

Looks good! But I think it's better to have a fuzz crate in the repository root excluded from the main workspace.

Signed-off-by: Arthur Chan <[email protected]>
@arthurscchan
Copy link
Contributor Author

Thanks for the suggestion. I have relocated the fuzz directory and fixes the Cargo.toml respectively.

Cargo.toml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

@arthurscchan we had some other projects added to oss-fuzz but the contributor disappeared. They now send me emails periodically to say it's broken and I don't have time to look into fixing it.

If we do this integration, are you interested in maintaining it going forward?

fuzz/Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Arthur Chan <[email protected]>
fuzz/Cargo.toml Outdated Show resolved Hide resolved
fuzz/Cargo.toml Outdated Show resolved Hide resolved
fuzz/Cargo.toml Outdated Show resolved Hide resolved
@DavidKorczynski
Copy link

@arthurscchan we had some other projects added to oss-fuzz but the contributor disappeared. They now send me emails periodically to say it's broken and I don't have time to look into fixing it.

If we do this integration, are you interested in maintaining it going forward?

We're happy to maintain it's building!

Which are the projects you're referring to, we could take a look at getting them build again.

@tarcieri
Copy link
Member

@DavidKorczynski the original PR was here: google/oss-fuzz#6908

Here are the failures: https://issues.oss-fuzz.com/issues/42515273

Signed-off-by: Arthur Chan <[email protected]>
Signed-off-by: Arthur Chan <[email protected]>
Signed-off-by: Arthur Chan <[email protected]>
.github/workflows/fuzz_build.yml Outdated Show resolved Hide resolved
fuzz/fuzz_targets/scrypt_fuzzer.rs Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member

@arthurscchan
I've split the fuzz target into 3 separate targets. I think it results in a clearer code and slightly more efficient fuzzing. Please verify that I've translated your intention properly. Note that there are some minor differences, e.g. SaltString is created from raw bytes instead of parsing fuzer-provided B64 string.

@arthurscchan
Copy link
Contributor Author

arthurscchan commented Oct 23, 2024

@arthurscchan I've split the fuzz target into 3 separate targets. I think it results in a clearer code and slightly more efficient fuzzing. Please verify that I've translated your intention properly. Note that there are some minor differences, e.g. SaltString is created from raw bytes instead of parsing fuzer-provided B64 string.

Thanks for helping me to fix the fuzzers. I this it is good.

@newpavlov
Copy link
Member

I merged the fuzz targets back into one. Now in addition to direct hashing using the scrypt function, it performs PHC hashing and verification of its result, i.e. instead of the fixed string we use serialized PHC hash for random password, salt, and parameters.

@newpavlov newpavlov merged commit d44ff8f into RustCrypto:master Oct 23, 2024
58 checks passed
@arthurscchan
Copy link
Contributor Author

Thanks. @newpavlov. For the OSS-Fuzz integration, could we use the email from your GH page as contact, such that you will receive notifications if issues are found?

@DavidKorczynski
Copy link

@DavidKorczynski the original PR was here: google/oss-fuzz#6908

Here are the failures: https://issues.oss-fuzz.com/issues/42515273

Thanks!

We're happy to maintain this integration (password-hashes) but I think rustcrypto might be tricky because it relies on Cryptofuzz. @guidovranken is the right person to ask here -- @guidovranken do you about the build failures of rustcrypto?

DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Oct 23, 2024
This PR initialises OSS-Fuzz integration for the scrypt project in Rust.
New fuzzers have been created, and a PR
(RustCrypto/password-hashes#534) has been
submitted upstream to merge the fuzzers.

---------

Signed-off-by: Arthur Chan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants