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;
