Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305

stratospher commented at 7:00 am on August 13, 2023: contributor

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

Run using:

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

DrahtBot commented at 7:00 am on August 13, 2023: contributor

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.

in src/test/fuzz/crypto_chacha20poly1305.cpp:21 in a4ce5fd363 outdated

16+{
17+    FuzzedDataProvider provider{buffer.data(), buffer.size()};
18+
19+    auto key = provider.ConsumeBytes<std::byte>(32);
20+    key.resize(32);
21+    auto aad = provider.ConsumeBytes<std::byte>(512);

sipa commented at 10:36 pm on August 13, 2023:

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.

stratospher commented at 4:45 pm on October 24, 2023:

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

in src/test/fuzz/crypto_chacha20poly1305.cpp:60 in a4ce5fd363 outdated

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

sipa commented at 10:56 pm on August 13, 2023:

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).

stratospher commented at 4:45 pm on October 24, 2023:

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

in src/test/fuzz/crypto_chacha20poly1305.cpp:116 in 9ac40226ae outdated

111+                }
112+            },
113+            [&] {
114+                bool ok = aead.Decrypt(cipher, aad, contents);
115+                if (ok) {
116+                    assert(plain == contents);

sipa commented at 10:59 pm on August 13, 2023:

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.

stratospher commented at 4:45 pm on October 24, 2023:

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

in src/bip324.h:57 in d2ee2f39fe outdated

53@@ -54,6 +54,7 @@ class BIP324Cipher
54 
55     /** Initialize when the other side's public key is received. Can only be called once.
56      *
57+     * initiator is set to true if the BIP324 cipher involves initiating the v2 P2P connection

sipa commented at 11:02 pm on August 13, 2023:

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.

stratospher commented at 4:45 pm on October 24, 2023:

done in https://github.com/bitcoin/bitcoin/pull/28267

sipa commented at 11:03 pm on August 13, 2023: member

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

sipa commented at 11:12 pm on August 13, 2023: member

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

stratospher force-pushed on Aug 14, 2023

stratospher commented at 3:55 am on August 14, 2023: contributor

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

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

DrahtBot added the label CI failed on Sep 4, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

DrahtBot added the label CI failed on Sep 20, 2023

DrahtBot removed the label CI failed on Sep 22, 2023

maflcko commented at 9:23 am on October 20, 2023: member

Are you still working on this?

stratospher force-pushed on Oct 24, 2023

DrahtBot added the label CI failed on Oct 24, 2023

stratospher commented at 4:45 pm on October 24, 2023: contributor

Are you still working on this?

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

DrahtBot removed the label CI failed on Oct 25, 2023

in src/test/fuzz/crypto_chacha20poly1305.cpp:62 in 48818c75e5 outdated

57+            aead.Encrypt(plain, aad, nonce, cipher);
58+        }
59+
60+        // Test Keystream output
61+        std::vector<std::byte> keystream(length);
62+        AEADChaCha20Poly1305 aead_for_ks(key);

theStack commented at 0:58 am on November 28, 2023:

nit: aead_for_ks is unused (can be just removed AFAICT, as it seems fine to reuse the existing aead object).

stratospher commented at 8:05 am on November 28, 2023:

thanks! done.

in src/test/fuzz/crypto_chacha20poly1305.cpp:86 in 48818c75e5 outdated

81+        // Optionally damage 1 bit in either the cipher (corresponding to a change in transit)
82+        // or the aad (to make sure that decryption will fail if the AAD mismatches).
83+        if (damage) {
84+            unsigned damage_bit = provider.ConsumeIntegralInRange<unsigned>(0, (cipher.size() + aad.size()) * 8U - 1U);
85+            unsigned damage_pos = damage_bit >> 3;
86+            std::byte damage_val{(uint8_t)(1U << (damage_bit & 3))};

theStack commented at 1:41 am on November 28, 2023:

(here and for the other fuzz test:)

0            std::byte damage_val{(uint8_t)(1U << (damage_bit & 7))};

As otherwise only the lower nibble within the byte is damaged (see also #28951).

stratospher commented at 8:05 am on November 28, 2023:

true! done.

theStack commented at 1:43 am on November 28, 2023: contributor

Concept ACK

stratospher force-pushed on Nov 28, 2023

fanquake referenced this in commit 05d3f8e822 on Nov 30, 2023

DrahtBot added the label CI failed on Jan 12, 2024

achow101 requested review from dergoegge on Apr 9, 2024

achow101 requested review from real-or-random on Apr 9, 2024

real-or-random commented at 8:21 am on April 10, 2024: contributor

@theStack want to re-review this? :)

DrahtBot commented at 6:15 pm on April 17, 2024: contributor

Could rebase for fresh CI?

stratospher force-pushed on Apr 17, 2024

DrahtBot removed the label CI failed on Apr 17, 2024

DrahtBot added the label CI failed on Jul 4, 2024

DrahtBot commented at 8:20 pm on July 4, 2024: contributor

🚧 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

stratospher force-pushed on Jul 5, 2024

stratospher force-pushed on Jul 6, 2024

DrahtBot removed the label CI failed on Jul 6, 2024

in src/test/fuzz/crypto_chacha20poly1305.cpp:58 in 3980563866 outdated

53+        unsigned aad_length = provider.ConsumeIntegralInRange<unsigned>(0, (1 << aad_length_bits) - 1);
54+        unsigned length_bits = 2 * ((mode >> 5) & 7);
55+        unsigned length = provider.ConsumeIntegralInRange<unsigned>(0, (1 << length_bits) - 1);
56+        // Generate aad and content.
57+        std::vector<std::byte> aad(aad_length);
58+        for (auto& val : aad) val = std::byte{(uint8_t)rng()};

dergoegge commented at 10:11 am on July 15, 2024:

nit:

0        auto aad{rng.randbytes<std::byte>(aad_length)};

stratospher commented at 1:16 pm on July 15, 2024:

done.

dergoegge approved

dergoegge commented at 10:12 am on July 15, 2024: member

tACK 39805638663bdd567a50ecac962631b45ce8d73a

DrahtBot requested review from theStack on Jul 15, 2024

Add fuzz test for AEADChacha20Poly1305 c807f33228

Add fuzz test for FSChaCha20Poly1305 8607773750

stratospher force-pushed on Jul 15, 2024

dergoegge approved

dergoegge commented at 4:43 pm on July 15, 2024: member

tACK 8607773750e60931e51a33e48cd077a1dedf9db3

fanquake requested review from marcofleon on Jul 15, 2024

marcofleon approved

marcofleon commented at 10:50 am on July 16, 2024: contributor

Tested ACK 8607773750e60931e51a33e48cd077a1dedf9db3. 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.

fanquake merged this on Jul 16, 2024

fanquake closed this on Jul 16, 2024

hebasto added the label Needs CMake port on Jul 16, 2024

stratospher deleted the branch on Jul 17, 2024

maflcko commented at 10:35 am on July 23, 2024: member

Timeout in https://github.com/bitcoin/bitcoin/issues/30505

hebasto commented at 12:45 pm on July 24, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/280.

hebasto removed the label Needs CMake port on Jul 24, 2024

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

Code Coverage

Reviews

Conflicts