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/9802)

diff --git a/crypto/include/internal/rand_int.h b/crypto/include/internal/rand_int.h
index ea3b1a4311addc02b0d9b0d017745b391718b127..a4947bc46508a893971d0631abfee5a0374e858b 100644 (file)
--- a/crypto/include/internal/rand_int.h
+++ b/crypto/include/internal/rand_int.h
@@ -26,7 +26,6 @@ typedef struct rand_pool_st RAND_POOL;
 void rand_cleanup_int(void);
 void rand_drbg_cleanup_int(void);
 void drbg_delete_thread_state(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 46839e768fd22fc4d3e5cada98223fe5c299248f..9fc0e8ef68aadbbcac562541e4f51b7dee98bb4a 100644 (file)
--- a/crypto/init.c
+++ b/crypto/init.c
@@ -847,6 +847,5 @@ void OPENSSL_fork_parent(void)
 
 void OPENSSL_fork_child(void)
 {
-    rand_fork();
 }
 #endif

diff --git a/crypto/rand/drbg_lib.c b/crypto/rand/drbg_lib.c
index 7d0c8027fd99635d243797cd153c3b4db08d0168..12bb627a04efab8e85f5de2c8f14104af4f5c7c1 100644 (file)
--- a/crypto/rand/drbg_lib.c
+++ b/crypto/rand/drbg_lib.c
@@ -197,7 +197,7 @@ static RAND_DRBG *rand_drbg_new(int secure,
     }
 
     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) {
@@ -578,6 +578,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) {
@@ -603,8 +604,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 f28642e4252056d67cb01c6ddd6cfedb9d444849..1c2b266330aed8a8e78d3673464269ad6b1ff11e 100644 (file)
--- a/crypto/rand/rand_lcl.h
+++ b/crypto/rand/rand_lcl.h
@@ -186,12 +186,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 */
 
     /*
@@ -283,19 +283,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 23bb2e68b1b79ae913d2720c5bb095ac8f1adfab..ce4e17c41496a11009a83f3975b22cc9128d6221 100644 (file)
--- a/crypto/rand/rand_lib.c
+++ b/crypto/rand/rand_lib.c
@@ -26,8 +26,6 @@ static CRYPTO_RWLOCK *rand_meth_lock;
 static const RAND_METHOD *default_RAND_meth;
 static CRYPTO_ONCE rand_init = CRYPTO_ONCE_STATIC_INIT;
 
-int rand_fork_count;
-
 static CRYPTO_RWLOCK *rand_nonce_lock;
 static int rand_nonce_count;
 
@@ -303,11 +301,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++;
-}
-
 DEFINE_RUN_ONCE_STATIC(do_rand_init)
 {
 #ifndef OPENSSL_NO_ENGINE

diff --git a/crypto/threads_none.c b/crypto/threads_none.c
index 4b1940ae44dbd63a50df824a5d876633a2786796..7348af02c1e8855a3ca493ca8924c0b93ab073ba 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 return 0;
+# endif
+}
 #endif

diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c
index 5a59779ebbb1d9b15267c35b74e907d467b12d6c..7e8603de663cdc6333b0d759f44efb5107329e9b 100644 (file)
--- a/crypto/threads_pthread.c
+++ b/crypto/threads_pthread.c
@@ -12,6 +12,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
@@ -193,4 +198,9 @@ int openssl_init_fork_handlers(void)
 # endif
     return 0;
 }
+
+int openssl_get_fork_id(void)
+{
+    return getpid();
+}
 #endif

diff --git a/crypto/threads_win.c b/crypto/threads_win.c
index b0b16fd3307f4bdc84e11282e120d15f716b5b75..44a360fcabb8d695e7aa7a0c4694083fcb5f0303 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 b4d76d5f2ed73595faca43ebd91a377e82673e75..23e17e5586e8f527924e454260040bd4dff30b6c 100644 (file)
--- a/include/internal/cryptlib.h
+++ b/include/internal/cryptlib.h
@@ -80,6 +80,7 @@ extern unsigned int OPENSSL_ia32cap_P[];
 void OPENSSL_showfatal(const char *fmta, ...);
 void crypto_cleanup_all_ex_data_int(void);
 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 76d9e93955730f2444ebdb7227e61f87547ab478..7325e2ffa38fee87c851f200089f0fde5bf53926 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"
 
@@ -669,6 +676,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
@@ -755,6 +796,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));