From dd5219451c3ce26221762a15d867edf43b463bb2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 8 Jan 2023 18:49:42 -0800 Subject: [PATCH] Fix RAND_enable_fork_unsafe_buffering when called after fork If a process calls fork(), then the child process never forks again, the child may wish to call RAND_enable_fork_unsafe_buffering(). However, doing so exposes a bug: we assume that, if the flag is set, we don't need to worry about fork-safety. But it is possible that the PRNG state was cloned from another process which does not work. Concretely, consider a zygote process, e.g. Chromium's. A zygote process would retain fork-safety, but pass along its PRNG state to each of its children. If the children never fork, they might disable fork-safety, hitting this bug. (Chromium does not call this API. This is just a hypothetical scenario.) Fix this by reseeding whenever the fork-safety bit changes. This fix does not strictly depend on the atomics work, but it causes us to unconditionally sample rand_fork_unsafe_buffering_enabled(). This no longer causes contention because it's just another atomic load. This only affects systems without MADV_WIPEONFORK and without fast RDRAND. If RDRAND is fast, we're always fork-safe and MADV_WIPEONFORK allows us to efficiently detect forks. Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan Change-Id: I6d0c471c62c951254faf85420a7dc3f4a9d65ee0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59850 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/fipsmodule/rand/rand.c | 17 ++++++++++++++-- crypto/rand_extra/rand_test.cc | 36 ++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/crypto/fipsmodule/rand/rand.c b/crypto/fipsmodule/rand/rand.c index bf6b0469f..a3fc6880d 100644 --- a/crypto/fipsmodule/rand/rand.c +++ b/crypto/fipsmodule/rand/rand.c @@ -65,6 +65,9 @@ struct rand_thread_state { // last_block_valid is non-zero iff |last_block| contains data from // |get_seed_entropy|. int last_block_valid; + // fork_unsafe_buffering is non-zero iff, when |drbg| was last (re)seeded, + // fork-unsafe buffering was enabled. + int fork_unsafe_buffering; #if defined(BORINGSSL_FIPS) // last_block contains the previous block from |get_seed_entropy|. @@ -331,6 +334,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, } const uint64_t fork_generation = CRYPTO_get_fork_generation(); + const int fork_unsafe_buffering = rand_fork_unsafe_buffering_enabled(); // Additional data is mixed into every CTR-DRBG call to protect, as best we // can, against forks & VM clones. We do not over-read this information and @@ -345,7 +349,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, // entropy is used. This can be expensive (one read per |RAND_bytes| call) // and so is disabled when we have fork detection, or if the application has // promised not to fork. - if (fork_generation != 0 || rand_fork_unsafe_buffering_enabled()) { + 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. @@ -388,6 +392,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, } state->calls = 0; state->fork_generation = fork_generation; + state->fork_unsafe_buffering = fork_unsafe_buffering; #if defined(BORINGSSL_FIPS) CRYPTO_MUTEX_init(&state->clear_drbg_lock); @@ -406,7 +411,14 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, } if (state->calls >= kReseedInterval || - state->fork_generation != fork_generation) { + // If we've forked since |state| was last seeded, reseed. + state->fork_generation != fork_generation || + // If |state| was seeded from a state with different fork-safety + // preferences, reseed. Suppose |state| was fork-safe, then forked into + // two children, but each of the children never fork and disable fork + // safety. The children must reseed to avoid working from the same PRNG + // state. + state->fork_unsafe_buffering != fork_unsafe_buffering) { uint8_t seed[CTR_DRBG_ENTROPY_LEN]; uint8_t reseed_additional_data[CTR_DRBG_ENTROPY_LEN] = {0}; size_t reseed_additional_data_len = 0; @@ -424,6 +436,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, } state->calls = 0; state->fork_generation = fork_generation; + state->fork_unsafe_buffering = fork_unsafe_buffering; } else { #if defined(BORINGSSL_FIPS) CRYPTO_MUTEX_lock_read(&state->clear_drbg_lock); diff --git a/crypto/rand_extra/rand_test.cc b/crypto/rand_extra/rand_test.cc index 2ed1deb41..1af28b04a 100644 --- a/crypto/rand_extra/rand_test.cc +++ b/crypto/rand_extra/rand_test.cc @@ -56,7 +56,7 @@ TEST(RandTest, NotObviouslyBroken) { #if !defined(OPENSSL_WINDOWS) && !defined(OPENSSL_IOS) && \ !defined(OPENSSL_FUCHSIA) && !defined(BORINGSSL_UNSAFE_DETERMINISTIC_MODE) -static bool ForkAndRand(bssl::Span out) { +static bool ForkAndRand(bssl::Span out, bool fork_unsafe_buffering) { int pipefds[2]; if (pipe(pipefds) < 0) { perror("pipe"); @@ -76,6 +76,9 @@ static bool ForkAndRand(bssl::Span out) { if (child == 0) { // This is the child. Generate entropy and write it to the parent. close(pipefds[0]); + if (fork_unsafe_buffering) { + RAND_enable_fork_unsafe_buffering(-1); + } RAND_bytes(out.data(), out.size()); while (!out.empty()) { ssize_t ret = write(pipefds[1], out.data(), out.size()); @@ -136,18 +139,27 @@ TEST(RandTest, Fork) { // intentionally uses smaller buffers than the others, to minimize the chance // of sneaking by with a large enough buffer that we've since reseeded from // the OS. - uint8_t buf1[16], buf2[16], buf3[16]; - ASSERT_TRUE(ForkAndRand(buf1)); - ASSERT_TRUE(ForkAndRand(buf2)); - RAND_bytes(buf3, sizeof(buf3)); + // + // All child processes should have different PRNGs, including the ones that + // disavow fork-safety. Although they are produced by fork, they themselves do + // not fork after that call. + uint8_t bufs[5][16]; + ASSERT_TRUE(ForkAndRand(bufs[0], /*fork_unsafe_buffering=*/false)); + ASSERT_TRUE(ForkAndRand(bufs[1], /*fork_unsafe_buffering=*/false)); + ASSERT_TRUE(ForkAndRand(bufs[2], /*fork_unsafe_buffering=*/true)); + ASSERT_TRUE(ForkAndRand(bufs[3], /*fork_unsafe_buffering=*/true)); + RAND_bytes(bufs[4], sizeof(bufs[4])); - // All should be different. - EXPECT_NE(Bytes(buf1), Bytes(buf2)); - EXPECT_NE(Bytes(buf2), Bytes(buf3)); - EXPECT_NE(Bytes(buf1), Bytes(buf3)); - EXPECT_NE(Bytes(buf1), Bytes(kZeros)); - EXPECT_NE(Bytes(buf2), Bytes(kZeros)); - EXPECT_NE(Bytes(buf3), Bytes(kZeros)); + // All should be different and non-zero. + for (const auto &buf : bufs) { + EXPECT_NE(Bytes(buf), Bytes(kZeros)); + } + for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(bufs); i++) { + for (size_t j = 0; j < i; j++) { + EXPECT_NE(Bytes(bufs[i]), Bytes(bufs[j])) + << "buffers " << i << " and " << j << " matched"; + } + } } #endif // !OPENSSL_WINDOWS && !OPENSSL_IOS && // !OPENSSL_FUCHSIA && !BORINGSSL_UNSAFE_DETERMINISTIC_MODE