Make QUIC insecure randomness thread-safe cl/358886449 accidentally causes undefined behavior by introducing insecure randomness in a thread-unsafe way, even though QuicRandom is expected to be thread-safe. This CL switches the random number generator state to thread_local storage to avoid the issue. This is slightly slower than before, but still 100x faster than secure randomness. This CL replaces the feature flag to ensure the thread-unsafe code does not get enabled. Protected by FLAGS_quic_reloadable_flag_quic_stateless_reset_faster_random. PiperOrigin-RevId: 358941810 Change-Id: Ie00942a0fad3f459a9eeff1a32a34af73da7f5d0
diff --git a/quic/core/crypto/quic_random.cc b/quic/core/crypto/quic_random.cc index 8292e9b..56657b5 100644 --- a/quic/core/crypto/quic_random.cc +++ b/quic/core/crypto/quic_random.cc
@@ -8,15 +8,43 @@ #include "third_party/boringssl/src/include/openssl/rand.h" #include "quic/platform/api/quic_bug_tracker.h" +#include "quic/platform/api/quic_logging.h" #include "common/platform/api/quiche_logging.h" namespace quic { namespace { +// Insecure randomness in DefaultRandom uses an implementation of +// xoshiro256++ 1.0 based on code in the public domain from +// <http://prng.di.unimi.it/xoshiro256plusplus.c>. + +inline uint64_t Xoshiro256PlusPlusRotLeft(uint64_t x, int k) { + return (x << k) | (x >> (64 - k)); +} + +uint64_t Xoshiro256PlusPlus() { + static thread_local uint64_t rng_state[4]; + static thread_local bool rng_state_initialized = false; + if (QUIC_PREDICT_FALSE(!rng_state_initialized)) { + RAND_bytes(reinterpret_cast<uint8_t*>(&rng_state), sizeof(rng_state)); + rng_state_initialized = true; + } + const uint64_t result = + Xoshiro256PlusPlusRotLeft(rng_state[0] + rng_state[3], 23) + rng_state[0]; + const uint64_t t = rng_state[1] << 17; + rng_state[2] ^= rng_state[0]; + rng_state[3] ^= rng_state[1]; + rng_state[1] ^= rng_state[2]; + rng_state[0] ^= rng_state[3]; + rng_state[2] ^= t; + rng_state[3] = Xoshiro256PlusPlusRotLeft(rng_state[3], 45); + return result; +} + class DefaultRandom : public QuicRandom { public: - DefaultRandom(); + DefaultRandom() {} DefaultRandom(const DefaultRandom&) = delete; DefaultRandom& operator=(const DefaultRandom&) = delete; ~DefaultRandom() override {} @@ -26,13 +54,8 @@ uint64_t RandUint64() override; void InsecureRandBytes(void* data, size_t len) override; uint64_t InsecureRandUint64() override; - - private: - QuicInsecureRandom insecure_random_; }; -DefaultRandom::DefaultRandom() : insecure_random_(this) {} - void DefaultRandom::RandBytes(void* data, size_t len) { RAND_bytes(reinterpret_cast<uint8_t*>(data), len); } @@ -44,11 +67,21 @@ } void DefaultRandom::InsecureRandBytes(void* data, size_t len) { - insecure_random_.InsecureRandBytes(data, len); + while (len >= sizeof(uint64_t)) { + uint64_t random_bytes64 = Xoshiro256PlusPlus(); + memcpy(data, &random_bytes64, sizeof(uint64_t)); + data = reinterpret_cast<char*>(data) + sizeof(uint64_t); + len -= sizeof(uint64_t); + } + if (len > 0) { + QUICHE_DCHECK_LT(len, sizeof(uint64_t)); + uint64_t random_bytes64 = Xoshiro256PlusPlus(); + memcpy(data, &random_bytes64, len); + } } uint64_t DefaultRandom::InsecureRandUint64() { - return insecure_random_.InsecureRandUint64(); + return Xoshiro256PlusPlus(); } } // namespace @@ -59,44 +92,4 @@ return random; } -// QuicInsecureRandom uses an implementation of xoshiro256++ 1.0 based on code -// in the public domain from <http://prng.di.unimi.it/xoshiro256plusplus.c>. - -namespace { -inline uint64_t Xoshiro256PlusPlusRotl(uint64_t x, int k) { - return (x << k) | (x >> (64 - k)); -} -} // namespace - -QuicInsecureRandom::QuicInsecureRandom(QuicRandom* random) { - random->RandBytes(&rng_state_, sizeof(rng_state_)); -} - -void QuicInsecureRandom::InsecureRandBytes(void* data, size_t len) { - while (len >= sizeof(uint64_t)) { - uint64_t random_bytes64 = InsecureRandUint64(); - memcpy(data, &random_bytes64, sizeof(uint64_t)); - data = reinterpret_cast<char*>(data) + sizeof(uint64_t); - len -= sizeof(uint64_t); - } - if (len > 0) { - QUICHE_DCHECK_LT(len, sizeof(uint64_t)); - uint64_t random_bytes64 = InsecureRandUint64(); - memcpy(data, &random_bytes64, len); - } -} - -uint64_t QuicInsecureRandom::InsecureRandUint64() { - const uint64_t result = - Xoshiro256PlusPlusRotl(rng_state_[0] + rng_state_[3], 23) + rng_state_[0]; - const uint64_t t = rng_state_[1] << 17; - rng_state_[2] ^= rng_state_[0]; - rng_state_[3] ^= rng_state_[1]; - rng_state_[1] ^= rng_state_[2]; - rng_state_[0] ^= rng_state_[3]; - rng_state_[2] ^= t; - rng_state_[3] = Xoshiro256PlusPlusRotl(rng_state_[3], 45); - return result; -} - } // namespace quic
diff --git a/quic/core/crypto/quic_random.h b/quic/core/crypto/quic_random.h index ff1c4bb..4d7bf94 100644 --- a/quic/core/crypto/quic_random.h +++ b/quic/core/crypto/quic_random.h
@@ -36,27 +36,6 @@ virtual uint64_t InsecureRandUint64() = 0; }; -// A class that generates non-cryptographically-secure random numbers. It uses -// a QuicRandom instance to seed its initial randomness. This MUST NOT be used -// for any application that requires cryptographically-secure randomness. -class QUIC_EXPORT_PRIVATE QuicInsecureRandom { - public: - // |random| is only used during construction of the QuicInsecureRandom to seed - // its inital state. - explicit QuicInsecureRandom(QuicRandom* random); - - // Generates |len| random bytes in the |data| buffer. This MUST NOT be used - // for any application that requires cryptographically-secure randomness. - void InsecureRandBytes(void* data, size_t len); - - // Returns a random number in the range [0, kuint64max]. This MUST NOT be used - // for any application that requires cryptographically-secure randomness. - uint64_t InsecureRandUint64(); - - private: - uint64_t rng_state_[4]; -}; - } // namespace quic #endif // QUICHE_QUIC_CORE_CRYPTO_QUIC_RANDOM_H_
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 9a88972..97930ff 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -56,7 +56,7 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_server_reverse_validate_new_path, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_single_ack_in_packet2, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_start_peer_migration_earlier, true) -QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_stateless_reset_faster_randomness, false) +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_stateless_reset_faster_random, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_testonly_default_false, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_testonly_default_true, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_use_normalized_sni_for_cert_selectioon, false)
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index 432bd13..fdccef9 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -1292,8 +1292,8 @@ // from comparing the entire packet to a known value. Therefore it has no // cryptographic use, and does not need a secure cryptographic pseudo-random // number generator. It's therefore safe to use WriteInsecureRandomBytes here. - if (GetQuicReloadableFlag(quic_stateless_reset_faster_randomness)) { - QUIC_RELOADABLE_FLAG_COUNT(quic_stateless_reset_faster_randomness); + if (GetQuicReloadableFlag(quic_stateless_reset_faster_random)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_stateless_reset_faster_random); if (!writer.WriteInsecureRandomBytes( QuicRandom::GetInstance(), kMinRandomBytesLengthInStatelessReset)) { return nullptr;