From 7d1cd932342e74421ae927800eeada14f504b944 Mon Sep 17 00:00:00 2001
From: Pieter Wuille <pieter@wuille.net>
Date: Tue, 18 Jul 2023 13:52:52 -0400
Subject: [PATCH] crypto: require key on ChaCha20 initialization

---
 src/crypto/chacha20.cpp                     |  5 -----
 src/crypto/chacha20.h                       |  6 ++++--
 src/random.cpp                              | 18 ++++++++----------
 src/test/fuzz/crypto_chacha20.cpp           |  8 +++-----
 src/test/fuzz/crypto_diff_fuzz_chacha20.cpp | 13 +++----------
 5 files changed, 18 insertions(+), 32 deletions(-)

diff --git a/src/crypto/chacha20.cpp b/src/crypto/chacha20.cpp
index 438e451c9a4..143179f905a 100644
--- a/src/crypto/chacha20.cpp
+++ b/src/crypto/chacha20.cpp
@@ -40,11 +40,6 @@ void ChaCha20Aligned::SetKey(Span<const std::byte> key) noexcept
     input[11] = 0;
 }
 
-ChaCha20Aligned::ChaCha20Aligned() noexcept
-{
-    memset(input, 0, sizeof(input));
-}
-
 ChaCha20Aligned::~ChaCha20Aligned()
 {
     memory_cleanse(input, sizeof(input));
diff --git a/src/crypto/chacha20.h b/src/crypto/chacha20.h
index a766f7d13c6..4d169616097 100644
--- a/src/crypto/chacha20.h
+++ b/src/crypto/chacha20.h
@@ -34,7 +34,8 @@ public:
     /** Block size (inputs/outputs to Keystream / Crypt should be multiples of this). */
     static constexpr unsigned BLOCKLEN{64};
 
-    ChaCha20Aligned() noexcept;
+    /** For safety, disallow initialization without key. */
+    ChaCha20Aligned() noexcept = delete;
 
     /** Initialize a cipher with specified 32-byte key. */
     ChaCha20Aligned(Span<const std::byte> key) noexcept;
@@ -84,7 +85,8 @@ public:
     /** Expected key length in constructor and SetKey. */
     static constexpr unsigned KEYLEN = ChaCha20Aligned::KEYLEN;
 
-    ChaCha20() noexcept = default;
+    /** For safety, disallow initialization without key. */
+    ChaCha20() noexcept = delete;
 
     /** Initialize a cipher with specified 32-byte key. */
     ChaCha20(Span<const std::byte> key) noexcept : m_aligned(key) {}
diff --git a/src/random.cpp b/src/random.cpp
index 3fdb857b2d0..51b8b3ad9d1 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -6,6 +6,7 @@
 #include <random.h>
 
 #include <compat/cpuid.h>
+#include <crypto/chacha20.h>
 #include <crypto/sha256.h>
 #include <crypto/sha512.h>
 #include <logging.h>
@@ -606,10 +607,7 @@ void FastRandomContext::fillrand(Span<std::byte> output)
     rng.Keystream(output);
 }
 
-FastRandomContext::FastRandomContext(const uint256& seed) noexcept : requires_seed(false), bitbuf_size(0)
-{
-    rng.SetKey(MakeByteSpan(seed));
-}
+FastRandomContext::FastRandomContext(const uint256& seed) noexcept : requires_seed(false), rng(MakeByteSpan(seed)), bitbuf_size(0) {}
 
 bool Random_SanityCheck()
 {
@@ -657,13 +655,13 @@ bool Random_SanityCheck()
     return true;
 }
 
-FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_seed(!fDeterministic), bitbuf_size(0)
+static constexpr std::array<std::byte, ChaCha20::KEYLEN> ZERO_KEY{};
+
+FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_seed(!fDeterministic), rng(ZERO_KEY), bitbuf_size(0)
 {
-    if (!fDeterministic) {
-        return;
-    }
-    static constexpr std::array<std::byte, ChaCha20::KEYLEN> ZERO{};
-    rng.SetKey(ZERO);
+    // Note that despite always initializing with ZERO_KEY, requires_seed is set to true if not
+    // fDeterministic. That means the rng will be reinitialized with a secure random key upon first
+    // use.
 }
 
 FastRandomContext& FastRandomContext::operator=(FastRandomContext&& from) noexcept
diff --git a/src/test/fuzz/crypto_chacha20.cpp b/src/test/fuzz/crypto_chacha20.cpp
index 160eebbe8ae..afe93249491 100644
--- a/src/test/fuzz/crypto_chacha20.cpp
+++ b/src/test/fuzz/crypto_chacha20.cpp
@@ -17,11 +17,9 @@ FUZZ_TARGET(crypto_chacha20)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 
-    ChaCha20 chacha20;
-    if (fuzzed_data_provider.ConsumeBool()) {
-        const std::vector<unsigned char> key = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32);
-        chacha20 = ChaCha20{MakeByteSpan(key)};
-    }
+    const std::vector<unsigned char> key = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32);
+    ChaCha20 chacha20{MakeByteSpan(key)};
+
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
         CallOneOf(
             fuzzed_data_provider,
diff --git a/src/test/fuzz/crypto_diff_fuzz_chacha20.cpp b/src/test/fuzz/crypto_diff_fuzz_chacha20.cpp
index f368629fbb7..ab7fa513ec5 100644
--- a/src/test/fuzz/crypto_diff_fuzz_chacha20.cpp
+++ b/src/test/fuzz/crypto_diff_fuzz_chacha20.cpp
@@ -267,20 +267,13 @@ void ECRYPT_keystream_bytes(ECRYPT_ctx* x, u8* stream, u32 bytes)
 
 FUZZ_TARGET(crypto_diff_fuzz_chacha20)
 {
-    static const unsigned char ZEROKEY[32] = {0};
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 
-    ChaCha20 chacha20;
     ECRYPT_ctx ctx;
 
-    if (fuzzed_data_provider.ConsumeBool()) {
-        const std::vector<unsigned char> key = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32);
-        chacha20 = ChaCha20{MakeByteSpan(key)};
-        ECRYPT_keysetup(&ctx, key.data(), key.size() * 8, 0);
-    } else {
-        // The default ChaCha20 constructor is equivalent to using the all-0 key.
-        ECRYPT_keysetup(&ctx, ZEROKEY, 256, 0);
-    }
+    const std::vector<unsigned char> key = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32);
+    ChaCha20 chacha20{MakeByteSpan(key)};
+    ECRYPT_keysetup(&ctx, key.data(), key.size() * 8, 0);
 
     // ECRYPT_keysetup() doesn't set the counter and nonce to 0 while SetKey() does
     static const uint8_t iv[8] = {0, 0, 0, 0, 0, 0, 0, 0};