Only draw from RDRAND for additional_data if it's fast. We seek to incorporate entropy into every |RAND_bytes| call to avoid problems with fork() and VM cloning. However, on some chips, RDRAND is significantly slower than a system call thus crushing the performance of |RAND_bytes|. This change disables use of RDRAND for this opportunistic draw for non-Intel chips. BoringSSL will then fall back to either the OS, or nothing (if fork-unsafe mode has been set). RDRAND is still used for seeding the PRNG whenever it is available. This now adds a new blocking case: RDRAND may be used for seeding, but the syscall to get additional_data was still blocking. Previously that didn't matter because, if a syscall was used to get additional_data, then a blocking one had already been used to seed. Thus the syscall for additional_data is now non-blocking. Also, we had both |hwrand| and |rdrand| names hanging around. We don't support entropy instructions other than RDRAND, so unify around |rdrand| naming. If we ever do add support for more we can properly abstract at that time. Change-Id: I91121b270a2ebc667543dad1324f37285daad893 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40565 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/crypto/cpu-intel.c b/crypto/cpu-intel.c index cc41fc4..53ece95 100644 --- a/crypto/cpu-intel.c +++ b/crypto/cpu-intel.c
@@ -123,7 +123,9 @@ // and |out[1]|. See the comment in |OPENSSL_cpuid_setup| about this. static void handle_cpu_env(uint32_t *out, const char *in) { const int invert = in[0] == '~'; - const int hex = in[invert] == '0' && in[invert+1] == 'x'; + const int or = in[0] == '|'; + const int skip_first_byte = invert || or; + const int hex = in[skip_first_byte] == '0' && in[skip_first_byte+1] == 'x'; int sscanf_result; uint64_t v; @@ -140,6 +142,9 @@ if (invert) { out[0] &= ~v; out[1] &= ~(v >> 32); + } else if (or) { + out[0] |= v; + out[1] |= (v >> 32); } else { out[0] = v; out[1] = v >> 32; @@ -264,10 +269,14 @@ // OPENSSL_ia32cap can contain zero, one or two values, separated with a ':'. // Each value is a 64-bit, unsigned value which may start with "0x" to - // indicate a hex value. Prior to the 64-bit value, a '~' may be given. + // indicate a hex value. Prior to the 64-bit value, a '~' or '|' may be given. // - // If '~' isn't present, then the value is taken as the result of the CPUID. - // Otherwise the value is inverted and ANDed with the probed CPUID result. + // If the '~' prefix is present: + // the value is inverted and ANDed with the probed CPUID result + // If the '|' prefix is present: + // the value is ORed with the probed CPUID result + // Otherwise: + // the value is taken as the result of the CPUID // // The first value determines OPENSSL_ia32cap_P[0] and [1]. The second [2] // and [3].
diff --git a/crypto/fipsmodule/rand/internal.h b/crypto/fipsmodule/rand/internal.h index 280aae4..cf77bb3 100644 --- a/crypto/fipsmodule/rand/internal.h +++ b/crypto/fipsmodule/rand/internal.h
@@ -47,10 +47,16 @@ void CRYPTO_sysrand_for_seed(uint8_t *buf, size_t len); // CRYPTO_sysrand_if_available fills |len| bytes at |buf| with entropy from the -// operating system, if the entropy pool is initialized. If it is uninitialized, -// it will not block and will instead fill |buf| with all zeros or early -// /dev/urandom output. -void CRYPTO_sysrand_if_available(uint8_t *buf, size_t len); +// 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); +#else +OPENSSL_INLINE int CRYPTO_sysrand_if_available(uint8_t *buf, size_t len) { + CRYPTO_sysrand(buf, len); + return 1; +} #endif // rand_fork_unsafe_buffering_enabled returns whether fork-unsafe buffering has @@ -105,10 +111,19 @@ #if defined(OPENSSL_X86_64) && !defined(OPENSSL_NO_ASM) + OPENSSL_INLINE int have_rdrand(void) { return (OPENSSL_ia32cap_get()[1] & (1u << 30)) != 0; } +// have_fast_rdrand returns true if RDRAND is supported and it's reasonably +// fast. Concretely the latter is defined by whether the chip is Intel (fast) or +// not (assumed slow). +OPENSSL_INLINE int have_fast_rdrand(void) { + const uint32_t *const ia32cap = OPENSSL_ia32cap_get(); + return (ia32cap[1] & (1u << 30)) && (ia32cap[0] & (1u << 30)); +} + // CRYPTO_rdrand writes eight bytes of random data from the hardware RNG to // |out|. It returns one on success or zero on hardware failure. int CRYPTO_rdrand(uint8_t out[8]); @@ -117,6 +132,17 @@ // the hardware RNG. The |len| argument must be a multiple of eight. It returns // one on success and zero on hardware failure. int CRYPTO_rdrand_multiple8_buf(uint8_t *buf, size_t len); + +#else // OPENSSL_X86_64 && !OPENSSL_NO_ASM + +OPENSSL_INLINE int have_rdrand(void) { + return 0; +} + +OPENSSL_INLINE int have_fast_rdrand(void) { + return 0; +} + #endif // OPENSSL_X86_64 && !OPENSSL_NO_ASM
diff --git a/crypto/fipsmodule/rand/rand.c b/crypto/fipsmodule/rand/rand.c index 87d7b30..7bed579 100644 --- a/crypto/fipsmodule/rand/rand.c +++ b/crypto/fipsmodule/rand/rand.c
@@ -125,11 +125,9 @@ #if defined(OPENSSL_X86_64) && !defined(OPENSSL_NO_ASM) && \ !defined(BORINGSSL_UNSAFE_DETERMINISTIC_MODE) -static int hwrand(uint8_t *buf, const size_t len) { - if (!have_rdrand()) { - return 0; - } - +// rdrand should only be called if either |have_rdrand| or |have_fast_rdrand| +// returned true. +static int rdrand(uint8_t *buf, const size_t len) { const size_t len_multiple8 = len & ~7; if (!CRYPTO_rdrand_multiple8_buf(buf, len_multiple8)) { return 0; @@ -157,7 +155,7 @@ #else -static int hwrand(uint8_t *buf, size_t len) { +static int rdrand(uint8_t *buf, size_t len) { return 0; } @@ -168,7 +166,8 @@ static void rand_get_seed(struct rand_thread_state *state, uint8_t seed[CTR_DRBG_ENTROPY_LEN]) { if (!state->last_block_valid) { - if (!hwrand(state->last_block, sizeof(state->last_block))) { + if (!have_rdrand() || + !rdrand(state->last_block, sizeof(state->last_block))) { CRYPTO_sysrand_for_seed(state->last_block, sizeof(state->last_block)); } state->last_block_valid = 1; @@ -179,8 +178,8 @@ #define FIPS_OVERREAD 10 uint8_t entropy[CTR_DRBG_ENTROPY_LEN * FIPS_OVERREAD]; - int used_hwrand = hwrand(entropy, sizeof(entropy)); - if (!used_hwrand) { + int used_rdrand = have_rdrand() && rdrand(entropy, sizeof(entropy)); + if (!used_rdrand) { CRYPTO_sysrand_for_seed(entropy, sizeof(entropy)); } @@ -215,7 +214,7 @@ #if defined(OPENSSL_URANDOM) // If we used RDRAND, also opportunistically read from the system. This avoids // solely relying on the hardware once the entropy pool has been initialized. - if (used_hwrand) { + if (used_rdrand) { CRYPTO_sysrand_if_available(entropy, CTR_DRBG_ENTROPY_LEN); for (size_t i = 0; i < CTR_DRBG_ENTROPY_LEN; i++) { seed[i] ^= entropy[i]; @@ -246,15 +245,24 @@ // don't reseed with it so, from the point of view of FIPS, this doesn't // provide “prediction resistance”. But, in practice, it does. uint8_t additional_data[32]; - if (!hwrand(additional_data, sizeof(additional_data))) { + // Intel chips have fast RDRAND instructions while, in other cases, RDRAND can + // be _slower_ than a system call. + if (!have_fast_rdrand() || + !rdrand(additional_data, sizeof(additional_data))) { // Without a hardware RNG to save us from address-space duplication, the OS // entropy is used. This can be expensive (one read per |RAND_bytes| call) // and so can be disabled by applications that we have ensured don't fork // and aren't at risk of VM cloning. - if (!rand_fork_unsafe_buffering_enabled()) { - CRYPTO_sysrand(additional_data, sizeof(additional_data)); - } else { + if (rand_fork_unsafe_buffering_enabled()) { 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. + CRYPTO_sysrand(additional_data, sizeof(additional_data)); } }
diff --git a/crypto/fipsmodule/rand/urandom.c b/crypto/fipsmodule/rand/urandom.c index 6ca3400..c20340a 100644 --- a/crypto/fipsmodule/rand/urandom.c +++ b/crypto/fipsmodule/rand/urandom.c
@@ -431,16 +431,18 @@ #endif } -void CRYPTO_sysrand_if_available(uint8_t *out, size_t requested) { - // Return all zeros if |fill_with_entropy| fails. - OPENSSL_memset(out, 0, requested); +#endif // BORINGSSL_FIPS - if (!fill_with_entropy(out, requested, /*block=*/0, /*seed=*/0) && - errno != EAGAIN) { +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 // BORINGSSL_FIPS #endif // OPENSSL_URANDOM
diff --git a/crypto/fipsmodule/rand/urandom_test.cc b/crypto/fipsmodule/rand/urandom_test.cc index 38b0202..266a603 100644 --- a/crypto/fipsmodule/rand/urandom_test.cc +++ b/crypto/fipsmodule/rand/urandom_test.cc
@@ -32,10 +32,6 @@ #define PTRACE_O_EXITKILL (1 << 20) #endif -#if defined(OPENSSL_NO_ASM) -static int have_rdrand() { return 0; } -#endif - // This test can be run with $OPENSSL_ia32cap=~0x4000000000000000 in order to // simulate the absence of RDRAND of machines that have it. @@ -423,18 +419,49 @@ !sysrand(true, kAdditionalDataLength)) { return ret; } - } else { - // 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. - if (!sysrand(!is_fips, CTR_DRBG_ENTROPY_LEN)) { - return ret; - } + } else if ( + // First additional data. If fast RDRAND isn't available then a + // non-blocking OS entropy draw will be tried. + (!have_fast_rdrand() && !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(!is_fips, CTR_DRBG_ENTROPY_LEN) || + // Second entropy draw's additional data. + (!have_fast_rdrand() && !sysrand(false, 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; + + case Event::Syscall::kUrandomIoctl: + ADD_FAILURE() << "Urandom polling 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) { @@ -453,6 +480,7 @@ TRACE_FLAG(URANDOM_ERROR); 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_extra/deterministic.c b/crypto/rand_extra/deterministic.c index 34547ea..36774c8 100644 --- a/crypto/rand_extra/deterministic.c +++ b/crypto/rand_extra/deterministic.c
@@ -49,8 +49,9 @@ CRYPTO_sysrand(out, requested); } -void CRYPTO_sysrand_if_available(uint8_t *out, size_t requested) { +int CRYPTO_sysrand_if_available(uint8_t *out, size_t requested) { CRYPTO_sysrand(out, requested); + return 1; } #endif // BORINGSSL_UNSAFE_DETERMINISTIC_MODE
diff --git a/util/all_tests.json b/util/all_tests.json index c4c2787..a251e5c 100644 --- a/util/all_tests.json +++ b/util/all_tests.json
@@ -13,10 +13,21 @@ "cmd": ["crypto/urandom_test"] }, { + "comment": "Without RDRAND", "cmd": ["crypto/urandom_test"], "env": ["OPENSSL_ia32cap=~0x4000000000000000"] }, { + "comment": "Potentially with RDRAND, but not Intel", + "cmd": ["crypto/urandom_test"], + "env": ["OPENSSL_ia32cap=~0x0000000040000000"] + }, + { + "comment": "Potentially with RDRAND, and forced to Intel", + "cmd": ["crypto/urandom_test"], + "env": ["OPENSSL_ia32cap=|0x0000000040000000"] + }, + { "cmd": ["crypto/crypto_test", "--fork_unsafe_buffering", "--gtest_filter=RandTest.*:-RandTest.Fork"] }, {