Add support for fork detection via pthread_atfork()

This provides an alternative way for fork detection that can be used if we are certain the platform does not have other ways of cloning an address space in use that are not noticed by pthread_atfork. This supports using pthread_atfork for fork detection on macOS, iOS, OpenBSD and FreeBSD. Linux continues to use MADVISE for the generation number and fork detection. Change-Id: I79fd7769477dc90bfe37229d2ff2e8c16898dff7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59906 Reviewed-by: David Benjamin <davidben@google.com> Auto-Submit: Bob Beck <bbe@google.com> Commit-Queue: Bob Beck <bbe@google.com>
google · Oct 20, 2023 · bfa8369 · bfa8369
1 parent 39d7ee9
commit bfa8369
Show file tree

Hide file tree

Showing 5 changed files with 112 additions and 33 deletions.
diff --git a/crypto/fipsmodule/rand/fork_detect.c b/crypto/fipsmodule/rand/fork_detect.c
@@ -17,31 +17,34 @@
 #endif
 
 #include <openssl/base.h>
-
 #include "fork_detect.h"
 
-#if defined(OPENSSL_LINUX)
-#include <assert.h>
-#include <sys/mman.h>
+#if defined(OPENSSL_FORK_DETECTION_MADVISE)
 #include <unistd.h>
 #include <stdlib.h>
-
-#include "../delocate.h"
-#include "../../internal.h"
-
-
+#include <assert.h>
+#include <sys/mman.h>
 #if defined(MADV_WIPEONFORK)
 static_assert(MADV_WIPEONFORK == 18, "MADV_WIPEONFORK is not 18");
 #else
 #define MADV_WIPEONFORK 18
 #endif
+#elif defined(OPENSSL_FORK_DETECTION_PTHREAD_ATFORK)
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+#endif // OPENSSL_FORK_DETECTION_MADVISE
 
+#include "../delocate.h"
+#include "../../internal.h"
+
+#if defined(OPENSSL_FORK_DETECTION_MADVISE)
+DEFINE_BSS_GET(int, g_force_madv_wipeonfork);
+DEFINE_BSS_GET(int, g_force_madv_wipeonfork_enabled);
 DEFINE_STATIC_ONCE(g_fork_detect_once);
 DEFINE_STATIC_MUTEX(g_fork_detect_lock);
 DEFINE_BSS_GET(CRYPTO_atomic_u32 *, g_fork_detect_addr);
 DEFINE_BSS_GET(uint64_t, g_fork_generation);
-DEFINE_BSS_GET(int, g_force_madv_wipeonfork);
-DEFINE_BSS_GET(int, g_force_madv_wipeonfork_enabled);
 
 static void init_fork_detect(void) {
   if (*g_force_madv_wipeonfork_bss_get()) {
@@ -73,9 +76,12 @@ static void init_fork_detect(void) {
   CRYPTO_atomic_store_u32(addr, 1);
   *g_fork_detect_addr_bss_get() = addr;
   *g_fork_generation_bss_get() = 1;
+
 }
 
 uint64_t CRYPTO_get_fork_generation(void) {
+  CRYPTO_once(g_fork_detect_once_bss_get(), init_fork_detect);
+
   // In a single-threaded process, there are obviously no races because there's
   // only a single mutator in the address space.
   //
@@ -87,7 +93,6 @@ uint64_t CRYPTO_get_fork_generation(void) {
   // child process is single-threaded, the child may become multi-threaded
   // before it observes this. Therefore, we must synchronize the logic below.
 
-  CRYPTO_once(g_fork_detect_once_bss_get(), init_fork_detect);
   CRYPTO_atomic_u32 *const flag_ptr = *g_fork_detect_addr_bss_get();
   if (flag_ptr == NULL) {
     // Our kernel is too old to support |MADV_WIPEONFORK| or
@@ -98,6 +103,12 @@ uint64_t CRYPTO_get_fork_generation(void) {
       // doesn't support it.
       return 42;
     }
+    // With Linux and clone(), we do not believe that pthread_atfork() is
+    // sufficient for detecting all forms of address space duplication. At this
+    // point we have a kernel that does not support MADV_WIPEONFORK. We could
+    // return the generation number from pthread_atfork() here and it would
+    // probably be safe in almost any situation, but to ensure safety we return
+    // 0 and force an entropy draw on every call.
     return 0;
   }
 
@@ -140,7 +151,34 @@ void CRYPTO_fork_detect_force_madv_wipeonfork_for_testing(int on) {
   *g_force_madv_wipeonfork_enabled_bss_get() = on;
 }
 
-#elif defined(OPENSSL_WINDOWS) || defined(OPENSSL_TRUSTY)
+#elif defined(OPENSSL_FORK_DETECTION_PTHREAD_ATFORK)
+
+DEFINE_STATIC_ONCE(g_pthread_fork_detection_once);
+DEFINE_BSS_GET(uint64_t, g_atfork_fork_generation);
+
+static void we_are_forked(void) {
+  // Immediately after a fork, the process must be single-threaded.
+  uint64_t value = *g_atfork_fork_generation_bss_get() + 1;
+  if (value == 0) {
+    value = 1;
+  }
+  *g_atfork_fork_generation_bss_get() = value;
+}
+
+static void init_pthread_fork_detection(void) {
+  if (pthread_atfork(NULL, NULL, we_are_forked) != 0) {
+    abort();
+  }
+  *g_atfork_fork_generation_bss_get() = 1;
+}
+
+uint64_t CRYPTO_get_fork_generation(void) {
+  CRYPTO_once(g_pthread_fork_detection_once_bss_get(), init_pthread_fork_detection);
+
+  return *g_atfork_fork_generation_bss_get();
+}
+
+#elif defined(OPENSSL_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

diff --git a/crypto/fipsmodule/rand/fork_detect.h b/crypto/fipsmodule/rand/fork_detect.h
@@ -17,6 +17,23 @@
 
 #include <openssl/base.h>
 
+#if defined(OPENSSL_LINUX)
+// On linux we use MADVISE instead of pthread_atfork(), due
+// to concerns about clone() being used for address space
+// duplication.
+#define OPENSSL_FORK_DETECTION
+#define OPENSSL_FORK_DETECTION_MADVISE
+#elif defined(OPENSSL_MACOS) || defined(OPENSSL_IOS) || \
+    defined(OPENSSL_OPENBSD) || defined(OPENSSL_FREEBSD)
+// These platforms may detect address space duplication with pthread_atfork.
+// iOS doesn't normally allow fork in apps, but it's there.
+#define OPENSSL_FORK_DETECTION
+#define OPENSSL_FORK_DETECTION_PTHREAD_ATFORK
+#elif defined(OPENSSL_WINDOWS) || defined(OPENSSL_TRUSTY)
+// These platforms do not fork.
+#define OPENSSL_DOES_NOT_FORK
+#endif
+
 #if defined(__cplusplus)
 extern "C" {
 #endif

diff --git a/crypto/fipsmodule/rand/fork_detect_test.cc b/crypto/fipsmodule/rand/fork_detect_test.cc
@@ -14,9 +14,11 @@
 
 #include <openssl/base.h>
 
+#include "fork_detect.h"
+
 // TSAN cannot cope with this test and complains that "starting new threads
 // after multi-threaded fork is not supported".
-#if defined(OPENSSL_LINUX) && !defined(OPENSSL_TSAN)
+#if defined(OPENSSL_FORK_DETECTION) && !defined(OPENSSL_TSAN)
 #include <errno.h>
 #include <inttypes.h>
 #include <stdio.h>
@@ -32,8 +34,6 @@
 
 #include <gtest/gtest.h>
 
-#include "fork_detect.h"
-
 
 static pid_t WaitpidEINTR(pid_t pid, int *out_status, int options) {
   pid_t ret;
@@ -47,19 +47,20 @@ static pid_t WaitpidEINTR(pid_t pid, int *out_status, int options) {
 // The *InChild functions run inside a child process and must report errors via
 // |stderr| and |_exit| rather than GTest.
 
-static void CheckGenerationInChild(const char *name, uint64_t expected) {
+static void CheckGenerationAtLeastInChild(const char *name,
+                                   uint64_t minimum_expected) {
   uint64_t generation = CRYPTO_get_fork_generation();
-  if (generation != expected) {
+  if (generation < minimum_expected) {
     fprintf(stderr, "%s generation (#1) was %" PRIu64 ", wanted %" PRIu64 ".\n",
-            name, generation, expected);
+            name, generation, minimum_expected);
     _exit(1);
   }
 
   // The generation should be stable.
-  generation = CRYPTO_get_fork_generation();
-  if (generation != expected) {
+  uint64_t new_generation = CRYPTO_get_fork_generation();
+  if (new_generation != generation) {
     fprintf(stderr, "%s generation (#2) was %" PRIu64 ", wanted %" PRIu64 ".\n",
-            name, generation, expected);
+            name, new_generation, generation);
     _exit(1);
   }
 }
@@ -95,10 +96,9 @@ static void ForkInChild(std::function<void()> f) {
 }
 
 TEST(ForkDetect, Test) {
-  const uint64_t start = CRYPTO_get_fork_generation();
+  uint64_t start = CRYPTO_get_fork_generation();
   if (start == 0) {
-    fprintf(stderr, "Fork detection not supported. Skipping test.\n");
-    return;
+    GTEST_SKIP() << "Fork detection not supported. Skipping test.\n";
   }
 
   // The fork generation should be stable.
@@ -111,16 +111,22 @@ TEST(ForkDetect, Test) {
     // Fork grandchildren before observing the fork generation. The
     // grandchildren will observe |start| + 1.
     for (int i = 0; i < 2; i++) {
-      ForkInChild([&] { CheckGenerationInChild("Grandchild", start + 1); });
+      ForkInChild(
+          [&] { CheckGenerationAtLeastInChild("Grandchild", start + 1); });
     }
 
     // Now the child also observes |start| + 1. This is fine because it has
     // already diverged from the grandchild at this point.
-    CheckGenerationInChild("Child", start + 1);
 
+    CheckGenerationAtLeastInChild("Child", start + 1);
+
+    // In the pthread_atfork the value may have changed.
+    uint64_t child_generation = CRYPTO_get_fork_generation();
     // Forked grandchildren will now observe |start| + 2.
     for (int i = 0; i < 2; i++) {
-      ForkInChild([&] { CheckGenerationInChild("Grandchild", start + 2); });
+      ForkInChild([&] {
+        CheckGenerationAtLeastInChild("Grandchild", child_generation + 1);
+      });
     }
 
 #if defined(OPENSSL_THREADS)
@@ -131,8 +137,10 @@ TEST(ForkDetect, Test) {
       std::vector<std::thread> threads(4);
       for (int i = 0; i < 2; i++) {
         for (auto &t : threads) {
-          t = std::thread(
-              [&] { CheckGenerationInChild("Grandchild thread", start + 2); });
+          t = std::thread([&] {
+            CheckGenerationAtLeastInChild("Grandchild thread",
+                                          child_generation + 1);
+          });
         }
         for (auto &t : threads) {
           t.join();
@@ -141,8 +149,15 @@ TEST(ForkDetect, Test) {
     });
 #endif  // OPENSSL_THREADS
 
-    // The child still observes |start| + 1.
-    CheckGenerationInChild("Child", start + 1);
+    // The child's observed value should be unchanged.
+    if (child_generation != CRYPTO_get_fork_generation()) {
+      fprintf(stderr,
+              "Child generation (final stable check) was %" PRIu64
+              ", wanted %" PRIu64 ".\n",
+              child_generation, CRYPTO_get_fork_generation());
+      _exit(1);
+    }
+
     _exit(0);
   }
 
@@ -157,4 +172,4 @@ TEST(ForkDetect, Test) {
   EXPECT_EQ(start, CRYPTO_get_fork_generation());
 }
 
-#endif  // OPENSSL_LINUX && !OPENSSL_TSAN
+#endif  // OPENSSL_FORK_DETECTION && !OPENSSL_TSAN
diff --git a/crypto/rand_extra/forkunsafe.c b/crypto/rand_extra/forkunsafe.c
@@ -33,6 +33,10 @@ void RAND_enable_fork_unsafe_buffering(int fd) {
 
   CRYPTO_atomic_store_u32(&g_buffering_enabled, 1);
 }
+
+void RAND_disable_fork_unsafe_buffering(void) {
+  CRYPTO_atomic_store_u32(&g_buffering_enabled, 0);
+}
 #endif
 
 int rand_fork_unsafe_buffering_enabled(void) {

diff --git a/include/openssl/rand.h b/include/openssl/rand.h
@@ -43,6 +43,11 @@ OPENSSL_EXPORT int RAND_bytes(uint8_t *buf, size_t len);
 // It has an unusual name because the buffer is unsafe across calls to |fork|.
 // Hence, this function should never be called by libraries.
 OPENSSL_EXPORT void RAND_enable_fork_unsafe_buffering(int fd);
+
+// RAND_disable_fork_unsafe_buffering disables efficient buffered reading of
+// /dev/urandom, causing BoringSSL to always draw entropy on every request
+// for random bytes.
+OPENSSL_EXPORT void RAND_disable_fork_unsafe_buffering(void);
 #endif
 
 #if defined(BORINGSSL_UNSAFE_DETERMINISTIC_MODE)