crypto::random refactoring and improvements (credits to @dimmarvel for spotting the thread safety issue)

src/crypto/crypto.cpp

@@ -1,4 +1,4 @@
-// Copyright (c) 2014-2022 Zano Project
+// Copyright (c) 2014-2025 Zano Project
 // Copyright (c) 2014-2018 The Louisdor Project
 // Copyright (c) 2012-2013 The Cryptonote developers
 // Distributed under the MIT/X11 software license, see the accompanying
@@ -35,32 +35,20 @@ namespace crypto {
   const key_image I = *reinterpret_cast<const key_image*>(&I_);
   const key_image L = *reinterpret_cast<const key_image*>(&L_);
 
-  struct random_init_singleton
-  {
-    random_init_singleton()
-    {
-      grant_random_initialize();
-    }
-  };
-
-  random_init_singleton init_rand; //place initializer here to avoid grant_random_initialize first call after threads will be possible(local static variables init is not thread-safe)
-
-  using std::abort;
-  using std::int32_t;
-  using std::int64_t;
-  using std::lock_guard;
-  using std::mutex;
-  using std::size_t;
-  using std::uint32_t;
-  using std::uint64_t;
-
   extern "C" {
 #include "crypto-ops.h"
 #include "random.h"
   }
   
 
-  mutex random_lock;
+  std::mutex& random_lock_accessor() noexcept
+  {
+    // this is a thread-safe approach
+    // note section 6.7: "If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization."
+    static std::mutex random_lock;
+    return random_lock;
+  }
+
 
   static inline unsigned char *operator &(ec_point &point) {
     return &reinterpret_cast<unsigned char &>(point);
@@ -78,9 +66,10 @@ namespace crypto {
     return &reinterpret_cast<const unsigned char &>(scalar);
   }
 
-  static inline void random_scalar(ec_scalar &res) {
+  static inline void random_scalar_no_lock(ec_scalar &res)
+  {
     unsigned char tmp[64];
-    generate_random_bytes(64, tmp);
+    generate_random_bytes_no_lock(64, tmp);
     sc_reduce(tmp);
     memcpy(&res, tmp, 32);
   }
@@ -118,10 +107,11 @@ namespace crypto {
     sc_reduce32(&res);
   }
 
-  void crypto_ops::generate_keys(public_key &pub, secret_key &sec) {
+  void crypto_ops::generate_keys(public_key &pub, secret_key &sec)
-    lock_guard<mutex> lock(random_lock);
+  {
+    std::lock_guard<std::mutex> lock(random_lock_accessor());
     ge_p3 point;
-    random_scalar(sec);
+    random_scalar_no_lock(sec);
     ge_scalarmult_base(&point, &sec);
     ge_p3_tobytes(&pub, &point);
   }
@@ -239,7 +229,7 @@ namespace crypto {
   };
 
   void crypto_ops::generate_signature(const hash &prefix_hash, const public_key &pub, const secret_key &sec, signature &sig) {
-    lock_guard<mutex> lock(random_lock);
+    std::lock_guard<std::mutex> lock(random_lock_accessor());
     ge_p3 tmp3;
     ec_scalar k;
     s_comm buf;
@@ -255,7 +245,7 @@ namespace crypto {
 #endif
     buf.h = prefix_hash;
     buf.key = pub;
-    random_scalar(k);
+    random_scalar_no_lock(k);
     ge_scalarmult_base(&tmp3, &k);
     ge_p3_tobytes(&buf.comm, &tmp3);
     hash_to_scalar(&buf, sizeof(s_comm), sig.c);
@@ -325,7 +315,7 @@ POP_VS_WARNINGS
     const public_key *const *pubs, size_t pubs_count,
     const secret_key &sec, size_t sec_index,
     signature *sig) {
-    lock_guard<mutex> lock(random_lock);
+    std::lock_guard<std::mutex> lock(random_lock_accessor());
     size_t i;
     ge_p3 image_unp;
     ge_dsmp image_pre;
@@ -362,15 +352,15 @@ POP_VS_WARNINGS
       ge_p2 tmp2;
       ge_p3 tmp3;
       if (i == sec_index) {
-        random_scalar(k);
+        random_scalar_no_lock(k);
         ge_scalarmult_base(&tmp3, &k);
         ge_p3_tobytes(&buf->ab[i].a, &tmp3);
         hash_to_ec(*pubs[i], tmp3);
         ge_scalarmult(&tmp2, &k, &tmp3);
         ge_tobytes(&buf->ab[i].b, &tmp2);
       } else {
-        random_scalar(sig[i].c);
+        random_scalar_no_lock(sig[i].c);
-        random_scalar(sig[i].r);
+        random_scalar_no_lock(sig[i].r);
         if (ge_frombytes_vartime(&tmp3, &*pubs[i]) != 0) {
           abort();
         }

src/crypto/crypto.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2014-2018 Zano Project
+// Copyright (c) 2014-2025 Zano Project
 // Copyright (c) 2014-2018 The Louisdor Project
 // Copyright (c) 2012-2013 The Cryptonote developers
 // Distributed under the MIT/X11 software license, see the accompanying
@@ -31,7 +31,7 @@ namespace crypto {
 #include "random.h"
   }
 
-  extern std::mutex random_lock;
+  std::mutex& random_lock_accessor() noexcept;
 
 #pragma pack(push, 1)
   POD_CLASS ec_point {
@@ -114,17 +114,27 @@ namespace crypto {
 
   };
 
+  // thread-safe version
+  inline void generate_random_bytes(size_t size, void* p_data)
+  {
+    std::lock_guard<std::mutex> lock(random_lock_accessor());
+    generate_random_bytes_no_lock(size, p_data);
+  }
+
+
   /* Generate a value filled with random bytes.
    */
   template<typename T>
-  typename std::enable_if<std::is_pod<T>::value, T>::type rand() {
+  typename std::enable_if<std::is_pod<T>::value, T>::type rand()
+  {
     typename std::remove_cv<T>::type res;
-    std::lock_guard<std::mutex> lock(random_lock);
+    std::lock_guard<std::mutex> lock(random_lock_accessor());
-    generate_random_bytes(sizeof(T), &res);
+    generate_random_bytes_no_lock(sizeof(T), &res);
     return res;
   }
 
   /* An adapter, to be used with std::shuffle, etc.
+   * Uses thread-safe crypto::rand<>().
    */
   struct uniform_random_bit_generator
   {

src/crypto/random.c

@@ -1,3 +1,4 @@
+// Copyright (c) 2018-2025 Zano Project
 // Copyright (c) 2012-2013 The Cryptonote developers
 // Distributed under the MIT/X11 software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
@@ -7,7 +8,6 @@
 #include <string.h>
 
 #include "hash-ops.h"
-//#include "initializer.h"
 #include "random.h"
 
 static_assert(RANDOM_STATE_SIZE >= HASH_DATA_AREA, "Invalid RANDOM_STATE_SIZE");
@@ -17,7 +17,7 @@ static_assert(RANDOM_STATE_SIZE >= HASH_DATA_AREA, "Invalid RANDOM_STATE_SIZE");
 #include <windows.h>
 #include <wincrypt.h>
 
-void generate_system_random_bytes(size_t n, void *result) {
+void generate_system_random_bytes_no_lock(size_t n, void *result) {
   HCRYPTPROV prov;
 #define must_succeed(x) do if (!(x)) assert(0); while (0)
   if(!CryptAcquireContext(&prov, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
@@ -40,7 +40,7 @@ void generate_system_random_bytes(size_t n, void *result) {
 #include <sys/types.h>
 #include <unistd.h>
 
-void generate_system_random_bytes(size_t n, void *result) {
+void generate_system_random_bytes_no_lock(size_t n, void *result) {
   int fd;
   if ((fd = open("/dev/urandom", O_RDONLY | O_NOCTTY | O_CLOEXEC)) < 0) {
     exit(EXIT_FAILURE);
@@ -70,6 +70,8 @@ void generate_system_random_bytes(size_t n, void *result) {
 
 static union hash_state state;
 
+static_assert(sizeof(union hash_state) == RANDOM_STATE_SIZE, "RANDOM_STATE_SIZE and hash_state size missmatch");
+
 #if !defined(NDEBUG)
 static volatile int curstate; /* To catch thread safety problems. */
 #endif
@@ -86,7 +88,7 @@ FINALIZER(deinit_random) {
 //INITIALIZER(init_random) {
 void init_random(void)
 {
-  generate_system_random_bytes(32, &state);
+  generate_system_random_bytes_no_lock(RANDOM_STATE_SIZE, &state);
   //REGISTER_FINA\LIZER(deinit_random);
 #if !defined(NDEBUG)
   assert(curstate == 0);
@@ -95,7 +97,7 @@ void init_random(void)
 }
 
 
-void grant_random_initialize(void)
+void grant_random_initialize_no_lock(void)
 {
   static bool initalized = false;
   if(!initalized)
@@ -105,9 +107,9 @@ void grant_random_initialize(void)
   }
 }
 
-void random_prng_initialize_with_seed(uint64_t seed)
+void random_prng_initialize_with_seed_no_lock(uint64_t seed)
 {
-  grant_random_initialize();
+  grant_random_initialize_no_lock();
 #if !defined(NDEBUG)
   assert(curstate == 1);
   curstate = 3;
@@ -122,9 +124,9 @@ void random_prng_initialize_with_seed(uint64_t seed)
 #endif
 }
 
-void random_prng_get_state(void *state_buffer, const size_t buffer_size)
+void random_prng_get_state_no_lock(void *state_buffer, const size_t buffer_size)
 {
-  grant_random_initialize();
+  grant_random_initialize_no_lock();
 #if !defined(NDEBUG)
   assert(curstate == 1);
   curstate = 4;
@@ -139,9 +141,9 @@ void random_prng_get_state(void *state_buffer, const size_t buffer_size)
 #endif
 }
 
-void random_prng_set_state(const void *state_buffer, const size_t buffer_size)
+void random_prng_set_state_no_lock(const void *state_buffer, const size_t buffer_size)
 {
-  grant_random_initialize();
+  grant_random_initialize_no_lock();
 #if !defined(NDEBUG)
   assert(curstate == 1);
   curstate = 5;
@@ -156,8 +158,9 @@ void random_prng_set_state(const void *state_buffer, const size_t buffer_size)
 #endif
 }
 
-void generate_random_bytes(size_t n, void *result) {
+void generate_random_bytes_no_lock(size_t n, void *result)
-  grant_random_initialize();
+{
+  grant_random_initialize_no_lock();
 #if !defined(NDEBUG)
   assert(curstate == 1);
   curstate = 2;

src/crypto/random.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2018-2019 Zano Project
+// Copyright (c) 2018-2025 Zano Project
 // Copyright (c) 2014-2018 The Boolberry developers
 // Copyright (c) 2012-2013 The Cryptonote developers
 // Distributed under the MIT/X11 software license, see the accompanying
@@ -9,13 +9,8 @@
 #include <stddef.h>
 #include <stdint.h>
 
-// use the cryptographically secure Pseudo-Random Number Generator provided by the operating system
+// NOT thread-safe, use with caution
-void generate_system_random_bytes(size_t n, void *result);
+void generate_random_bytes_no_lock(size_t n, void *result);
-
-void generate_random_bytes(size_t n, void *result);
-
-// checks if PRNG is initialized and initializes it if necessary
-void grant_random_initialize(void);
 
 #define RANDOM_STATE_SIZE 200
 
@@ -24,14 +19,14 @@ void grant_random_initialize(void);
 // reinitializes PRNG with the given seed
 // !!!ATTENTION!!!! Improper use of this routine may lead to SECURITY BREACH!
 // Use with care and ONLY for tests or debug purposes!
-void random_prng_initialize_with_seed(uint64_t seed);
+void random_prng_initialize_with_seed_no_lock(uint64_t seed);
 
 // gets internal RPNG state (state_buffer should be 200 bytes long)
-void random_prng_get_state(void *state_buffer, const size_t buffer_size);
+void random_prng_get_state_no_lock(void *state_buffer, const size_t buffer_size);
 
 // sets internal RPNG state (state_buffer should be 200 bytes long)
 // !!!ATTENTION!!!! Improper use of this routine may lead to SECURITY BREACH!
 // Use with care and ONLY for tests or debug purposes!
-void random_prng_set_state(const void *state_buffer, const size_t buffer_size);
+void random_prng_set_state_no_lock(const void *state_buffer, const size_t buffer_size);
 
 #endif // #ifdef USE_INSECURE_RANDOM_RPNG_ROUTINES

tests/core_tests/random_helper.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2014-2019 Zano Project
+// Copyright (c) 2014-2025 Zano Project
 // Copyright (c) 2014-2018 The Louisdor Project
 // Distributed under the MIT/X11 software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
@@ -16,15 +16,15 @@ struct random_state_test_restorer
 {
   random_state_test_restorer()
   {
-    crypto::random_prng_get_state(&m_state, sizeof m_state);
+    crypto::random_prng_get_state_no_lock(&m_state, sizeof m_state);
   }
   ~random_state_test_restorer()
   {
-    crypto::random_prng_set_state(&m_state, sizeof m_state);
+    crypto::random_prng_set_state_no_lock(&m_state, sizeof m_state);
   }
   static void reset_random(uint64_t seed = 0)
   {
-    crypto::random_prng_initialize_with_seed(seed);
+    crypto::random_prng_initialize_with_seed_no_lock(seed);
   }
 private:
   uint8_t m_state[RANDOM_STATE_SIZE];

tests/unit_tests/db_accessors.cpp

@@ -317,7 +317,7 @@ struct bcs_stub_t
 
 TEST(db_accessor_tests, median_db_cache_test)
 {
-  crypto::random_prng_initialize_with_seed(0); // make this test deterministic (the same crypto::rand() sequence)
+  crypto::random_prng_initialize_with_seed_no_lock(0); // make this test deterministic (the same crypto::rand() sequence)
 
   epee::shared_recursive_mutex m_rw_lock;
   tools::db::basic_db_accessor m_db(std::shared_ptr<tools::db::i_db_backend>(new tools::db::lmdb_db_backend), m_rw_lock);

tests/unit_tests/db_tests.cpp

@@ -362,8 +362,8 @@ namespace db_test
   void multithread_test_1()
   {
     char prng_state[200] = {};
-    crypto::random_prng_get_state(prng_state, sizeof prng_state); // store current RPNG state
+    crypto::random_prng_get_state_no_lock(prng_state, sizeof prng_state); // store current RPNG state
-    crypto::random_prng_initialize_with_seed(0); // this mades this test deterministic
+    crypto::random_prng_initialize_with_seed_no_lock(0); // this mades this test deterministic
 
     bool result = false;
     try
@@ -377,7 +377,7 @@ namespace db_test
     }
 
     // restore PRNG state to keep other tests unaffected
-    crypto::random_prng_set_state(prng_state, sizeof prng_state);
+    crypto::random_prng_set_state_no_lock(prng_state, sizeof prng_state);
 
     ASSERT_TRUE(result);
   }