Merge bitcoin/bitcoin#28267: crypto: BIP324 ciphersuite follow-up

93cb8f0 refactor: add missing headers for BIP324 ciphersuite (stratospher)
d22d5d9 crypto: BIP324 ciphersuite follow-up (stratospher)

Pull request description:

  follow-up to #28008.
  * move `dummy_tag` variable in FSChaCha20Poly1305 crypto_tests outside of the loop to be reused every time
  * use easy to read `cipher.last()` in `AEADChaCha20Poly1305::Decrypt()`
  * comment for initiator in `BIP324Cipher::Initialize()`
  * systematically damage ciphertext with bit positions in bip324_tests
  * use 4095 max bytes for `aad` in bip324 fuzz test

ACKs for top commit:
  fanquake:
    ACK 93cb8f0 - thanks for following up here.

Tree-SHA512: 361f3e226d3168fdef69a2eebe6092cfc04ba14ce009420222e762698001eaf8be69a1138dab0be237964509c2b96a41a0b4db5c1df43ef75062f143c5aa741a

Author: fanquake

src/bip324.cpp

@@ -8,14 +8,19 @@
 #include <crypto/chacha20.h>
 #include <crypto/chacha20poly1305.h>
 #include <crypto/hkdf_sha256_32.h>
+#include <key.h>
+#include <pubkey.h>
 #include <random.h>
 #include <span.h>
 #include <support/cleanse.h>
+#include <uint256.h>
 
 #include <algorithm>
 #include <assert.h>
 #include <cstdint>
 #include <cstddef>
+#include <iterator>
+#include <string>
 
 BIP324Cipher::BIP324Cipher() noexcept
 {

src/bip324.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_BIP324_H
 #define BITCOIN_BIP324_H
 
+#include <array>
 #include <cstddef>
 #include <optional>
 
@@ -54,6 +55,7 @@ class BIP324Cipher
 
     /** Initialize when the other side's public key is received. Can only be called once.
      *
+     * initiator is set to true if we are the initiator establishing the v2 P2P connection.
      * self_decrypt is only for testing, and swaps encryption/decryption keys, so that encryption
      * and decryption can be tested without knowing the other side's private key.
      */

src/crypto/chacha20poly1305.cpp

@@ -11,9 +11,7 @@
 #include <support/cleanse.h>
 
 #include <assert.h>
-#include <cstdint>
 #include <cstddef>
-#include <iterator>
 
 AEADChaCha20Poly1305::AEADChaCha20Poly1305(Span<const std::byte> key) noexcept : m_chacha20(UCharCast(key.data()))
 {
@@ -95,7 +93,7 @@ bool AEADChaCha20Poly1305::Decrypt(Span<const std::byte> cipher, Span<const std:
     m_chacha20.Seek64(nonce, 0);
     std::byte expected_tag[EXPANSION];
     ComputeTag(m_chacha20, aad, cipher.first(cipher.size() - EXPANSION), expected_tag);
-    if (timingsafe_bcmp(UCharCast(expected_tag), UCharCast(cipher.data() + cipher.size() - EXPANSION), EXPANSION)) return false;
+    if (timingsafe_bcmp(UCharCast(expected_tag), UCharCast(cipher.last(EXPANSION).data()), EXPANSION)) return false;
 
     // Decrypt (starting at block 1).
     m_chacha20.Crypt(UCharCast(cipher.data()), UCharCast(plain1.data()), plain1.size());

src/crypto/chacha20poly1305.h

@@ -6,7 +6,6 @@
 #define BITCOIN_CRYPTO_CHACHA20POLY1305_H
 
 #include <cstddef>
-#include <cstdlib>
 #include <stdint.h>
 
 #include <crypto/chacha20.h>

src/test/bip324_tests.cpp

@@ -6,13 +6,15 @@
 #include <chainparams.h>
 #include <key.h>
 #include <pubkey.h>
+#include <span.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
 #include <util/strencodings.h>
 
 #include <array>
-#include <vector>
 #include <cstddef>
+#include <cstdint>
+#include <vector>
 
 #include <boost/test/unit_test.hpp>
 
@@ -131,10 +133,10 @@ void TestBIP324PacketVector(
         // Decrypt length
         auto to_decrypt = ciphertext;
         if (error >= 2 && error <= 9) {
-            to_decrypt[InsecureRandRange(to_decrypt.size())] ^= std::byte(1U << InsecureRandRange(8));
+            to_decrypt[InsecureRandRange(to_decrypt.size())] ^= std::byte(1U << (error - 2));
         }
 
-        // Decrypt length and resize ciphertext to accomodate.
+        // Decrypt length and resize ciphertext to accommodate.
         uint32_t dec_len = dec_cipher.DecryptLength(MakeByteSpan(to_decrypt).first(cipher.LENGTH_LEN));
         to_decrypt.resize(dec_len + cipher.EXPANSION);
 

src/test/crypto_tests.cpp

@@ -300,11 +300,11 @@ static void TestFSChaCha20Poly1305(const std::string& plain_hex, const std::stri
     for (int it = 0; it < 10; ++it) {
         // During it==0 we use the single-plain Encrypt/Decrypt; others use a split at prefix.
         size_t prefix = it ? InsecureRandRange(plain.size() + 1) : plain.size();
+        std::byte dummy_tag[FSChaCha20Poly1305::EXPANSION] = {{}};
 
         // Do msg_idx dummy encryptions to seek to the correct packet.
         FSChaCha20Poly1305 enc_aead{key, 224};
         for (uint64_t i = 0; i < msg_idx; ++i) {
-            std::byte dummy_tag[FSChaCha20Poly1305::EXPANSION] = {{}};
             enc_aead.Encrypt(Span{dummy_tag}.first(0), Span{dummy_tag}.first(0), dummy_tag);
         }
 
@@ -319,7 +319,6 @@ static void TestFSChaCha20Poly1305(const std::string& plain_hex, const std::stri
         // Do msg_idx dummy decryptions to seek to the correct packet.
         FSChaCha20Poly1305 dec_aead{key, 224};
         for (uint64_t i = 0; i < msg_idx; ++i) {
-            std::byte dummy_tag[FSChaCha20Poly1305::EXPANSION] = {{}};
             dec_aead.Decrypt(dummy_tag, Span{dummy_tag}.first(0), Span{dummy_tag}.first(0));
         }
 

src/test/fuzz/bip324.cpp

@@ -11,7 +11,6 @@
 #include <test/util/xoroshiro128plusplus.h>
 
 #include <cstdint>
-#include <tuple>
 #include <vector>
 
 namespace {
@@ -75,13 +74,13 @@ FUZZ_TARGET(bip324_cipher_roundtrip, .init=Initialize)
         // - Bit 0: whether the ignore bit is set in message
         // - Bit 1: whether the responder (0) or initiator (1) sends
         // - Bit 2: whether this ciphertext will be corrupted (making it the last sent one)
-        // - Bit 3-4: controls the maximum aad length (max 511 bytes)
+        // - Bit 3-4: controls the maximum aad length (max 4095 bytes)
         // - Bit 5-7: controls the maximum content length (max 16383 bytes, for performance reasons)
         unsigned mode = provider.ConsumeIntegral<uint8_t>();
         bool ignore = mode & 1;
         bool from_init = mode & 2;
         bool damage = mode & 4;
-        unsigned aad_length_bits = 3 * ((mode >> 3) & 3);
+        unsigned aad_length_bits = 4 * ((mode >> 3) & 3);
         unsigned aad_length = provider.ConsumeIntegralInRange<unsigned>(0, (1 << aad_length_bits) - 1);
         unsigned length_bits = 2 * ((mode >> 5) & 7);
         unsigned length = provider.ConsumeIntegralInRange<unsigned>(0, (1 << length_bits) - 1);