Skip to content

[Fuzz] Poly1305 differential fuzzing against Floodyberry's implementation #23322

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[Fuzz] Poly1305 differential fuzzing against Floodyberry's implementation #23322

wants to merge 3 commits into from

Conversation

prakash1512
Copy link
Contributor

This PR compares Bitcoin Core's implementation of Poly1305 with Floodyberry's public domain implementation in order to find implementation discrepancies if any.

Instructions to test this PR:
Step1: Fetch the pull request
Step2: Switch to the pull request branch and run the following commands:

$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ FUZZ=crypto_diff_fuzz_poly1305 src/test/fuzz/fuzz

@fanquake fanquake added the Tests label Oct 20, 2021
laanwj
laanwj reviewed Oct 20, 2021
View reviewed changes
@prakash1512 prakash1512 force-pushed the diff-fuzzing-poly1305 branch 3 times, most recently from 6331453 to d2af151 Compare October 20, 2021 21:22
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 21, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK practicalswift
Stale ACK laanwj, stratospher

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@practicalswift
Copy link
Contributor

practicalswift commented Oct 21, 2021 •
edited
Loading

Concept ACK on introducing more differential fuzzing

Ideas for further differential fuzzing:

  • It would be nice to have differential fuzzing for CRIPEMD160, CSHA1, CSHA256, CHash160 and CHash256: all which are used in EvalScript.
  • Also, it would be nice to have differential fuzzing of src/crypto/sha256.cpp (internal implementation) vs src/crypto/sha256_{avx2,shani,sse41,sse4}.cpp. The self-test SelfTest() in src/crypto/sha256.cpp might serve as inspiration.

@DrahtBot DrahtBot mentioned this pull request Oct 21, 2021
Merged
@laanwj
Copy link
Member

laanwj commented Oct 21, 2021

Code review ACK d2af151

@DrahtBot DrahtBot mentioned this pull request Nov 5, 2021
Closed
prakash1512 and others added 3 commits April 2, 2022 21:17
@prakash1512 @stratospher
Add Floodyberry's poly1305 implementation
5ac6d8f 
Co-authored-by: Ruhi <44024636+stratospher@users.noreply.github.com>
@prakash1512 @stratospher
Change in reference implementation
3f00cdb 
Co-authored-by: Ruhi <44024636+stratospher@users.noreply.github.com>
@prakash1512 @stratospher
Add fuzzing harness and Makefile
a208e06 
Co-authored-by: Ruhi <44024636+stratospher@users.noreply.github.com>
@stratospher stratospher force-pushed the diff-fuzzing-poly1305 branch from d2af151 to a208e06 Compare April 2, 2022 15:52
@DrahtBot DrahtBot mentioned this pull request Jun 15, 2022
Closed
This was referenced Jun 25, 2022
Closed
Closed
@DrahtBot DrahtBot mentioned this pull request Jul 19, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Sep 13, 2022
Closed
@achow101
Copy link
Member

Are you still working on this?

@stratospher
Copy link
Contributor

stratospher commented Oct 17, 2022 •
edited
Loading

Yes, I'm still working on this. I'm looking for concept ACKs for this approach. Also wondering whether differential fuzzing between c++ and python implementations would be more valuable? (See #23441 (comment) also.)

I'll update soon regarding the ci fuzz crash.
EDIT: it's only missing some sanitizer suppressions.

@sipa
Copy link
Member

sipa commented Oct 17, 2022

I don't think differential fuzzing with a Python implementation is worth it (much slower, and much harder due to cross-language comparing). There is also nothing particularly authoritative about any given Python implementation; in the end it's comparison with actually-deployed poly1305 code we want.

@stratospher
Copy link
Contributor

I don't think differential fuzzing with a Python implementation is worth it (much slower, and much harder due to cross-language comparing). There is also nothing particularly authoritative about any given Python implementation; in the end it's comparison with actually-deployed poly1305 code we want.

I was thinking of the python implementation in the functional testing framework but this makes sense.
Closing this PR until we find a better way to test.

@sipa
Copy link
Member

sipa commented Dec 12, 2022

FWIW, I didn't mean to suggest to close this PR. I meant that I don't think fuzzing against a Python implementation adds anything compared to what this PR was aiming to do.

@stratospher
Copy link
Contributor

thanks for clarifying! I find the reference and current implementation to be similar for a good differential fuzz test now. I'd like to understand more about how differential fuzzing is done in other projects before continuing with this PR.

@sipa sipa mentioned this pull request Sep 8, 2023
Closed
43 tasks
@bitcoin bitcoin locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

Assignees
No one assigned
Labels
Tests
Projects
No open projects
P2P encryption
Status: Needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@prakash1512 @DrahtBot @practicalswift @laanwj @achow101 @stratospher @sipa @fanquake