From d22d5d925c000bf25ad2410ca66c4c21eea75004 Mon Sep 17 00:00:00 2001
From: stratospher <44024636+stratospher@users.noreply.github.com>
Date: Sun, 13 Aug 2023 11:55:46 +0530
Subject: [PATCH] crypto: BIP324 ciphersuite follow-up

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
---
 src/bip324.h                    | 1 +
 src/crypto/chacha20poly1305.cpp | 2 +-
 src/test/bip324_tests.cpp       | 4 ++--
 src/test/crypto_tests.cpp       | 3 +--
 src/test/fuzz/bip324.cpp        | 4 ++--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/bip324.h b/src/bip324.h
index 8d025c2ee37..ecab299af5e 100644
--- a/src/bip324.h
+++ b/src/bip324.h
@@ -54,6 +54,7 @@ public:
 
     /** 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.
      */
diff --git a/src/crypto/chacha20poly1305.cpp b/src/crypto/chacha20poly1305.cpp
index c936dd22659..2636ebe2b2a 100644
--- a/src/crypto/chacha20poly1305.cpp
+++ b/src/crypto/chacha20poly1305.cpp
@@ -95,7 +95,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());
diff --git a/src/test/bip324_tests.cpp b/src/test/bip324_tests.cpp
index ccb9e59e589..0bc22496e3f 100644
--- a/src/test/bip324_tests.cpp
+++ b/src/test/bip324_tests.cpp
@@ -131,10 +131,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);
 
diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp
index 6663c914a9a..6fbe74a680a 100644
--- a/src/test/crypto_tests.cpp
+++ b/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));
         }
 
diff --git a/src/test/fuzz/bip324.cpp b/src/test/fuzz/bip324.cpp
index 359de6c66a3..8282261c52a 100644
--- a/src/test/fuzz/bip324.cpp
+++ b/src/test/fuzz/bip324.cpp
@@ -75,13 +75,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);