Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

stratospher · 2023-08-13T07:00:24Z

This PR adds fuzz tests for AEADChaCha20Poly1305 and FSChaCha20Poly1305 introduced in #28008.

Run using:

$ FUZZ=crypto_aeadchacha20poly1305 src/test/fuzz/fuzz
$ FUZZ=crypto_fschacha20poly1305 src/test/fuzz/fuzz

DrahtBot · 2023-08-13T07:00:27Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, marcofleon
Concept ACK	theStack

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.

sipa

Mostly comments on the fuzz tests. The last commit looks good to me (and maybe we want to split it off).

sipa · 2023-08-13T22:36:41Z

src/test/fuzz/crypto_chacha20poly1305.cpp

+
+    auto key = provider.ConsumeBytes<std::byte>(32);
+    key.resize(32);
+    auto aad = provider.ConsumeBytes<std::byte>(512);


This will always construct an AAD of 512 bytes, if that much data is available in the fuzzer input. I think it's better to make the AAD size dynamic, and a function of the fuzzer input data too.

true! it's done like in bip324_cipher_roundtrip fuzz test now.

sipa · 2023-08-13T22:56:35Z

src/test/fuzz/crypto_chacha20poly1305.cpp

+            },
+            [&] {
+                bool ok = aead.Decrypt(cipher, aad, nonce, contents); // with splits too
+                if (ok) {


This will never trigger, as the AEAD advances between between the preceding Encrypt call and the Decrypt call here, so encryption and decryption will never use the same key. With distinct keys, it should be cryptographically impossible for anyone (let alone a fuzzer) to get a match (and if, with extremely low probability, ok=true is returned, it'll be accidental, and plaintext and decrypted result won't match).

There are two possible styles of fuzz tests we could write here, and this feels like a mix between the two:

A "exercise-the-API" fuzz test, which is only intended to make sure no crashes happen under arbitrary sequences of method calls. I personally think such tests are of rather low value, though of course better than nothing, and we do have some in the codebase (including crypto_chacha20 and crypto_fschacha20). If that's the goal here, you can probably just ignore the return value of Decrypt.

A "test encryption and decryption match" fuzz test (possibly combined with a "if errors are introduced between encryption and decryption, the latter fails"). Such tests are a lot more interesting and they test some notion of correctness of behavior in addition to just not crashing. If you want that here, you'll need some approach that involves two AEADChaCha20Poly1305 objects (one for encrypting, one for decrypting), initialized with matching (or deliberately mismatching) keys, and then running through some scenario that's applied to both to keep them in sync. The bip324_cipher_roundtrip fuzz test is of this style (but at a higher level).

thanks for the interesting explanation! updated it to a "test encryption and decryption match" kind of fuzz test.

sipa · 2023-08-13T22:59:38Z

src/test/fuzz/crypto_chacha20poly1305.cpp

+            [&] {
+                bool ok = aead.Decrypt(cipher, aad, contents);
+                if (ok) {
+                    assert(plain == contents);


The same comment as I've given on the other test about mixing "exercise API" and "test correctness" fuzzing applies here: ok will never be true (except with negligible probability), and if it does, plain and contents won't match.

makes sense. updated it to a "test encryption and decryption match" kind of fuzz test.

sipa · 2023-08-13T23:02:02Z

src/bip324.h

@@ -54,6 +54,7 @@ class BIP324Cipher

    /** Initialize when the other side's public key is received. Can only be called once.
     *
+     * initiator is set to true if the BIP324 cipher involves initiating the v2 P2P connection


Nit: . at the end of the sentence so it's clear the lines are about different arguments.

Also perhaps say "if we are the initiator" or so; every v2 P2P connection involves initiating somehow; the question is whether it's us or the remote side doing that.

done in #28267

sipa · 2023-08-13T23:12:24Z

@stratospher Want to also address #28008 (comment) and #28008 (comment) here?

stratospher · 2023-08-14T03:55:49Z

so split off the last commit into #28267 since they aren't related to the fuzz test. (included #28008 (comment) and #28008 (comment) there)

thinking about #28263 (comment). will address it soon.

maflcko · 2023-10-20T09:23:16Z

Are you still working on this?

stratospher · 2023-10-24T16:45:42Z

Are you still working on this?

yes! updated the PR to address #28263 (comment).

theStack

Concept ACK

src/test/fuzz/crypto_chacha20poly1305.cpp

e67634e fuzz: BIP324: damage ciphertext/aad in full byte range (Sebastian Falbesoner) Pull request description: This PR is a tiny improvement for the `bip324_cipher_roundtrip` fuzz target: currently the damaging of input data for decryption (either ciphertext or aad) only ever happens in the lower nibble within the byte at the damage position, as the bit position for the `damage_val` byte was calculated with `damage_bit & 3` (corresponding to `% 4`) rather than `damage_bit & 7` (corresponding to the expected `% 8`). Noticed while reviewing #28263 which uses similar constructs. ACKs for top commit: stratospher: ACK e67634e. dergoegge: utACK e67634e Tree-SHA512: 1bab4df28708e079874feee939beef45eff235215375c339decc696f4c9aef04e4b417322b045491c8aec6e88ec8ec2db564e27ef1b0be352b6ff4ed38bad49a

real-or-random · 2024-04-10T08:21:24Z

@theStack want to re-review this? :)

DrahtBot · 2024-04-17T18:15:16Z

Could rebase for fresh CI?

DrahtBot · 2024-07-04T20:20:28Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23942811284}

dergoegge

tACK 3980563

src/test/fuzz/crypto_chacha20poly1305.cpp

dergoegge

tACK 8607773

marcofleon

Tested ACK 8607773. Ran both targets for ~200 CPU hours. Coverage of intended targets looks good to me. The simulation of damaged keys and checks that follow seem useful as well.

maflcko · 2024-07-23T10:35:46Z

Timeout in #30505

hebasto · 2024-07-24T12:45:05Z

Ported to the CMake-based build system in hebasto#280.

stratospher mentioned this pull request Aug 13, 2023

BIP324 ciphersuite #28008

Merged

sipa reviewed Aug 13, 2023

View reviewed changes

stratospher force-pushed the fuzz_chacha20poly1305 branch from d2ee2f3 to 9ac4022 Compare August 14, 2023 03:52

sipa mentioned this pull request Sep 3, 2023

BIP324 tracking issue #27634

Closed

43 tasks

DrahtBot added CI failed and removed CI failed labels Sep 4, 2023

DrahtBot added CI failed and removed CI failed labels Sep 20, 2023

stratospher force-pushed the fuzz_chacha20poly1305 branch 2 times, most recently from 2e48c8b to 9494695 Compare October 24, 2023 08:16

DrahtBot added the CI failed label Oct 24, 2023

DrahtBot removed the CI failed label Oct 25, 2023

theStack mentioned this pull request Nov 28, 2023

fuzz: BIP324: damage ciphertext/aad in full byte range #28951

Merged

theStack reviewed Nov 28, 2023

View reviewed changes

src/test/fuzz/crypto_chacha20poly1305.cpp Outdated Show resolved Hide resolved

src/test/fuzz/crypto_chacha20poly1305.cpp Outdated Show resolved Hide resolved

stratospher force-pushed the fuzz_chacha20poly1305 branch from 9494695 to 20f769a Compare November 28, 2023 08:03

DrahtBot added the CI failed label Jan 12, 2024

achow101 requested review from dergoegge and real-or-random April 9, 2024 14:57

stratospher force-pushed the fuzz_chacha20poly1305 branch from 20f769a to 5a25e70 Compare April 17, 2024 18:19

DrahtBot removed the CI failed label Apr 17, 2024

DrahtBot added the CI failed label Jul 4, 2024

stratospher force-pushed the fuzz_chacha20poly1305 branch 2 times, most recently from 2dd6cf4 to 3980563 Compare July 6, 2024 07:01

DrahtBot removed the CI failed label Jul 6, 2024

dergoegge approved these changes Jul 15, 2024

View reviewed changes

src/test/fuzz/crypto_chacha20poly1305.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from theStack July 15, 2024 10:12

stratospher added 2 commits July 15, 2024 18:25

Add fuzz test for AEADChacha20Poly1305

c807f33

Add fuzz test for FSChaCha20Poly1305

8607773

stratospher force-pushed the fuzz_chacha20poly1305 branch from 3980563 to 8607773 Compare July 15, 2024 13:15

dergoegge approved these changes Jul 15, 2024

View reviewed changes

fanquake requested a review from marcofleon July 15, 2024 16:57

marcofleon approved these changes Jul 16, 2024

View reviewed changes

fanquake merged commit 1db0be8 into bitcoin:master Jul 16, 2024
16 checks passed

hebasto added the Needs CMake port label Jul 16, 2024

hebasto mentioned this pull request Jul 16, 2024

build: Introduce CMake-based build system #30454

Merged

stratospher deleted the fuzz_chacha20poly1305 branch July 17, 2024 06:40

hebasto mentioned this pull request Jul 24, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#280

Closed

hebasto removed the Needs CMake port label Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

stratospher commented Aug 13, 2023 •

edited

Loading

DrahtBot commented Aug 13, 2023 •

edited

Loading

sipa left a comment

sipa Aug 13, 2023

stratospher Oct 24, 2023

sipa Aug 13, 2023

stratospher Oct 24, 2023

sipa Aug 13, 2023

stratospher Oct 24, 2023

sipa Aug 13, 2023 •

edited

Loading

stratospher Oct 24, 2023

sipa commented Aug 13, 2023

stratospher commented Aug 14, 2023

maflcko commented Oct 20, 2023

stratospher commented Oct 24, 2023

theStack left a comment

real-or-random commented Apr 10, 2024

DrahtBot commented Apr 17, 2024

DrahtBot commented Jul 4, 2024

dergoegge left a comment

dergoegge left a comment

marcofleon left a comment

maflcko commented Jul 23, 2024

hebasto commented Jul 24, 2024

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

Conversation

stratospher commented Aug 13, 2023 • edited Loading

DrahtBot commented Aug 13, 2023 • edited Loading

Code Coverage

Reviews

Conflicts

sipa left a comment

Choose a reason for hiding this comment

sipa Aug 13, 2023

Choose a reason for hiding this comment

stratospher Oct 24, 2023

Choose a reason for hiding this comment

sipa Aug 13, 2023

Choose a reason for hiding this comment

stratospher Oct 24, 2023

Choose a reason for hiding this comment

sipa Aug 13, 2023

Choose a reason for hiding this comment

stratospher Oct 24, 2023

Choose a reason for hiding this comment

sipa Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

stratospher Oct 24, 2023

Choose a reason for hiding this comment

sipa commented Aug 13, 2023

stratospher commented Aug 14, 2023

maflcko commented Oct 20, 2023

stratospher commented Oct 24, 2023

theStack left a comment

Choose a reason for hiding this comment

real-or-random commented Apr 10, 2024

DrahtBot commented Apr 17, 2024

DrahtBot commented Jul 4, 2024

dergoegge left a comment

Choose a reason for hiding this comment

dergoegge left a comment

Choose a reason for hiding this comment

marcofleon left a comment

Choose a reason for hiding this comment

maflcko commented Jul 23, 2024

hebasto commented Jul 24, 2024

stratospher commented Aug 13, 2023 •

edited

Loading

DrahtBot commented Aug 13, 2023 •

edited

Loading

sipa Aug 13, 2023 •

edited

Loading