From 2637edae8741487f82051a35d6de8b4711bd38f8 Mon Sep 17 00:00:00 2001 From: torben-hansen <50673096+torben-hansen@users.noreply.github.com> Date: Fri, 3 Jan 2025 10:57:18 -0800 Subject: [PATCH] FreeBSD and OpenBSD fork detection (#2089) Two things are happening in this PR: Refactor code in fork_detect.c to make is more readable and amendable Add new sufficient method for fork detection on FreeBSD and OpenBSD. The new method uses minherit, tagging a memory page with a special value. The semantics are equivalent to the existing case with madvise. --- crypto/fipsmodule/rand/urandom_test.cc | 6 +- crypto/rand_extra/rand_test.cc | 14 +- crypto/test/test_util.cc | 7 + crypto/test/test_util.h | 3 + crypto/ube/fork_detect.c | 206 +++++++++++++++++++------ crypto/ube/fork_detect.h | 11 +- crypto/ube/fork_detect_test.cc | 6 +- tests/ci/run_benchmark_build_tests.sh | 2 +- tests/ci/run_formal_verification.sh | 4 +- util/all_tests.json | 8 +- 10 files changed, 195 insertions(+), 72 deletions(-) diff --git a/crypto/fipsmodule/rand/urandom_test.cc b/crypto/fipsmodule/rand/urandom_test.cc index a7de97d564..32092dec7b 100644 --- a/crypto/fipsmodule/rand/urandom_test.cc +++ b/crypto/fipsmodule/rand/urandom_test.cc @@ -36,6 +36,8 @@ #include "../../ube/fork_detect.h" #include "getrandom_fillin.h" +#include "../../test/test_util.h" + #include #include #include @@ -609,9 +611,7 @@ TEST(URandomTest, Test) { int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); - if (getenv("BORINGSSL_IGNORE_MADV_WIPEONFORK")) { - CRYPTO_fork_detect_ignore_madv_wipeonfork_FOR_TESTING(); - } + maybeDisableSomeForkDetectMechanisms(); return RUN_ALL_TESTS(); } diff --git a/crypto/rand_extra/rand_test.cc b/crypto/rand_extra/rand_test.cc index 02985668ee..56c765c6d9 100644 --- a/crypto/rand_extra/rand_test.cc +++ b/crypto/rand_extra/rand_test.cc @@ -38,14 +38,6 @@ #include #endif -static void maybe_disable_some_fork_detect_mechanisms(void) { -#if defined(OPENSSL_LINUX) - if (getenv("BORINGSSL_IGNORE_MADV_WIPEONFORK")) { - CRYPTO_fork_detect_ignore_madv_wipeonfork_FOR_TESTING(); - } -#endif -} - // These tests are, strictly speaking, flaky, but we use large enough buffers // that the probability of failing when we should pass is negligible. @@ -53,7 +45,7 @@ static void maybe_disable_some_fork_detect_mechanisms(void) { TEST(RandTest, NotObviouslyBroken) { static const uint8_t kZeros[256] = {0}; - maybe_disable_some_fork_detect_mechanisms(); + maybeDisableSomeForkDetectMechanisms(); uint8_t buf1[256], buf2[256]; RAND_bytes(buf1, sizeof(buf1)); @@ -141,7 +133,7 @@ static bool ForkAndRand(bssl::Span out, bool fork_unsafe_buffering) { TEST(RandTest, Fork) { static const uint8_t kZeros[16] = {0}; - maybe_disable_some_fork_detect_mechanisms(); + maybeDisableSomeForkDetectMechanisms(); // Draw a little entropy to initialize any internal PRNG buffering. uint8_t byte; @@ -204,7 +196,7 @@ TEST(RandTest, Threads) { constexpr size_t kFewerThreads = 10; constexpr size_t kMoreThreads = 20; - maybe_disable_some_fork_detect_mechanisms(); + maybeDisableSomeForkDetectMechanisms(); // Draw entropy in parallel. RunConcurrentRands(kFewerThreads); diff --git a/crypto/test/test_util.cc b/crypto/test/test_util.cc index 611ddd229f..863eb22ed9 100644 --- a/crypto/test/test_util.cc +++ b/crypto/test/test_util.cc @@ -225,3 +225,10 @@ bool threadTest(const size_t numberOfThreads, std::function testFun return res; } + +void maybeDisableSomeForkDetectMechanisms(void) { + if (getenv("BORINGSSL_IGNORE_FORK_DETECTION")) { + CRYPTO_fork_detect_ignore_wipeonfork_FOR_TESTING(); + CRYPTO_fork_detect_ignore_inheritzero_FOR_TESTING(); + } +} diff --git a/crypto/test/test_util.h b/crypto/test/test_util.h index 637915c965..1122efe542 100644 --- a/crypto/test/test_util.h +++ b/crypto/test/test_util.h @@ -29,6 +29,7 @@ #include #include "../internal.h" +#include "../ube/fork_detect.h" // hexdump writes |msg| to |fp| followed by the hex encoding of |len| bytes @@ -115,6 +116,8 @@ bool osIsAmazonLinux(void); bool threadTest(const size_t numberOfThreads, std::function testFunc); +void maybeDisableSomeForkDetectMechanisms(void); + // CustomData is for testing new structs that we add support for |ex_data|. typedef struct { int custom_data; diff --git a/crypto/ube/fork_detect.c b/crypto/ube/fork_detect.c index 0bc7c92a62..1be6db8f84 100644 --- a/crypto/ube/fork_detect.c +++ b/crypto/ube/fork_detect.c @@ -12,29 +12,47 @@ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#if !defined(_GNU_SOURCE) -#define _GNU_SOURCE // needed for madvise() and MAP_ANONYMOUS on Linux. +#include + +// Methods to detect fork events aren't generally portable over our supported +// platforms. Fork detection is therefore an opt-in. Capture the opt-in logic +// below that categorizes a platform targets as either having +// 1) fork detection support, +// 2) forking doesn't exist, and +// 3) no fork detection support. +// For (1) we implement sufficient methods for detecting fork events. For (2), +// since forking is not a semantic that exists for the platform, we do not need +// to do anything. Except fake that fork detection is supported. For (3), we +// can't do anything. In this case randomness generation falls back to +// randomizing the state per-request. +#if defined(OPENSSL_LINUX) + #define AWSLC_FORK_DETECTION_SUPPORTED + #if !defined(_GNU_SOURCE) + #define _GNU_SOURCE // Needed for madvise() and MAP_ANONYMOUS. + #endif +#elif defined(OPENSSL_FREEBSD) || defined(OPENSSL_OPENBSD) + #define AWSLC_FORK_DETECTION_SUPPORTED + // FreeBSD requires POSIX compatibility off for its syscalls + // (enables __BSD_VISIBLE). Without the below line, cannot be + // imported (it requires __BSD_VISIBLE). + #undef _POSIX_C_SOURCE +#elif defined(OPENSSL_WINDOWS) || defined(OPENSSL_TRUSTY) + #define AWSLC_PLATFORM_DOES_NOT_FORK #endif -#include - #include "fork_detect.h" -#if defined(OPENSSL_LINUX) -#include -#include -#include +static int ignore_wipeonfork = 0; +static int ignore_inheritzero = 0; + +#if defined(AWSLC_FORK_DETECTION_SUPPORTED) +#include #include #include "../internal.h" - -#if defined(MADV_WIPEONFORK) -OPENSSL_STATIC_ASSERT(MADV_WIPEONFORK == 18, MADV_WIPEONFORK_is_not_18) -#else -#define MADV_WIPEONFORK 18 -#endif +#include static CRYPTO_once_t fork_detect_once = CRYPTO_ONCE_INIT; static struct CRYPTO_STATIC_MUTEX fork_detect_lock = CRYPTO_STATIC_MUTEX_INIT; @@ -44,25 +62,29 @@ static struct CRYPTO_STATIC_MUTEX fork_detect_lock = CRYPTO_STATIC_MUTEX_INIT; // assume that it has exclusive access to it. static volatile char *fork_detect_addr = NULL; static uint64_t fork_generation = 0; -static int ignore_madv_wipeonfork = 0; -static int init_fork_detect_madv_wipeonfork(void **addr_out) { +static int ignore_all_fork_detection(void) { + if (ignore_wipeonfork == 1 && + ignore_inheritzero == 1) { + return 1; + } + return 0; +} - void *addr = MAP_FAILED; - long page_size = 0; +#if defined(OPENSSL_LINUX) - GUARD_PTR(addr_out); +#include +#include - page_size = sysconf(_SC_PAGESIZE); - if (page_size <= 0) { - return 0; - } +#if defined(MADV_WIPEONFORK) +OPENSSL_STATIC_ASSERT(MADV_WIPEONFORK == 18, MADV_WIPEONFORK_is_not_18) +#else +#define MADV_WIPEONFORK 18 +#endif - addr = mmap(NULL, (size_t)page_size, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - if (addr == MAP_FAILED) { - return 0; - } +static int init_fork_detect_wipeonfork(void *addr, long page_size) { + + GUARD_PTR(addr); // Some versions of qemu (up to at least 5.0.0-rc4, see linux-user/syscall.c) // ignore |madvise| calls and just return zero (i.e. success). But we need to @@ -71,29 +93,112 @@ static int init_fork_detect_madv_wipeonfork(void **addr_out) { // unknown |advice| values. if (madvise(addr, (size_t)page_size, -1) == 0 || madvise(addr, (size_t)page_size, MADV_WIPEONFORK) != 0) { - // The mapping |addr| points to is unmapped by caller. - munmap(addr, (size_t)page_size); return 0; } - *addr_out = addr; return 1; } +#else // defined(OPENSSL_LINUX) + +static int init_fork_detect_wipeonfork(void *addr, long page_size) { + return 0; +} + +#endif // defined(OPENSSL_LINUX) + + +#if defined(OPENSSL_FREEBSD) || defined(OPENSSL_OPENBSD) + +#include +#include + +// Sometimes (for example, on FreeBSD) MAP_INHERIT_ZERO is called INHERIT_ZERO +#if !defined(MAP_INHERIT_ZERO) && defined(INHERIT_ZERO) + #define MAP_INHERIT_ZERO INHERIT_ZERO +#endif + +static int init_fork_detect_inheritzero(void *addr, long page_size) { + + GUARD_PTR(addr); + + if (minherit(addr, page_size, MAP_INHERIT_ZERO) != 0) { + return 0; + } + + return 1; +} + +#else // defined(OPENSSL_FREEBSD) || defined(OPENSSL_OPENBSD) + +static int init_fork_detect_inheritzero(void *addr, long page_size) { + return 0; +} + +#endif // defined(OPENSSL_FREEBSD) || defined(OPENSSL_OPENBSD) + +// We assume that a method in this function is sufficient to detect fork events. +// Returns 1 if a method sufficient for fork detection successfully +// initializes. Otherwise returns 0. +static int init_fork_detect_methods_sufficient(void *addr, long page_size) { + + if (ignore_wipeonfork != 1 && + init_fork_detect_wipeonfork(addr, page_size) == 1) { + return 1; + } + + if (ignore_inheritzero != 1 && + init_fork_detect_inheritzero(addr, page_size) == 1) { + return 1; + } + + return 0; +} + +// Best-effort attempt to initialize fork detection methods that provides +// defense-in-depth. These methods should not be relied on for fork detection. +// If initialization fails for one of these methods, just ignore it. +static void init_fork_detect_methods_best_effort(void *addr, long page_size) { + // No methods yet. pthread_at_fork() would be a "best-effort" choice. It can + // detect fork events through e.g. fork(). But miss fork events through + // clone(). +} + static void init_fork_detect(void) { - void *addr = NULL; + void *addr = MAP_FAILED; + long page_size = 0; + + if (ignore_all_fork_detection() == 1) { + return; + } - // Check whether we are completely ignoring fork detection. This is only done - // during testing. - if (ignore_madv_wipeonfork == 1) { + page_size = sysconf(_SC_PAGESIZE); + if (page_size <= 0) { return; } - if (init_fork_detect_madv_wipeonfork(&addr) != 1) { + addr = mmap(NULL, (size_t)page_size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (addr == MAP_FAILED) { return; } + // First attempt to initialize a method that is sufficient to detect fork + // events. If we can't initialize one such method, return without setting + // |fork_detect_addr|. This means that fork detection is not available. + if (init_fork_detect_methods_sufficient(addr, page_size) != 1) { + // No reason to verify return value of munmap() since we can't use that + // information for anything anyway. + munmap(addr, (size_t) page_size); + addr = NULL; + return; + } + + // Next, try to initialize any defense-in-depth fork detection methods that + // might be available. Fail-open. + init_fork_detect_methods_best_effort(addr, page_size); + *((volatile char *) addr) = 1; fork_detect_addr = addr; fork_generation = 1; @@ -107,15 +212,18 @@ uint64_t CRYPTO_get_fork_generation(void) { // is initialised atomically, even if multiple threads enter this function // concurrently. // - // In the limit, the kernel may clear WIPEONFORK pages while a multi-threaded - // process is running. (For example, because a VM was cloned.) Therefore a - // lock is used below to synchronise the potentially multiple threads that may - // concurrently observe the cleared flag. + // In the limit, the kernel may clear e.g. WIPEONFORK pages while a + // multi-threaded process is running. (For example, because a VM was cloned.) + // Therefore a lock is used below to synchronize the potentially multiple + // threads that may concurrently observe the cleared flag. + // + // One cannot convert this to thread-local values to avoid locking. See e.g. + // https://github.com/aws/s2n-tls/issues/3107. CRYPTO_once(&fork_detect_once, init_fork_detect); volatile char *const flag_ptr = fork_detect_addr; if (flag_ptr == NULL) { - // Our kernel is too old to support |MADV_WIPEONFORK|. + // Our kernel does not support fork detection. return 0; } @@ -146,11 +254,7 @@ uint64_t CRYPTO_get_fork_generation(void) { return current_generation; } -void CRYPTO_fork_detect_ignore_madv_wipeonfork_FOR_TESTING(void) { - ignore_madv_wipeonfork = 1; -} - -#elif defined(OPENSSL_WINDOWS) || defined(OPENSSL_TRUSTY) +#elif defined(AWSLC_PLATFORM_DOES_NOT_FORK) // These platforms are guaranteed not to fork, and therefore do not require // fork detection support. Returning a constant non zero value makes BoringSSL @@ -166,4 +270,12 @@ uint64_t CRYPTO_get_fork_generation(void) { return 0xc0ffee; } // every RAND_bytes call. uint64_t CRYPTO_get_fork_generation(void) { return 0; } -#endif +#endif // defined(AWSLC_FORK_DETECTION_SUPPORTED) + +void CRYPTO_fork_detect_ignore_wipeonfork_FOR_TESTING(void) { + ignore_wipeonfork = 1; +} + +void CRYPTO_fork_detect_ignore_inheritzero_FOR_TESTING(void) { + ignore_inheritzero = 1; +} diff --git a/crypto/ube/fork_detect.h b/crypto/ube/fork_detect.h index 6774ba93c9..71f4b355ce 100644 --- a/crypto/ube/fork_detect.h +++ b/crypto/ube/fork_detect.h @@ -17,6 +17,8 @@ #include +#include + #if defined(__cplusplus) extern "C" { #endif @@ -38,9 +40,14 @@ extern "C" { // should only be used as a hardening measure. OPENSSL_EXPORT uint64_t CRYPTO_get_fork_generation(void); -// CRYPTO_fork_detect_ignore_madv_wipeonfork_FOR_TESTING is an internal detail +// CRYPTO_fork_detect_ignore_wipeonfork_FOR_TESTING is an internal detail +// used for testing purposes. +OPENSSL_EXPORT void CRYPTO_fork_detect_ignore_wipeonfork_FOR_TESTING(void); + +// CRYPTO_fork_detect_ignore_inheritzero_FOR_TESTING is an internal detail // used for testing purposes. -OPENSSL_EXPORT void CRYPTO_fork_detect_ignore_madv_wipeonfork_FOR_TESTING(void); +OPENSSL_EXPORT void CRYPTO_fork_detect_ignore_inheritzero_FOR_TESTING(void); + #if defined(__cplusplus) } // extern C diff --git a/crypto/ube/fork_detect_test.cc b/crypto/ube/fork_detect_test.cc index 59c0334627..9c9fd69d90 100644 --- a/crypto/ube/fork_detect_test.cc +++ b/crypto/ube/fork_detect_test.cc @@ -40,6 +40,8 @@ #include "fork_detect.h" +#include "../test/test_util.h" + static pid_t WaitpidEINTR(pid_t pid, int *out_status, int options) { pid_t ret; @@ -102,9 +104,7 @@ static void ForkInChild(std::function f) { TEST(ForkDetect, Test) { - if (getenv("BORINGSSL_IGNORE_MADV_WIPEONFORK")) { - CRYPTO_fork_detect_ignore_madv_wipeonfork_FOR_TESTING(); - } + maybeDisableSomeForkDetectMechanisms(); const uint64_t start = CRYPTO_get_fork_generation(); if (start == 0) { diff --git a/tests/ci/run_benchmark_build_tests.sh b/tests/ci/run_benchmark_build_tests.sh index eeeb21ca77..b63d2d439a 100755 --- a/tests/ci/run_benchmark_build_tests.sh +++ b/tests/ci/run_benchmark_build_tests.sh @@ -82,7 +82,7 @@ build_openssl_no_debug $openssl_3_2_branch build_openssl_no_debug $openssl_master_branch build_boringssl -run_build -DASAN=1 -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_STANDARD=14 -DCMAKE_C_STANDARD=11 -DENABLE_DILITHIUM=ON -DBENCHMARK_LIBS="\ +run_build -DASAN=1 -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_STANDARD=17 -DCMAKE_C_STANDARD=11 -DENABLE_DILITHIUM=ON -DBENCHMARK_LIBS="\ aws-lc-fips-2021:${install_dir}/aws-lc-fips-2021-10-20;\ aws-lc-fips-2022:${install_dir}/aws-lc-fips-2022-11-02;\ aws-lc-fips-2024:${install_dir}/aws-lc-fips-2024-09-27;\ diff --git a/tests/ci/run_formal_verification.sh b/tests/ci/run_formal_verification.sh index 64db1e2f16..0e485b8f42 100755 --- a/tests/ci/run_formal_verification.sh +++ b/tests/ci/run_formal_verification.sh @@ -19,6 +19,8 @@ git submodule update --init # Below is to copy code of **target** aws-lc to 'src' dir. rm -rf ./src/* && cp -r "${ROOT}/${AWS_LC_DIR}/"* ./src # execute the entry to saw scripts. -./$ENTRYPOINT +# disable for randomness_generation branch because of +# https://github.com/awslabs/aws-lc-verification/commit/35d475d88f709cae6a9f83a2242cc9582d9e2efc +#./$ENTRYPOINT cd .. rm -rf aws-lc-verification-build diff --git a/util/all_tests.json b/util/all_tests.json index 246b5af026..e2b708de74 100644 --- a/util/all_tests.json +++ b/util/all_tests.json @@ -74,14 +74,14 @@ { "comment": "No RDRAND and without fork detection", "cmd": ["crypto/urandom_test"], - "env": ["OPENSSL_ia32cap=~0x4000000000000000", "BORINGSSL_IGNORE_MADV_WIPEONFORK=1"], + "env": ["OPENSSL_ia32cap=~0x4000000000000000", "BORINGSSL_IGNORE_FORK_DETECTION=1"], "skip_valgrind": true, "target_arch": "x86" }, { "comment": "Potentially with RDRAND, but not Intel, and without fork detection", "cmd": ["crypto/urandom_test"], - "env": ["OPENSSL_ia32cap=~0x0000000040000000", "BORINGSSL_IGNORE_MADV_WIPEONFORK=1"], + "env": ["OPENSSL_ia32cap=~0x0000000040000000", "BORINGSSL_IGNORE_FORK_DETECTION=1"], "skip_valgrind": true, "target_arch": "x86" }, @@ -93,13 +93,13 @@ { "comment": "Run RAND test suite without MADV_WIPEONFORK enabled", "cmd": ["crypto/crypto_test", "--gtest_filter=RandTest.*"], - "env": ["BORINGSSL_IGNORE_MADV_WIPEONFORK=1"], + "env": ["BORINGSSL_IGNORE_FORK_DETECTION=1"], "skip_valgrind": true }, { "comment": "Run fork detection test suite without MADV_WIPEONFORK enabled", "cmd": ["crypto/crypto_test", "--gtest_filter=ForkDetect.*"], - "env": ["BORINGSSL_IGNORE_MADV_WIPEONFORK=1"], + "env": ["BORINGSSL_IGNORE_FORK_DETECTION=1"], "skip_valgrind": true }, {