From 3a9245f74336696ef707ed1d6537fbd33fc74135 Mon Sep 17 00:00:00 2001 From: sowle Date: Thu, 31 Jul 2025 04:14:10 +0300 Subject: [PATCH] crypto::random refactoring and improvements (credits to @dimmarvel for spotting the thread safety issue) --- src/crypto/crypto.cpp | 54 +++++++++++++------------------ src/crypto/crypto.h | 20 +++++++++--- src/crypto/random.c | 29 +++++++++-------- src/crypto/random.h | 17 ++++------ tests/core_tests/random_helper.h | 8 ++--- tests/unit_tests/db_accessors.cpp | 2 +- tests/unit_tests/db_tests.cpp | 6 ++-- 7 files changed, 67 insertions(+), 69 deletions(-) diff --git a/src/crypto/crypto.cpp b/src/crypto/crypto.cpp index cfdfb866..b2b7c4bb 100644 --- a/src/crypto/crypto.cpp +++ b/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(&I_); const key_image L = *reinterpret_cast(&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(point); @@ -78,9 +66,10 @@ namespace crypto { return &reinterpret_cast(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) { - lock_guard lock(random_lock); + void crypto_ops::generate_keys(public_key &pub, secret_key &sec) + { + std::lock_guard 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 lock(random_lock); + std::lock_guard 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 lock(random_lock); + std::lock_guard 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(sig[i].r); + random_scalar_no_lock(sig[i].c); + random_scalar_no_lock(sig[i].r); if (ge_frombytes_vartime(&tmp3, &*pubs[i]) != 0) { abort(); } diff --git a/src/crypto/crypto.h b/src/crypto/crypto.h index 08cf26d5..9433ad4f 100644 --- a/src/crypto/crypto.h +++ b/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 lock(random_lock_accessor()); + generate_random_bytes_no_lock(size, p_data); + } + + /* Generate a value filled with random bytes. */ template - typename std::enable_if::value, T>::type rand() { + typename std::enable_if::value, T>::type rand() + { typename std::remove_cv::type res; - std::lock_guard lock(random_lock); - generate_random_bytes(sizeof(T), &res); + std::lock_guard lock(random_lock_accessor()); + 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 { diff --git a/src/crypto/random.c b/src/crypto/random.c index a54bdf74..e38fb18d 100644 --- a/src/crypto/random.c +++ b/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 #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 #include -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 #include -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) { - grant_random_initialize(); +void generate_random_bytes_no_lock(size_t n, void *result) +{ + grant_random_initialize_no_lock(); #if !defined(NDEBUG) assert(curstate == 1); curstate = 2; diff --git a/src/crypto/random.h b/src/crypto/random.h index dbc79302..7db79b8b 100644 --- a/src/crypto/random.h +++ b/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 #include -// use the cryptographically secure Pseudo-Random Number Generator provided by the operating system -void generate_system_random_bytes(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); +// NOT thread-safe, use with caution +void generate_random_bytes_no_lock(size_t n, void *result); #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 diff --git a/tests/core_tests/random_helper.h b/tests/core_tests/random_helper.h index 61d49f80..f548e324 100644 --- a/tests/core_tests/random_helper.h +++ b/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]; diff --git a/tests/unit_tests/db_accessors.cpp b/tests/unit_tests/db_accessors.cpp index 4994d1fb..a549e776 100644 --- a/tests/unit_tests/db_accessors.cpp +++ b/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(new tools::db::lmdb_db_backend), m_rw_lock); diff --git a/tests/unit_tests/db_tests.cpp b/tests/unit_tests/db_tests.cpp index 61a0ae19..4eb7a2d5 100644 --- a/tests/unit_tests/db_tests.cpp +++ b/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_initialize_with_seed(0); // this mades this test deterministic + crypto::random_prng_get_state_no_lock(prng_state, sizeof prng_state); // store current RPNG state + 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); }