drbg: ensure fork-safety without using a pthread_atfork handler

When the new OpenSSL CSPRNG was introduced in version 1.1.1,
it was announced in the release notes that it would be fork-safe,
which the old CSPRNG hadn't been.

The fork-safety was implemented using a fork count, which was
incremented by a pthread_atfork handler. Initially, this handler
was enabled by default. Unfortunately, the default behaviour
had to be changed for other reasons in commit b5319bdbd095, so
the new OpenSSL CSPRNG failed to keep its promise.

This commit restores the fork-safety using a different approach.
It replaces the fork count by a fork id, which coincides with
the process id on UNIX-like operating systems and is zero on other
operating systems. It is used to detect when an automatic reseed
after a fork is necessary.

To prevent a future regression, it also adds a test to verify that
the child reseeds after fork.

CVE-2019-1549

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9832)

diff --git a/crypto/include/internal/rand_int.h b/crypto/include/internal/rand_int.h
index c5d0c20551bee1427e4e5515d38ca01f7b23eedb..bc427e3cf47b90f7c251055c2c64c5c48cf53a17 100644 (file)
--- a/crypto/include/internal/rand_int.h
+++ b/crypto/include/internal/rand_int.h
@@ -24,7 +24,6 @@
 typedef struct rand_pool_st RAND_POOL;
 
 void rand_cleanup_int(void);
-void rand_fork(void);
 
 /* Hardware-based seeding functions. */
 size_t rand_acquire_entropy_from_tsc(RAND_POOL *pool);

diff --git a/crypto/init.c b/crypto/init.c
index 36c63338777501d416ba7c38171270978ff3273b..6536bd5266b532a11a254d16f295cf9d7f1af3dd 100644 (file)
--- a/crypto/init.c
+++ b/crypto/init.c
@@ -676,7 +676,6 @@ void OPENSSL_fork_parent(void)
 
 void OPENSSL_fork_child(void)
 {
-    rand_fork();
     /* TODO(3.0): Inform all providers about a fork event */
 }
 #endif

diff --git a/crypto/rand/drbg_lib.c b/crypto/rand/drbg_lib.c
index f8b58d7245631358c4b662e03e488e6c3c5a1e9b..c24222188f8e7f885e44f3a5dedf20cea077448a 100644 (file)
--- a/crypto/rand/drbg_lib.c
+++ b/crypto/rand/drbg_lib.c
@@ -415,7 +415,7 @@ static RAND_DRBG *rand_drbg_new(OPENSSL_CTX *ctx,
 
     drbg->libctx = ctx;
     drbg->secure = secure && CRYPTO_secure_allocated(drbg);
-    drbg->fork_count = rand_fork_count;
+    drbg->fork_id = openssl_get_fork_id();
     drbg->parent = parent;
 
     if (parent == NULL) {
@@ -829,6 +829,7 @@ int RAND_DRBG_generate(RAND_DRBG *drbg, unsigned char *out, size_t outlen,
                        int prediction_resistance,
                        const unsigned char *adin, size_t adinlen)
 {
+    int fork_id;
     int reseed_required = 0;
 
     if (drbg->state != DRBG_READY) {
@@ -854,8 +855,10 @@ int RAND_DRBG_generate(RAND_DRBG *drbg, unsigned char *out, size_t outlen,
         return 0;
     }
 
-    if (drbg->fork_count != rand_fork_count) {
-        drbg->fork_count = rand_fork_count;
+    fork_id = openssl_get_fork_id();
+
+    if (drbg->fork_id != fork_id) {
+        drbg->fork_id = fork_id;
         reseed_required = 1;
     }
 

diff --git a/crypto/rand/rand_lcl.h b/crypto/rand/rand_lcl.h
index c954cf5f39bc38da6ee3b1a1ee45ebdc94d04461..0c92d75666fc20a6bc195b1a4b27c99a2f36f804 100644 (file)
--- a/crypto/rand/rand_lcl.h
+++ b/crypto/rand/rand_lcl.h
@@ -225,12 +225,12 @@ struct rand_drbg_st {
     int secure; /* 1: allocated on the secure heap, 0: otherwise */
     int type; /* the nid of the underlying algorithm */
     /*
-     * Stores the value of the rand_fork_count global as of when we last
-     * reseeded.  The DRBG reseeds automatically whenever drbg->fork_count !=
-     * rand_fork_count.  Used to provide fork-safety and reseed this DRBG in
-     * the child process.
+     * Stores the return value of openssl_get_fork_id() as of when we last
+     * reseeded.  The DRBG reseeds automatically whenever drbg->fork_id !=
+     * openssl_get_fork_id().  Used to provide fork-safety and reseed this
+     * DRBG in the child process.
      */
-    int fork_count;
+    int fork_id;
     unsigned short flags; /* various external flags */
 
     /*
@@ -331,19 +331,6 @@ struct rand_drbg_st {
 /* The global RAND method, and the global buffer and DRBG instance. */
 extern RAND_METHOD rand_meth;
 
-/*
- * A "generation count" of forks.  Incremented in the child process after a
- * fork.  Since rand_fork_count is increment-only, and only ever written to in
- * the child process of the fork, which is guaranteed to be single-threaded, no
- * locking is needed for normal (read) accesses; the rest of pthread fork
- * processing is assumed to introduce the necessary memory barriers.  Sibling
- * children of a given parent will produce duplicate values, but this is not
- * problematic because the reseeding process pulls input from the system CSPRNG
- * and/or other global sources, so the siblings will end up generating
- * different output streams.
- */
-extern int rand_fork_count;
-
 /* DRBG helpers */
 int rand_drbg_restart(RAND_DRBG *drbg,
                       const unsigned char *buffer, size_t len, size_t entropy);

diff --git a/crypto/rand/rand_lib.c b/crypto/rand/rand_lib.c
index 1ab2a8246c4af5b636c90dd853512ac587c656d6..285089e390113f82a147691a21fb59e3f0704794 100644 (file)
--- a/crypto/rand/rand_lib.c
+++ b/crypto/rand/rand_lib.c
@@ -30,8 +30,6 @@ static CRYPTO_ONCE rand_init = CRYPTO_ONCE_STATIC_INIT;
 static int rand_inited = 0;
 #endif /* FIPS_MODE */
 
-int rand_fork_count;
-
 #ifdef OPENSSL_RAND_SEED_RDTSC
 /*
  * IMPORTANT NOTE:  It is not currently possible to use this code
@@ -238,11 +236,6 @@ void rand_drbg_cleanup_additional_data(RAND_POOL *pool, unsigned char *out)
     rand_pool_reattach(pool, out);
 }
 
-void rand_fork(void)
-{
-    rand_fork_count++;
-}
-
 #ifndef FIPS_MODE
 DEFINE_RUN_ONCE_STATIC(do_rand_init)
 {

diff --git a/crypto/threads_none.c b/crypto/threads_none.c
index ff95556c17d62356ffbec20bf84340256b431d65..c12d5610aae1e6bcb69037727a5bd3e96102a9d6 100644 (file)
--- a/crypto/threads_none.c
+++ b/crypto/threads_none.c
@@ -12,6 +12,11 @@
 
 #if !defined(OPENSSL_THREADS) || defined(CRYPTO_TDEBUG)
 
+# if defined(OPENSSL_SYS_UNIX)
+#  include <sys/types.h>
+#  include <unistd.h>
+# endif
+
 CRYPTO_RWLOCK *CRYPTO_THREAD_lock_new(void)
 {
     CRYPTO_RWLOCK *lock;
@@ -133,4 +138,12 @@ int openssl_init_fork_handlers(void)
     return 0;
 }
 
+int openssl_get_fork_id(void)
+{
+# if defined(OPENSSL_SYS_UNIX)
+    return getpid();
+# else
+    return 0;
+# endif
+}
 #endif

diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c
index c3fd2411db18e27d1a9ff98e1ed3561c7c658c69..762da60a87093c772c487c98e8e0df23c8eef749 100644 (file)
--- a/crypto/threads_pthread.c
+++ b/crypto/threads_pthread.c
@@ -16,6 +16,11 @@
 
 #if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
 
+# if defined(OPENSSL_SYS_UNIX)
+#  include <sys/types.h>
+#  include <unistd.h>
+#endif
+
 # ifdef PTHREAD_RWLOCK_INITIALIZER
 #  define USE_RWLOCK
 # endif
@@ -207,4 +212,9 @@ int openssl_init_fork_handlers(void)
     return 0;
 }
 # endif /* FIPS_MODE */
+
+int openssl_get_fork_id(void)
+{
+    return getpid();
+}
 #endif

diff --git a/crypto/threads_win.c b/crypto/threads_win.c
index 73203834c184cbbc522a7bce29a5f453420141d4..933fdae562e1cc50658542e01454c42e23668452 100644 (file)
--- a/crypto/threads_win.c
+++ b/crypto/threads_win.c
@@ -164,4 +164,8 @@ int openssl_init_fork_handlers(void)
     return 0;
 }
 
+int openssl_get_fork_id(void)
+{
+    return 0;
+}
 #endif

diff --git a/include/internal/cryptlib.h b/include/internal/cryptlib.h
index d591f203d2f700981e019b7f22334d7e2f08dd59..cfac74a328c44ee60122de67c97a2ab51d5f9edd 100644 (file)
--- a/include/internal/cryptlib.h
+++ b/include/internal/cryptlib.h
@@ -93,6 +93,7 @@ void OPENSSL_showfatal(const char *fmta, ...);
 int do_ex_data_init(OPENSSL_CTX *ctx);
 void crypto_cleanup_all_ex_data_int(OPENSSL_CTX *ctx);
 int openssl_init_fork_handlers(void);
+int openssl_get_fork_id(void);
 
 char *ossl_safe_getenv(const char *name);
 

diff --git a/test/drbgtest.c b/test/drbgtest.c
index 9efdd87a4d75288ef42ff5430244b5d62eb85fa5..4559ffd635aff53436e937e8236692fb7fcf38e0 100644 (file)
--- a/test/drbgtest.c
+++ b/test/drbgtest.c
@@ -22,6 +22,13 @@
 # include <windows.h>
 #endif
 
+
+#if defined(OPENSSL_SYS_UNIX)
+# include <sys/types.h>
+# include <sys/wait.h>
+# include <unistd.h>
+#endif
+
 #include "testutil.h"
 #include "drbgtest.h"
 
@@ -708,6 +715,40 @@ static int test_drbg_reseed(int expect_success,
     return 1;
 }
 
+
+#if defined(OPENSSL_SYS_UNIX)
+/*
+ * Test whether master, public and private DRBG are reseeded after
+ * forking the process.
+ */
+static int test_drbg_reseed_after_fork(RAND_DRBG *master,
+                                       RAND_DRBG *public,
+                                       RAND_DRBG *private)
+{
+    pid_t pid;
+    int status=0;
+
+    pid = fork();
+    if (!TEST_int_ge(pid, 0))
+        return 0;
+
+    if (pid > 0) {
+        /* I'm the parent; wait for the child and check its exit code */
+        return TEST_int_eq(waitpid(pid, &status, 0), pid) && TEST_int_eq(status, 0);
+    }
+
+    /* I'm the child; check whether all three DRBGs reseed. */
+    if (!TEST_true(test_drbg_reseed(1, master, public, private, 1, 1, 1, 0)))
+        status = 1;
+
+    /* Remove hooks  */
+    unhook_drbg(master);
+    unhook_drbg(public);
+    unhook_drbg(private);
+    exit(status);
+}
+#endif
+
 /*
  * Test whether the default rand_method (RAND_OpenSSL()) is
  * setup correctly, in particular whether reseeding  works
@@ -798,6 +839,10 @@ static int test_rand_drbg_reseed(void)
         goto error;
     reset_drbg_hook_ctx();
 
+#if defined(OPENSSL_SYS_UNIX)
+    if (!TEST_true(test_drbg_reseed_after_fork(master, public, private)))
+        goto error;
+#endif
 
     /* fill 'randomness' buffer with some arbitrary data */
     memset(rand_add_buf, 'r', sizeof(rand_add_buf));