Refactor Stateful_RNG and add a lock

This differs from our general practice of requiring external locks if
multiple threads wish to use the same object, but reduces risk wrt
incorrect usage causing catastrophic failure such as duplicating RNG
outputs.

See also #2397

8 files changed, 185 insertions, 142 deletions

diff --git a/src/lib/rng/chacha_rng/chacha_rng.cpp b/src/lib/rng/chacha_rng/chacha_rng.cpp
index 065a19060..3dc69ec1b 100644
--- a/src/lib/rng/chacha_rng/chacha_rng.cpp
+++ b/src/lib/rng/chacha_rng/chacha_rng.cpp
@@ -52,24 +52,15 @@ ChaCha_RNG::ChaCha_RNG(Entropy_Sources& entropy_sources,
    clear();
    }
 
-void ChaCha_RNG::clear()
+void ChaCha_RNG::clear_state()
    {
-   Stateful_RNG::clear();
-
    m_hmac->set_key(std::vector<uint8_t>(m_hmac->output_length(), 0x00));
    m_chacha->set_key(m_hmac->final());
    }
 
-void ChaCha_RNG::randomize(uint8_t output[], size_t output_len)
-   {
-   randomize_with_input(output, output_len, nullptr, 0);
-   }
-
-void ChaCha_RNG::randomize_with_input(uint8_t output[], size_t output_len,
-                                      const uint8_t input[], size_t input_len)
+void ChaCha_RNG::generate_output(uint8_t output[], size_t output_len,
+                                 const uint8_t input[], size_t input_len)
    {
-   reseed_check();
-
    if(input_len > 0)
       {
       update(input, input_len);
@@ -88,16 +79,6 @@ void ChaCha_RNG::update(const uint8_t input[], size_t input_len)
    m_hmac->set_key(mac_key);
    }
 
-void ChaCha_RNG::add_entropy(const uint8_t input[], size_t input_len)
-   {
-   update(input, input_len);
-
-   if(8*input_len >= security_level())
-      {
-      reset_reseed_counter();
-      }
-   }
-
 size_t ChaCha_RNG::security_level() const
    {
    return 256;
diff --git a/src/lib/rng/chacha_rng/chacha_rng.h b/src/lib/rng/chacha_rng/chacha_rng.h
index a0d7d38f2..c50c2d0c2 100644
--- a/src/lib/rng/chacha_rng/chacha_rng.h
+++ b/src/lib/rng/chacha_rng/chacha_rng.h
@@ -104,21 +104,17 @@ class BOTAN_PUBLIC_API(2,3) ChaCha_RNG final : public Stateful_RNG
 
       std::string name() const override { return "ChaCha_RNG"; }
 
-      void clear() override;
-
-      void randomize(uint8_t output[], size_t output_len) override;
-
-      void randomize_with_input(uint8_t output[], size_t output_len,
-                                const uint8_t input[], size_t input_len) override;
-
-      void add_entropy(const uint8_t input[], size_t input_len) override;
-
       size_t security_level() const override;
 
       size_t max_number_of_bytes_per_request() const override { return 0; }
 
    private:
-      void update(const uint8_t input[], size_t input_len);
+      void update(const uint8_t input[], size_t input_len) override;
+
+      void generate_output(uint8_t output[], size_t output_len,
+                           const uint8_t input[], size_t input_len) override;
+
+      void clear_state() override;
 
       std::unique_ptr<MessageAuthenticationCode> m_hmac;
       std::unique_ptr<StreamCipher> m_chacha;
diff --git a/src/lib/rng/hmac_drbg/hmac_drbg.cpp b/src/lib/rng/hmac_drbg/hmac_drbg.cpp
index d4240f4e0..2b66a839c 100644
--- a/src/lib/rng/hmac_drbg/hmac_drbg.cpp
+++ b/src/lib/rng/hmac_drbg/hmac_drbg.cpp
@@ -12,6 +12,25 @@ namespace Botan {
 
 namespace {
 
+size_t hmac_drbg_security_level(size_t mac_output_length)
+   {
+   // security strength of the hash function
+   // for pre-image resistance (see NIST SP 800-57)
+   // SHA-160: 128 bits
+   // SHA-224, SHA-512/224: 192 bits,
+   // SHA-256, SHA-512/256, SHA-384, SHA-512: >= 256 bits
+   // NIST SP 800-90A only supports up to 256 bits though
+
+   if(mac_output_length < 32)
+      {
+      return (mac_output_length - 4) * 8;
+      }
+   else
+      {
+      return 32 * 8;
+      }
+   }
+
 void check_limits(size_t reseed_interval,
                   size_t max_number_of_bytes_per_request)
    {
@@ -36,7 +55,8 @@ HMAC_DRBG::HMAC_DRBG(std::unique_ptr<MessageAuthenticationCode> prf,
                      size_t max_number_of_bytes_per_request) :
    Stateful_RNG(underlying_rng, reseed_interval),
    m_mac(std::move(prf)),
-   m_max_number_of_bytes_per_request(max_number_of_bytes_per_request)
+   m_max_number_of_bytes_per_request(max_number_of_bytes_per_request),
+   m_security_level(hmac_drbg_security_level(m_mac->output_length()))
    {
    BOTAN_ASSERT_NONNULL(m_mac);
 
@@ -52,7 +72,8 @@ HMAC_DRBG::HMAC_DRBG(std::unique_ptr<MessageAuthenticationCode> prf,
                      size_t max_number_of_bytes_per_request) :
    Stateful_RNG(underlying_rng, entropy_sources, reseed_interval),
    m_mac(std::move(prf)),
-   m_max_number_of_bytes_per_request(max_number_of_bytes_per_request)
+   m_max_number_of_bytes_per_request(max_number_of_bytes_per_request),
+   m_security_level(hmac_drbg_security_level(m_mac->output_length()))
    {
    BOTAN_ASSERT_NONNULL(m_mac);
 
@@ -67,7 +88,8 @@ HMAC_DRBG::HMAC_DRBG(std::unique_ptr<MessageAuthenticationCode> prf,
                      size_t max_number_of_bytes_per_request) :
    Stateful_RNG(entropy_sources, reseed_interval),
    m_mac(std::move(prf)),
-   m_max_number_of_bytes_per_request(max_number_of_bytes_per_request)
+   m_max_number_of_bytes_per_request(max_number_of_bytes_per_request),
+   m_security_level(hmac_drbg_security_level(m_mac->output_length()))
    {
    BOTAN_ASSERT_NONNULL(m_mac);
 
@@ -79,22 +101,33 @@ HMAC_DRBG::HMAC_DRBG(std::unique_ptr<MessageAuthenticationCode> prf,
 HMAC_DRBG::HMAC_DRBG(std::unique_ptr<MessageAuthenticationCode> prf) :
    Stateful_RNG(),
    m_mac(std::move(prf)),
-   m_max_number_of_bytes_per_request(64*1024)
+   m_max_number_of_bytes_per_request(64*1024),
+   m_security_level(hmac_drbg_security_level(m_mac->output_length()))
    {
    BOTAN_ASSERT_NONNULL(m_mac);
    clear();
    }
 
-void HMAC_DRBG::clear()
+HMAC_DRBG::HMAC_DRBG(const std::string& hmac_hash) :
+   Stateful_RNG(),
+   m_mac(MessageAuthenticationCode::create_or_throw("HMAC(" + hmac_hash + ")")),
+   m_max_number_of_bytes_per_request(64 * 1024),
+   m_security_level(hmac_drbg_security_level(m_mac->output_length()))
    {
-   Stateful_RNG::clear();
+   clear();
+   }
 
-   const size_t output_length = m_mac->output_length();
+void HMAC_DRBG::clear_state()
+   {
+   if(m_V.size() == 0)
+      {
+      const size_t output_length = m_mac->output_length();
+      m_V.resize(output_length);
+      }
 
-   m_V.resize(output_length);
    for(size_t i = 0; i != m_V.size(); ++i)
       m_V[i] = 0x01;
-   m_mac->set_key(std::vector<uint8_t>(output_length, 0x00));
+   m_mac->set_key(std::vector<uint8_t>(m_V.size(), 0x00));
    }
 
 std::string HMAC_DRBG::name() const
@@ -102,44 +135,30 @@ std::string HMAC_DRBG::name() const
    return "HMAC_DRBG(" + m_mac->name() + ")";
    }
 
-void HMAC_DRBG::randomize(uint8_t output[], size_t output_len)
-   {
-   randomize_with_input(output, output_len, nullptr, 0);
-   }
-
 /*
 * HMAC_DRBG generation
 * See NIST SP800-90A section 10.1.2.5
 */
-void HMAC_DRBG::randomize_with_input(uint8_t output[], size_t output_len,
-                                     const uint8_t input[], size_t input_len)
+void HMAC_DRBG::generate_output(uint8_t output[], size_t output_len,
+                                const uint8_t input[], size_t input_len)
    {
-   while(output_len > 0)
+   if(input_len > 0)
       {
-      size_t this_req = std::min(m_max_number_of_bytes_per_request, output_len);
-      output_len -= this_req;
-
-      reseed_check();
-
-      if(input_len > 0)
-         {
-         update(input, input_len);
-         }
-
-      while(this_req)
-         {
-         const size_t to_copy = std::min(this_req, m_V.size());
-         m_mac->update(m_V.data(), m_V.size());
-         m_mac->final(m_V.data());
-         copy_mem(output, m_V.data(), to_copy);
+      update(input, input_len);
+      }
 
-         output += to_copy;
-         this_req -= to_copy;
-         }
+   while(output_len > 0)
+      {
+      const size_t to_copy = std::min(output_len, m_V.size());
+      m_mac->update(m_V.data(), m_V.size());
+      m_mac->final(m_V.data());
+      copy_mem(output, m_V.data(), to_copy);
 
-      update(input, input_len);
+      output += to_copy;
+      output_len -= to_copy;
       }
 
+   update(input, input_len);
    }
 
 /*
@@ -171,33 +190,8 @@ void HMAC_DRBG::update(const uint8_t input[], size_t input_len)
       }
    }
 
-void HMAC_DRBG::add_entropy(const uint8_t input[], size_t input_len)
-   {
-   update(input, input_len);
-
-   if(8*input_len >= security_level())
-      {
-      reset_reseed_counter();
-      }
-   }
-
 size_t HMAC_DRBG::security_level() const
    {
-   // security strength of the hash function
-   // for pre-image resistance (see NIST SP 800-57)
-   // SHA-160: 128 bits, SHA-224, SHA-512/224: 192 bits,
-   // SHA-256, SHA-512/256, SHA-384, SHA-512: >= 256 bits
-   // NIST SP 800-90A only supports up to 256 bits though
-
-   const size_t output_length = m_mac->output_length();
-
-   if(output_length < 32)
-      {
-      return (output_length - 4) * 8;
-      }
-   else
-      {
-      return 32 * 8;
-      }
+   return m_security_level;
    }
 }
diff --git a/src/lib/rng/hmac_drbg/hmac_drbg.h b/src/lib/rng/hmac_drbg/hmac_drbg.h
index 1d4c81ab3..a4c288c74 100644
--- a/src/lib/rng/hmac_drbg/hmac_drbg.h
+++ b/src/lib/rng/hmac_drbg/hmac_drbg.h
@@ -34,6 +34,11 @@ class BOTAN_PUBLIC_API(2,0) HMAC_DRBG final : public Stateful_RNG
       explicit HMAC_DRBG(std::unique_ptr<MessageAuthenticationCode> prf);
 
       /**
+      * Constructor taking a string for the hash
+      */
+      explicit HMAC_DRBG(const std::string& hmac_hash);
+
+      /**
       * Initialize an HMAC_DRBG instance with the given MAC as PRF (normally HMAC)
       *
       * Automatic reseeding from @p underlying_rng will take place after
@@ -119,39 +124,25 @@ class BOTAN_PUBLIC_API(2,0) HMAC_DRBG final : public Stateful_RNG
                 size_t reseed_interval = BOTAN_RNG_DEFAULT_RESEED_INTERVAL,
                 size_t max_number_of_bytes_per_request = 64 * 1024);
 
-      /**
-      * Constructor taking a string for the hash
-      */
-      explicit HMAC_DRBG(const std::string& hmac_hash) :
-         Stateful_RNG(),
-         m_mac(MessageAuthenticationCode::create_or_throw("HMAC(" + hmac_hash + ")")),
-         m_max_number_of_bytes_per_request(64 * 1024)
-         {
-         clear();
-         }
-
       std::string name() const override;
 
-      void clear() override;
-
-      void randomize(uint8_t output[], size_t output_len) override;
-
-      void randomize_with_input(uint8_t output[], size_t output_len,
-                                const uint8_t input[], size_t input_len) override;
-
-      void add_entropy(const uint8_t input[], size_t input_len) override;
-
       size_t security_level() const override;
 
       size_t max_number_of_bytes_per_request() const override
          { return m_max_number_of_bytes_per_request; }
 
    private:
-      void update(const uint8_t input[], size_t input_len);
+      void update(const uint8_t input[], size_t input_len) override;
+
+      void generate_output(uint8_t output[], size_t output_len,
+                           const uint8_t input[], size_t input_len) override;
+
+      void clear_state() override;
 
       std::unique_ptr<MessageAuthenticationCode> m_mac;
       secure_vector<uint8_t> m_V;
       const size_t m_max_number_of_bytes_per_request;
+      const size_t m_security_level;
    };
 
 }
diff --git a/src/lib/rng/rng.h b/src/lib/rng/rng.h
index 0959802df..29fb4f0ab 100644
--- a/src/lib/rng/rng.h
+++ b/src/lib/rng/rng.h
@@ -225,8 +225,12 @@ class BOTAN_PUBLIC_API(2,0) Null_RNG final : public RandomNumberGenerator
 * Wraps access to a RNG in a mutex
 * Note that most of the time it's much better to use a RNG per thread
 * otherwise the RNG will act as an unnecessary contention point
+*
+* Since 2.16.0 all Stateful_RNG instances have an internal lock, so
+* this class is no longer needed. It will be removed in a future major
+* release.
 */
-class BOTAN_PUBLIC_API(2,0) Serialized_RNG final : public RandomNumberGenerator
+class BOTAN_PUBLIC_API(2,0) BOTAN_DEPRECATED("No longer required") Serialized_RNG final : public RandomNumberGenerator
    {
    public:
       void randomize(uint8_t out[], size_t len) override
@@ -273,7 +277,7 @@ class BOTAN_PUBLIC_API(2,0) Serialized_RNG final : public RandomNumberGenerator
          m_rng->add_entropy(in, len);
          }
 
-      BOTAN_DEPRECATED("Use Serialized_RNG(new AutoSeeded_RNG)") Serialized_RNG();
+      BOTAN_DEPRECATED("Use Serialized_RNG(new AutoSeeded_RNG) instead") Serialized_RNG();
 
       explicit Serialized_RNG(RandomNumberGenerator* rng) : m_rng(rng) {}
    private:
diff --git a/src/lib/rng/stateful_rng/stateful_rng.cpp b/src/lib/rng/stateful_rng/stateful_rng.cpp
index 10b3ab84b..c7b3484ee 100644
--- a/src/lib/rng/stateful_rng/stateful_rng.cpp
+++ b/src/lib/rng/stateful_rng/stateful_rng.cpp
@@ -1,5 +1,5 @@
 /*
-* (C) 2016 Jack Lloyd
+* (C) 2016,2020 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -8,66 +8,118 @@
 #include <botan/internal/os_utils.h>
 #include <botan/loadstor.h>
 
-#if defined(BOTAN_HAS_PROCESSOR_RNG)
-  #include <botan/processor_rng.h>
+#if defined(BOTAN_HAS_SYSTEM_RNG)
+  #include <botan/system_rng.h>
 #endif
 
 namespace Botan {
 
 void Stateful_RNG::clear()
    {
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
    m_reseed_counter = 0;
    m_last_pid = 0;
+   clear_state();
    }
 
 void Stateful_RNG::force_reseed()
    {
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
    m_reseed_counter = 0;
    }
 
 bool Stateful_RNG::is_seeded() const
    {
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
    return m_reseed_counter > 0;
    }
 
-void Stateful_RNG::initialize_with(const uint8_t input[], size_t len)
+void Stateful_RNG::add_entropy(const uint8_t input[], size_t input_len)
    {
-   add_entropy(input, len);
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
+
+   update(input, input_len);
 
-   if(8*len >= security_level())
+   if(8*input_len >= security_level())
       {
       reset_reseed_counter();
       }
    }
 
+void Stateful_RNG::initialize_with(const uint8_t input[], size_t len)
+   {
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
+
+   clear();
+   add_entropy(input, len);
+   }
+
+void Stateful_RNG::randomize(uint8_t output[], size_t output_len)
+   {
+   randomize_with_input(output, output_len, nullptr, 0);
+   }
+
 void Stateful_RNG::randomize_with_ts_input(uint8_t output[], size_t output_len)
    {
-   uint8_t additional_input[24] = { 0 };
+   uint8_t additional_input[20] = { 0 };
 
    store_le(OS::get_high_resolution_clock(), additional_input);
 
-#if defined(BOTAN_HAS_PROCESSOR_RNG)
-   if(Processor_RNG::available())
+#if defined(BOTAN_HAS_SYSTEM_RNG)
+   System_RNG system_rng;
+   system_rng.randomize(additional_input + 8, sizeof(additional_input) - 8);
+#else
+   store_le(OS::get_system_timestamp_ns(), additional_input + 8);
+   store_le(OS::get_process_id(), additional_input + 16);
+#endif
+
+   randomize_with_input(output, output_len, additional_input, sizeof(additional_input));
+   }
+
+void Stateful_RNG::randomize_with_input(uint8_t output[], size_t output_len,
+                                        const uint8_t input[], size_t input_len)
+   {
+   if(output_len == 0)
+      return;
+
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
+
+   const size_t max_per_request = max_number_of_bytes_per_request();
+
+   if(max_per_request == 0) // no limit
       {
-      Processor_RNG hwrng;
-      hwrng.randomize(additional_input + 8, sizeof(additional_input) - 8);
+      reseed_check();
+      this->generate_output(output, output_len, input, input_len);
       }
    else
-#endif
       {
-      store_le(OS::get_system_timestamp_ns(), additional_input + 8);
-      store_le(m_last_pid, additional_input + 16);
-      store_le(static_cast<uint32_t>(m_reseed_counter), additional_input + 20);
-      }
+      while(output_len > 0)
+         {
+         const size_t this_req = std::min(max_per_request, output_len);
 
-   randomize_with_input(output, output_len, additional_input, sizeof(additional_input));
+         /*
+         * We split the request into several requests to the underlying DRBG but
+         * pass the input to each invocation. It might be more sensible to only
+         * provide it for the first invocation, however between 2.0 and 2.15
+         * HMAC_DRBG always provided it for all requests so retain that here.
+         */
+
+         reseed_check();
+         this->generate_output(output, this_req, input, input_len);
+
+         output += this_req;
+         output_len -= this_req;
+         }
+      }
    }
 
 size_t Stateful_RNG::reseed(Entropy_Sources& srcs,
                             size_t poll_bits,
                             std::chrono::milliseconds poll_timeout)
    {
-   size_t bits_collected = RandomNumberGenerator::reseed(srcs, poll_bits, poll_timeout);
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
+
+   const size_t bits_collected = RandomNumberGenerator::reseed(srcs, poll_bits, poll_timeout);
 
    if(bits_collected >= security_level())
       {
@@ -79,6 +131,8 @@ size_t Stateful_RNG::reseed(Entropy_Sources& srcs,
 
 void Stateful_RNG::reseed_from_rng(RandomNumberGenerator& rng, size_t poll_bits)
    {
+   lock_guard_type<recursive_mutex_type> lock(m_mutex);
+
    RandomNumberGenerator::reseed_from_rng(rng, poll_bits);
 
    if(poll_bits >= security_level())
@@ -87,8 +141,16 @@ void Stateful_RNG::reseed_from_rng(RandomNumberGenerator& rng, size_t poll_bits)
       }
    }
 
+void Stateful_RNG::reset_reseed_counter()
+   {
+   // Lock is held whenever this function is called
+   m_reseed_counter = 1;
+   }
+
 void Stateful_RNG::reseed_check()
    {
+   // Lock is held whenever this function is called
+
    const uint32_t cur_pid = OS::get_process_id();
 
    const bool fork_detected = (m_last_pid > 0) && (cur_pid != m_last_pid);
diff --git a/src/lib/rng/stateful_rng/stateful_rng.h b/src/lib/rng/stateful_rng/stateful_rng.h
index 9bf04327a..e1311fcc9 100644
--- a/src/lib/rng/stateful_rng/stateful_rng.h
+++ b/src/lib/rng/stateful_rng/stateful_rng.h
@@ -8,6 +8,7 @@
 #define BOTAN_STATEFUL_RNG_H_
 
 #include <botan/rng.h>
+#include <botan/mutex.h>
 
 namespace Botan {
 
@@ -85,6 +86,13 @@ class BOTAN_PUBLIC_API(2,0) Stateful_RNG : public RandomNumberGenerator
       void reseed_from_rng(RandomNumberGenerator& rng,
                            size_t poll_bits = BOTAN_RNG_RESEED_POLL_BITS) override final;
 
+      void add_entropy(const uint8_t input[], size_t input_len) override final;
+
+      void randomize(uint8_t output[], size_t output_len) override final;
+
+      void randomize_with_input(uint8_t output[], size_t output_len,
+                                const uint8_t input[], size_t input_len) override final;
+
       /**
       * Overrides default implementation and also includes the current
       * process ID and the reseed counter.
@@ -119,18 +127,23 @@ class BOTAN_PUBLIC_API(2,0) Stateful_RNG : public RandomNumberGenerator
 
       size_t reseed_interval() const { return m_reseed_interval; }
 
-      void clear() override;
+      void clear() override final;
 
    protected:
       void reseed_check();
 
-      /**
-      * Called by a subclass to notify that a reseed has been
-      * successfully performed.
-      */
-      void reset_reseed_counter() { m_reseed_counter = 1; }
+      virtual void generate_output(uint8_t output[], size_t output_len,
+                                   const uint8_t input[], size_t input_len) = 0;
+
+      virtual void update(const uint8_t input[], size_t input_len) = 0;
+
+      virtual void clear_state() = 0;
 
    private:
+      void reset_reseed_counter();
+
+      mutable recursive_mutex_type m_mutex;
+
       // A non-owned and possibly null pointer to shared RNG
       RandomNumberGenerator* m_underlying_rng = nullptr;
 
diff --git a/src/lib/utils/mutex.h b/src/lib/utils/mutex.h
index 34fed5c81..a230af988 100644
--- a/src/lib/utils/mutex.h
+++ b/src/lib/utils/mutex.h
@@ -17,6 +17,7 @@ namespace Botan {
 
 template<typename T> using lock_guard_type = std::lock_guard<T>;
 typedef std::mutex mutex_type;
+typedef std::recursive_mutex recursive_mutex_type;
 
 }
 
@@ -49,6 +50,7 @@ class noop_mutex final
    };
 
 typedef noop_mutex mutex_type;
+typedef noop_mutex recursive_mutex_type;
 template<typename T> using lock_guard_type = lock_guard<T>;
 
 }