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

@@ -36,6 +36,8 @@
 #include "../../ube/fork_detect.h"
 #include "getrandom_fillin.h"
 
+#include "../../test/test_util.h"
+
 #include <cstdlib>
 #include <unistd.h>
 #include <fcntl.h>
@@ -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();
 }

crypto/rand_extra/rand_test.cc

@@ -38,22 +38,14 @@
 #include <unistd.h>
 #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.
 
 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<uint8_t> 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);

crypto/test/test_util.cc

@@ -225,3 +225,10 @@ bool threadTest(const size_t numberOfThreads, std::function<void(bool*)> 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();
+  }
+}

crypto/test/test_util.h

@@ -29,6 +29,7 @@
 #include <openssl/span.h>
 
 #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<void(bool*)> testFunc);
 
+void maybeDisableSomeForkDetectMechanisms(void);
+
 // CustomData is for testing new structs that we add support for |ex_data|.
 typedef struct {
   int custom_data;

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 <openssl/target.h>
+
+// 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, <sys/mman.h> 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 <openssl/base.h>
-
 #include "fork_detect.h"
 
-#if defined(OPENSSL_LINUX)
-#include <sys/mman.h>
-#include <unistd.h>
-#include <stdlib.h>
+static int ignore_wipeonfork = 0;
+static int ignore_inheritzero = 0;
+
+#if defined(AWSLC_FORK_DETECTION_SUPPORTED)
 
+#include <openssl/base.h>
 #include <openssl/type_check.h>
 
 #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 <stdlib.h>
 
 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 <sys/mman.h>
+#include <unistd.h>
 
-  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 <sys/mman.h>
+#include <unistd.h>
+
+// 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;
+}

crypto/ube/fork_detect.h

@@ -17,6 +17,8 @@
 
 #include <openssl/base.h>
 
+#include <stdint.h>
+
 #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