Upgrade the opportunistic CRYPTO_sysrand calls to normal ones We do not believe this is needed anymore. See go/boringssl-and-os-entropy (internal). After this change, we still use RDRAND for the seed and the OS for the personalization string. There's no reason for this now. Rather, this is meant to be a more easily reverted change. I've left some TODOs to clean this up, once this sticks. Update-Note: Builds with BORINGSSL_FIPS will no longer attempt to work around broken OS entropy with RDRAND. The OS is expected to be functional. Bug: 446280903 Change-Id: I60bf9c4c0c1268d76b26301b41097179de453120 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82108 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com>
diff --git a/crypto/bcm_support.h b/crypto/bcm_support.h index faea2bb..dd82f62 100644 --- a/crypto/bcm_support.h +++ b/crypto/bcm_support.h
@@ -35,13 +35,6 @@ // system. void CRYPTO_sysrand(uint8_t *buf, size_t len); -// CRYPTO_sysrand_if_available fills |len| bytes at |buf| with entropy from the -// operating system, or early /dev/urandom data, and returns 1, _if_ the entropy -// pool is initialized or if getrandom() is not available and not in FIPS mode. -// Otherwise it will not block and will instead fill |buf| with all zeros and -// return 0. -int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len); - // CRYPTO_sysrand_for_seed fills |len| bytes at |buf| with entropy from the // operating system. It may draw from the |GRND_RANDOM| pool on Android, // depending on the vendor's configuration.
diff --git a/crypto/fipsmodule/rand/rand.cc.inc b/crypto/fipsmodule/rand/rand.cc.inc index 71ed096..be181e9 100644 --- a/crypto/fipsmodule/rand/rand.cc.inc +++ b/crypto/fipsmodule/rand/rand.cc.inc
@@ -301,12 +301,13 @@ } } - // If we used something other than system entropy then also - // opportunistically read from the system. This avoids solely relying on the - // hardware once the entropy pool has been initialized. + // If we used something other than system entropy then also read from the + // system. This avoids solely relying on the hardware. + // TODO(crbug.com/446280903): Once this change sticks, switch + // |get_seed_entropy| to draw from the OS instead of RDRAND. *out_additional_input_len = 0; - if (want_additional_input && - CRYPTO_sysrand_if_available(additional_input, CTR_DRBG_SEED_LEN)) { + if (want_additional_input) { + CRYPTO_sysrand(additional_input, CTR_DRBG_SEED_LEN); *out_additional_input_len = CTR_DRBG_SEED_LEN; } } @@ -352,13 +353,7 @@ // promised not to fork. if (fork_generation != 0 || fork_unsafe_buffering) { OPENSSL_memset(additional_data, 0, sizeof(additional_data)); - } else if (!have_rdrand()) { - // No alternative so block for OS entropy. - CRYPTO_sysrand(additional_data, sizeof(additional_data)); - } else if (!CRYPTO_sysrand_if_available(additional_data, - sizeof(additional_data)) && - !rdrand(additional_data, sizeof(additional_data))) { - // RDRAND failed: block for OS entropy. + } else { CRYPTO_sysrand(additional_data, sizeof(additional_data)); } }
diff --git a/crypto/rand/deterministic.cc b/crypto/rand/deterministic.cc index ca97d95..f520e10 100644 --- a/crypto/rand/deterministic.cc +++ b/crypto/rand/deterministic.cc
@@ -53,11 +53,6 @@ CRYPTO_chacha_20(out, out, requested, kZeroKey, nonce, 0); } -int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len) { - CRYPTO_sysrand(buf, len); - return 1; -} - void CRYPTO_sysrand_for_seed(uint8_t *out, size_t requested) { CRYPTO_sysrand(out, requested); }
diff --git a/crypto/rand/getentropy.cc b/crypto/rand/getentropy.cc index 64cb3e9..f2956bb 100644 --- a/crypto/rand/getentropy.cc +++ b/crypto/rand/getentropy.cc
@@ -48,11 +48,6 @@ } } -int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len) { - CRYPTO_sysrand(buf, len); - return 1; -} - void CRYPTO_sysrand_for_seed(uint8_t *out, size_t requested) { CRYPTO_sysrand(out, requested); }
diff --git a/crypto/rand/ios.cc b/crypto/rand/ios.cc index dd418df..a9135e4 100644 --- a/crypto/rand/ios.cc +++ b/crypto/rand/ios.cc
@@ -30,11 +30,6 @@ } } -int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len) { - CRYPTO_sysrand(buf, len); - return 1; -} - void CRYPTO_sysrand_for_seed(uint8_t *out, size_t requested) { CRYPTO_sysrand(out, requested); }
diff --git a/crypto/rand/trusty.cc b/crypto/rand/trusty.cc index a2b23ae..ad73a06 100644 --- a/crypto/rand/trusty.cc +++ b/crypto/rand/trusty.cc
@@ -34,11 +34,6 @@ } } -int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len) { - CRYPTO_sysrand(buf, len); - return 1; -} - void CRYPTO_sysrand_for_seed(uint8_t *out, size_t requested) { CRYPTO_sysrand(out, requested); }
diff --git a/crypto/rand/urandom.cc b/crypto/rand/urandom.cc index 868b9f4..6fa393e 100644 --- a/crypto/rand/urandom.cc +++ b/crypto/rand/urandom.cc
@@ -231,30 +231,26 @@ } // fill_with_entropy writes |len| bytes of entropy into |out|. It returns one -// on success and zero on error. If |block| is one, this function will block -// until the entropy pool is initialized. Otherwise, this function may fail, -// setting |errno| to |EAGAIN| if the entropy pool has not yet been initialized. -// If |seed| is one, this function will OR in the value of +// on success and zero on error. This function will block until the entropy pool +// is initialized. If |seed| is one, this function will OR in the value of // |*extra_getrandom_flags_for_seed()| when using |getrandom|. -static int fill_with_entropy(uint8_t *out, size_t len, int block, int seed) { +static int fill_with_entropy(uint8_t *out, size_t len, int seed) { if (len == 0) { return 1; } #if defined(USE_NR_getrandom) int getrandom_flags = 0; - if (!block) { - getrandom_flags |= GRND_NONBLOCK; - } if (seed) { getrandom_flags |= extra_getrandom_flags_for_seed; } #endif CRYPTO_init_sysrand(); - if (block) { - CRYPTO_once(&wait_for_entropy_once, wait_for_entropy); - } + // TODO(crbug.com/446280903): After the change to uniformly use OS entropy has + // stuck, remove this |wait_for_entropy| hook. The |getrandom| calls below + // already wait for entropy. |wait_for_entropy| just added more useful errors. + CRYPTO_once(&wait_for_entropy_once, wait_for_entropy); // Clear |errno| so it has defined value if |read| or |getrandom| // "successfully" returns zero. @@ -289,29 +285,17 @@ // CRYPTO_sysrand puts |requested| random bytes into |out|. void CRYPTO_sysrand(uint8_t *out, size_t requested) { - if (!fill_with_entropy(out, requested, /*block=*/1, /*seed=*/0)) { + if (!fill_with_entropy(out, requested, /*seed=*/0)) { perror("entropy fill failed"); abort(); } } void CRYPTO_sysrand_for_seed(uint8_t *out, size_t requested) { - if (!fill_with_entropy(out, requested, /*block=*/1, /*seed=*/1)) { + if (!fill_with_entropy(out, requested, /*seed=*/1)) { perror("entropy fill failed"); abort(); } } -int CRYPTO_sysrand_if_available(uint8_t *out, size_t requested) { - if (fill_with_entropy(out, requested, /*block=*/0, /*seed=*/0)) { - return 1; - } else if (errno == EAGAIN) { - OPENSSL_memset(out, 0, requested); - return 0; - } else { - perror("opportunistic entropy fill failed"); - abort(); - } -} - #endif // OPENSSL_RAND_URANDOM
diff --git a/crypto/rand/urandom_test.cc b/crypto/rand/urandom_test.cc index 6657cdd..e610607 100644 --- a/crypto/rand/urandom_test.cc +++ b/crypto/rand/urandom_test.cc
@@ -649,7 +649,7 @@ // Probe for getrandom support ret.push_back(Event::GetRandom(1, GRND_NONBLOCK)); std::function<void()> wait_for_entropy; - std::function<bool(bool, size_t)> sysrand; + std::function<bool(size_t)> sysrand; if (flags & NO_GETRANDOM) { if (kIsFIPS) { @@ -664,7 +664,7 @@ return ret; } - sysrand = [&ret, flags](bool block, size_t len) { + sysrand = [&ret, flags](size_t len) { ret.push_back(Event::UrandomRead(len)); if (flags & URANDOM_ERROR) { ret.push_back(Event::Abort()); @@ -688,73 +688,48 @@ ret.push_back(Event::GetRandom(1, 0)); getrandom_ready = true; }; - sysrand = [&ret, &wait_for_entropy](bool block, size_t len) { - if (block) { - wait_for_entropy(); - } - ret.push_back(Event::GetRandom(len, block ? 0 : GRND_NONBLOCK)); + sysrand = [&ret, &wait_for_entropy](size_t len) { + wait_for_entropy(); + ret.push_back(Event::GetRandom(len, 0)); return true; }; } - const size_t kSeedLength = CTR_DRBG_SEED_LEN * (kIsFIPS ? 10 : 1); + const size_t kSeedLength = + CTR_DRBG_SEED_LEN * (kIsFIPS ? 10 : 1) + (kIsFIPS ? 16 : 0); const size_t kAdditionalDataLength = 32; if (!have_rdrand()) { if (!have_fork_detection()) { - if (!sysrand(true, kAdditionalDataLength)) { + if (!sysrand(kAdditionalDataLength)) { return ret; } used_daemon = kUsesDaemon && AppendDaemonEvents(&ret, flags); } - if ( // Initialise CRNGT. - (!used_daemon && !sysrand(true, kSeedLength + (kIsFIPS ? 16 : 0))) || - // Personalisation draw if the daemon was used. - (used_daemon && !sysrand(false, CTR_DRBG_SEED_LEN)) || - // Second entropy draw. - (!have_fork_detection() && !sysrand(true, kAdditionalDataLength))) { + if (// Initialise CRNGT. If the daemon is used, the OS is used for the + // personalisation data of length |CTR_DRBG_SEED_LEN|. Otherwise, it + // used for the seed itself, including FIPS overread. + !sysrand(used_daemon ? CTR_DRBG_SEED_LEN : kSeedLength) || + // Second additional data, when other fork-safety measures have failed. + (!have_fork_detection() && !sysrand(kAdditionalDataLength))) { return ret; } } else if ( - // First additional data. If fast RDRAND isn't available then a - // non-blocking OS entropy draw will be tried. + // First additional data, when other fork-safety measures have failed. If + // fast RDRAND isn't available then we use OS entropy. (!have_fast_rdrand() && !have_fork_detection() && - !sysrand(false, kAdditionalDataLength)) || - // Opportuntistic entropy draw in FIPS mode because RDRAND was used. - // In non-FIPS mode it's just drawn from |CRYPTO_sysrand| in a blocking - // way. - !sysrand(!kIsFIPS, CTR_DRBG_SEED_LEN) || - // Second entropy draw's additional data. + !sysrand(kAdditionalDataLength)) || + // OS entropy for the seed, or personalisation data if RDRAND was used. + !sysrand(CTR_DRBG_SEED_LEN) || + // Second additional data, when other fork-safety measures have failed. (!have_fast_rdrand() && !have_fork_detection() && - !sysrand(false, kAdditionalDataLength))) { + !sysrand(kAdditionalDataLength))) { return ret; } return ret; } -static void CheckInvariants(const std::vector<Event> &events) { - // If RDRAND is available then there should be no blocking syscalls in FIPS - // mode. -#if defined(BORINGSSL_FIPS) - if (have_rdrand()) { - for (const auto &event : events) { - switch (event.type) { - case Event::Syscall::kGetRandom: - if ((event.flags & GRND_NONBLOCK) == 0) { - ADD_FAILURE() << "Blocking getrandom found with RDRAND: " - << ToString(events); - } - break; - - default: - break; - } - } - } -#endif -} - // Tests that |TestFunctionPRNGModel| is a correct model for the code in // urandom.c, at least to the limits of the the |Event| type. TEST(URandomTest, Test) { @@ -793,7 +768,6 @@ TRACE_FLAG(SOCKET_READ_SHORT); const std::vector<Event> expected_trace = TestFunctionPRNGModel(flags); - CheckInvariants(expected_trace); std::vector<Event> actual_trace; GetTrace(&actual_trace, flags, TestFunction);
diff --git a/crypto/rand/windows.cc b/crypto/rand/windows.cc index 59df467..42cc981 100644 --- a/crypto/rand/windows.cc +++ b/crypto/rand/windows.cc
@@ -86,11 +86,6 @@ #endif // WINAPI_PARTITION_APP && !WINAPI_PARTITION_DESKTOP -int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len) { - CRYPTO_sysrand(buf, len); - return 1; -} - void CRYPTO_sysrand_for_seed(uint8_t *out, size_t requested) { CRYPTO_sysrand(out, requested); }