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

1. 
   prakash1512 commented at 9:39 am on October 20, 2021: 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:

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

2. fanquake added the label Tests on Oct 20, 2021
3. in src/test/fuzz/crypto_diff_fuzz_poly1305.cpp:22 in c336310719 outdated

   17+static INLINE void fU32TO8_LE_FAST(uint8_t* p, const uint32_t v) { *(uint32_t*)p = v; }
   18+
   19+#define U8TO32_LE(p) fU8TO32_LE_FAST(p)
   20+#define U32TO8_LE(p, v) fU32TO8_LE_FAST(p, v)
   21+
   22+void poly1305_auth(unsigned char out[16], const unsigned char* m, size_t inlen, const unsigned char key[32])
   

   ---

   ---

   

   laanwj commented at 1:31 pm on October 20, 2021:

   If you include third party code, can you please add a comment that mentions

   • Where this code comes from (https://github.com/floodyberry/poly1305-donna I think?)
   • Author name
   • Which license it is under

   ---

   

   prakash1512 commented at 8:35 pm on October 20, 2021:

   Thanks for the review I have made the necessary changes, and PR is ready for further review.

   ---

   

   laanwj commented at 5:10 pm on October 21, 2021:

   Thanks!
4. prakash1512 force-pushed on Oct 20, 2021
5. prakash1512 force-pushed on Oct 20, 2021
6. prakash1512 force-pushed on Oct 20, 2021
7. 
   DrahtBot commented at 5:44 am on October 21, 2021: contributor

   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.

8. 
   practicalswift commented at 8:05 am on October 21, 2021: contributor

   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.
9. 
   laanwj commented at 5:15 pm on October 21, 2021: member

   Code review ACK d2af151e3cb638110087d905a31c205f4d81f7bc
10. DrahtBot added the label Needs rebase on Dec 17, 2021
11. Add Floodyberry's poly1305 implementation

    Co-authored-by: Ruhi <44024636+stratospher@users.noreply.github.com>

    5ac6d8f2ff
12. Change in reference implementation

    Co-authored-by: Ruhi <44024636+stratospher@users.noreply.github.com>

    3f00cdb4a3
13. Add fuzzing harness and Makefile

    Co-authored-by: Ruhi <44024636+stratospher@users.noreply.github.com>

    a208e06da4
14. stratospher force-pushed on Apr 2, 2022
15. DrahtBot removed the label Needs rebase on Apr 2, 2022
16. 
    achow101 commented at 6:43 pm on October 12, 2022: member

    Are you still working on this?
17. 
    stratospher commented at 7:31 pm on October 17, 2022: contributor

    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.

18. 
    sipa commented at 8:09 pm on October 17, 2022: member

    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.
19. 
    stratospher commented at 8:50 am on December 12, 2022: 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.

20. prakash1512 closed this on Dec 12, 2022

    ---

21. 
    sipa commented at 1:58 pm on December 12, 2022: member

    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.
22. 
    stratospher commented at 7:13 pm on December 12, 2022: 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.
23. bitcoin locked this on Dec 12, 2023

Contributors
prakash1512 laanwj DrahtBot practicalswift achow101 stratospher sipa

Labels
Tests