Skip to content

Commit 1b0fe00

Browse files
mspncpmattcaswell
authored andcommitted
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 b5319bd, 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 #9802)
1 parent 73a683b commit 1b0fe00

File tree

10 files changed

+84
-30
lines changed

10 files changed

+84
-30
lines changed

crypto/include/internal/rand_int.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ typedef struct rand_pool_st RAND_POOL;
2626
void rand_cleanup_int(void);
2727
void rand_drbg_cleanup_int(void);
2828
void drbg_delete_thread_state(void);
29-
void rand_fork(void);
3029

3130
/* Hardware-based seeding functions. */
3231
size_t rand_acquire_entropy_from_tsc(RAND_POOL *pool);

crypto/init.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -847,6 +847,5 @@ void OPENSSL_fork_parent(void)
847847

848848
void OPENSSL_fork_child(void)
849849
{
850-
rand_fork();
851850
}
852851
#endif

crypto/rand/drbg_lib.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ static RAND_DRBG *rand_drbg_new(int secure,
197197
}
198198

199199
drbg->secure = secure && CRYPTO_secure_allocated(drbg);
200-
drbg->fork_count = rand_fork_count;
200+
drbg->fork_id = openssl_get_fork_id();
201201
drbg->parent = parent;
202202

203203
if (parent == NULL) {
@@ -578,6 +578,7 @@ int RAND_DRBG_generate(RAND_DRBG *drbg, unsigned char *out, size_t outlen,
578578
int prediction_resistance,
579579
const unsigned char *adin, size_t adinlen)
580580
{
581+
int fork_id;
581582
int reseed_required = 0;
582583

583584
if (drbg->state != DRBG_READY) {
@@ -603,8 +604,10 @@ int RAND_DRBG_generate(RAND_DRBG *drbg, unsigned char *out, size_t outlen,
603604
return 0;
604605
}
605606

606-
if (drbg->fork_count != rand_fork_count) {
607-
drbg->fork_count = rand_fork_count;
607+
fork_id = openssl_get_fork_id();
608+
609+
if (drbg->fork_id != fork_id) {
610+
drbg->fork_id = fork_id;
608611
reseed_required = 1;
609612
}
610613

crypto/rand/rand_lcl.h

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,12 @@ struct rand_drbg_st {
186186
int secure; /* 1: allocated on the secure heap, 0: otherwise */
187187
int type; /* the nid of the underlying algorithm */
188188
/*
189-
* Stores the value of the rand_fork_count global as of when we last
190-
* reseeded. The DRBG reseeds automatically whenever drbg->fork_count !=
191-
* rand_fork_count. Used to provide fork-safety and reseed this DRBG in
192-
* the child process.
189+
* Stores the return value of openssl_get_fork_id() as of when we last
190+
* reseeded. The DRBG reseeds automatically whenever drbg->fork_id !=
191+
* openssl_get_fork_id(). Used to provide fork-safety and reseed this
192+
* DRBG in the child process.
193193
*/
194-
int fork_count;
194+
int fork_id;
195195
unsigned short flags; /* various external flags */
196196

197197
/*
@@ -283,19 +283,6 @@ struct rand_drbg_st {
283283
/* The global RAND method, and the global buffer and DRBG instance. */
284284
extern RAND_METHOD rand_meth;
285285

286-
/*
287-
* A "generation count" of forks. Incremented in the child process after a
288-
* fork. Since rand_fork_count is increment-only, and only ever written to in
289-
* the child process of the fork, which is guaranteed to be single-threaded, no
290-
* locking is needed for normal (read) accesses; the rest of pthread fork
291-
* processing is assumed to introduce the necessary memory barriers. Sibling
292-
* children of a given parent will produce duplicate values, but this is not
293-
* problematic because the reseeding process pulls input from the system CSPRNG
294-
* and/or other global sources, so the siblings will end up generating
295-
* different output streams.
296-
*/
297-
extern int rand_fork_count;
298-
299286
/* DRBG helpers */
300287
int rand_drbg_restart(RAND_DRBG *drbg,
301288
const unsigned char *buffer, size_t len, size_t entropy);

crypto/rand/rand_lib.c

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@ static CRYPTO_RWLOCK *rand_meth_lock;
2626
static const RAND_METHOD *default_RAND_meth;
2727
static CRYPTO_ONCE rand_init = CRYPTO_ONCE_STATIC_INIT;
2828

29-
int rand_fork_count;
30-
3129
static CRYPTO_RWLOCK *rand_nonce_lock;
3230
static int rand_nonce_count;
3331

@@ -303,11 +301,6 @@ void rand_drbg_cleanup_additional_data(RAND_POOL *pool, unsigned char *out)
303301
rand_pool_reattach(pool, out);
304302
}
305303

306-
void rand_fork(void)
307-
{
308-
rand_fork_count++;
309-
}
310-
311304
DEFINE_RUN_ONCE_STATIC(do_rand_init)
312305
{
313306
#ifndef OPENSSL_NO_ENGINE

crypto/threads_none.c

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@
1212

1313
#if !defined(OPENSSL_THREADS) || defined(CRYPTO_TDEBUG)
1414

15+
# if defined(OPENSSL_SYS_UNIX)
16+
# include <sys/types.h>
17+
# include <unistd.h>
18+
# endif
19+
1520
CRYPTO_RWLOCK *CRYPTO_THREAD_lock_new(void)
1621
{
1722
CRYPTO_RWLOCK *lock;
@@ -133,4 +138,12 @@ int openssl_init_fork_handlers(void)
133138
return 0;
134139
}
135140

141+
int openssl_get_fork_id(void)
142+
{
143+
# if defined(OPENSSL_SYS_UNIX)
144+
return getpid();
145+
# else
146+
return return 0;
147+
# endif
148+
}
136149
#endif

crypto/threads_pthread.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@
1212

1313
#if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
1414

15+
# if defined(OPENSSL_SYS_UNIX)
16+
# include <sys/types.h>
17+
# include <unistd.h>
18+
#endif
19+
1520
# ifdef PTHREAD_RWLOCK_INITIALIZER
1621
# define USE_RWLOCK
1722
# endif
@@ -193,4 +198,9 @@ int openssl_init_fork_handlers(void)
193198
# endif
194199
return 0;
195200
}
201+
202+
int openssl_get_fork_id(void)
203+
{
204+
return getpid();
205+
}
196206
#endif

crypto/threads_win.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,4 +164,8 @@ int openssl_init_fork_handlers(void)
164164
return 0;
165165
}
166166

167+
int openssl_get_fork_id(void)
168+
{
169+
return 0;
170+
}
167171
#endif

include/internal/cryptlib.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ extern unsigned int OPENSSL_ia32cap_P[];
8080
void OPENSSL_showfatal(const char *fmta, ...);
8181
void crypto_cleanup_all_ex_data_int(void);
8282
int openssl_init_fork_handlers(void);
83+
int openssl_get_fork_id(void);
8384

8485
char *ossl_safe_getenv(const char *name);
8586

test/drbgtest.c

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@
2222
# include <windows.h>
2323
#endif
2424

25+
26+
#if defined(OPENSSL_SYS_UNIX)
27+
# include <sys/types.h>
28+
# include <sys/wait.h>
29+
# include <unistd.h>
30+
#endif
31+
2532
#include "testutil.h"
2633
#include "drbgtest.h"
2734

@@ -669,6 +676,40 @@ static int test_drbg_reseed(int expect_success,
669676
return 1;
670677
}
671678

679+
680+
#if defined(OPENSSL_SYS_UNIX)
681+
/*
682+
* Test whether master, public and private DRBG are reseeded after
683+
* forking the process.
684+
*/
685+
static int test_drbg_reseed_after_fork(RAND_DRBG *master,
686+
RAND_DRBG *public,
687+
RAND_DRBG *private)
688+
{
689+
pid_t pid;
690+
int status=0;
691+
692+
pid = fork();
693+
if (!TEST_int_ge(pid, 0))
694+
return 0;
695+
696+
if (pid > 0) {
697+
/* I'm the parent; wait for the child and check its exit code */
698+
return TEST_int_eq(waitpid(pid, &status, 0), pid) && TEST_int_eq(status, 0);
699+
}
700+
701+
/* I'm the child; check whether all three DRBGs reseed. */
702+
if (!TEST_true(test_drbg_reseed(1, master, public, private, 1, 1, 1, 0)))
703+
status = 1;
704+
705+
/* Remove hooks */
706+
unhook_drbg(master);
707+
unhook_drbg(public);
708+
unhook_drbg(private);
709+
exit(status);
710+
}
711+
#endif
712+
672713
/*
673714
* Test whether the default rand_method (RAND_OpenSSL()) is
674715
* setup correctly, in particular whether reseeding works
@@ -755,6 +796,10 @@ static int test_rand_drbg_reseed(void)
755796
goto error;
756797
reset_drbg_hook_ctx();
757798

799+
#if defined(OPENSSL_SYS_UNIX)
800+
if (!TEST_true(test_drbg_reseed_after_fork(master, public, private)))
801+
goto error;
802+
#endif
758803

759804
/* fill 'randomness' buffer with some arbitrary data */
760805
memset(rand_add_buf, 'r', sizeof(rand_add_buf));

0 commit comments

Comments
 (0)