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

Adapt existing sig fuzz harness including more algorithms #1955

Merged

Conversation

nathaniel-brough
Copy link
Contributor

Adapts existing fuzz-harness to support more algorithms.

This is a continuation of the effort described in #1215

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@nathaniel-brough
Copy link
Contributor Author

The failing check doesn't appear to be related to this PR. I'm unsure but it kind of looks like a flaky performance check.

@cothan
Copy link
Contributor

cothan commented Oct 22, 2024

It's seem like you found a rare case that the verify operation failed to verify the signature.
I hope the bug lies in the implementation, not the algorithm itself. I don't know about CROSS signature but this one is concerning.

if (OQS_SIG_verify(sig, message, message_len, signature, signature_len, public_key) != OQS_SUCCESS) {

@nathaniel-brough
Copy link
Contributor Author

Hmm ok. It doesn't look like the public/private keypair or the message is saved on failure here, so I guess it'll be a little difficult to reproduce.

While this seems unrelated to the PR, I'll see if I can modify my fuzzer in such a way to trigger the bug in a way that's reproducible.

@nathaniel-brough
Copy link
Contributor Author

nathaniel-brough commented Oct 22, 2024

Ok I've updated this fuzz harness (locally) such that it might be able to find this bug in a deterministic way. Is there a preference on stacking changes or just drip feeding in changes so that each PR is atomic i.e. doing one thing?

@SWilson4
Copy link
Member

Looping in @alexrow and @rtjk as the maintainers of CROSS.

@SWilson4 SWilson4 mentioned this pull request Oct 22, 2024
@rtjk
Copy link
Contributor

rtjk commented Oct 22, 2024

Thank you @SWilson4, we're taking a look at this verification failure and will respond soon.

@rtjk
Copy link
Contributor

rtjk commented Oct 23, 2024

I managed to reproduce this very rare verification error in CROSS-RSDPG-128-small. I'm still looking for the root cause but I can confirm that it seems totally unrelated to the new fuzzing harness.

@SWilson4
Copy link
Member

I managed to reproduce this very rare verification error in CROSS-RDSDP-128-small. I'm still looking for the root cause but I can confirm that it seems totaly unrelated to the new fuzzing harness.

Thanks for taking a look! I'll open a separate issue to track the error, then.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
@nathaniel-brough nathaniel-brough force-pushed the add_more_sig_fuzzers branch 2 times, most recently from 606269f to 6e43a56 Compare October 30, 2024 05:00
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

LGTM.

@SWilson4
Copy link
Member

SWilson4 commented Nov 1, 2024

Thanks for the contribution @nathaniel-brough!

@dstebila dstebila merged commit 60af4a9 into open-quantum-safe:main Nov 1, 2024
72 checks passed
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.

6 participants