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
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?

Add fuzz test for AEADChacha20Poly1305 aa8f5cb011

Add fuzz test for FSChaCha20Poly1305 5a25e70636

stratospher force-pushed on Apr 17, 2024

DrahtBot removed the label CI failed on Apr 17, 2024

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

Code Coverage

Reviews

Conflicts