From 9ed79d8ee1ef845fce94739787d45ad03f675eaa Mon Sep 17 00:00:00 2001 From: Rich Salz Date: Mon, 7 Aug 2017 19:21:36 -0400 Subject: [PATCH] Various RAND improvements Try to put DRBG and rand_bytes buffers in secure heap Read the TSC fewer times (but it's still not enabled). Short-circuit return in win RAND_poll_ex; other minor tweaks and format-fixes. Use the _bytes version of rdrand/rdseed Fix ia32cap checks. Reviewed-by: Bernd Edlinger (Merged from https://github.com/openssl/openssl/pull/4100) --- crypto/rand/rand_lcl.h | 9 ++++-- crypto/rand/rand_lib.c | 62 ++++++++++++++++++++++-------------------- crypto/rand/rand_win.c | 27 ++++++++++-------- 3 files changed, 55 insertions(+), 43 deletions(-) diff --git a/crypto/rand/rand_lcl.h b/crypto/rand/rand_lcl.h index c96625456d..e60f619d61 100644 --- a/crypto/rand/rand_lcl.h +++ b/crypto/rand/rand_lcl.h @@ -25,6 +25,9 @@ */ # define RANDOMNESS_NEEDED 16 +/* How many times to read the TSC as a randomness source. */ +# define TSC_READ_COUNT 4 + /* Maximum amount of randomness to hold in RAND_BYTES_BUFFER. */ # define MAX_RANDOMNESS_HELD (4 * RANDOMNESS_NEEDED) @@ -57,9 +60,10 @@ typedef enum drbg_status_e { */ typedef struct rand_bytes_buffer_st { CRYPTO_RWLOCK *lock; + unsigned char *buff; size_t size; size_t curr; - unsigned char *buff; + int secure; } RAND_BYTES_BUFFER; /* @@ -90,7 +94,8 @@ struct rand_drbg_st { int nid; /* the underlying algorithm */ int fork_count; unsigned short flags; /* various external flags */ - unsigned short filled; + char filled; + char secure; /* * This is a fixed-size buffer, but we malloc to make it a little * harder to find; a classic security/performance trade-off. diff --git a/crypto/rand/rand_lib.c b/crypto/rand/rand_lib.c index 0810518cc6..489b5380c9 100644 --- a/crypto/rand/rand_lib.c +++ b/crypto/rand/rand_lib.c @@ -30,8 +30,8 @@ int rand_fork_count; #ifdef OPENSSL_RAND_SEED_RDTSC /* * IMPORTANT NOTE: It is not currently possible to use this code - * because we are not sure about the amount of randomness. Some - * SP900 tests have been run, but there is internal skepticism. + * because we are not sure about the amount of randomness it provides. + * Some SP900 tests have been run, but there is internal skepticism. * So for now this code is not used. */ # error "RDTSC enabled? Should not be possible!" @@ -47,45 +47,39 @@ void rand_read_tsc(RAND_poll_fn cb, void *arg) unsigned char c; int i; - for (i = 0; i < 10; i++) { - c = (unsigned char)(OPENSSL_rdtsc() & 0xFF); - cb(arg, &c, 1, 0.5); + if ((OPENSSL_ia32cap_P[0] & (1 << 4)) != 0) { + for (i = 0; i < TSC_READ_COUNT; i++) { + c = (unsigned char)(OPENSSL_rdtsc() & 0xFF); + cb(arg, &c, 1, 0.5); + } } } #endif #ifdef OPENSSL_RAND_SEED_RDCPU -size_t OPENSSL_ia32_rdseed(void); -size_t OPENSSL_ia32_rdrand(void); +size_t OPENSSL_ia32_rdseed_bytes(char *buf, size_t len); +size_t OPENSSL_ia32_rdrand_bytes(char *buf, size_t len); extern unsigned int OPENSSL_ia32cap_P[]; int rand_read_cpu(RAND_poll_fn cb, void *arg) { - size_t i, s; + char buff[RANDOMNESS_NEEDED]; /* If RDSEED is available, use that. */ - if ((OPENSSL_ia32cap_P[1] & (1 << 18)) != 0) { - for (i = 0; i < RANDOMNESS_NEEDED; i += sizeof(s)) { - s = OPENSSL_ia32_rdseed(); - if (s == 0) - break; - cb(arg, &s, (int)sizeof(s), sizeof(s)); - } - if (i >= RANDOMNESS_NEEDED) + if ((OPENSSL_ia32cap_P[2] & (1 << 18)) != 0) { + if (OPENSSL_ia32_rdseed_bytes(buff, sizeof(buff)) == sizeof(buff)) { + cb(arg, buff, (int)sizeof(buff), sizeof(buff)); return 1; + } } /* Second choice is RDRAND. */ if ((OPENSSL_ia32cap_P[1] & (1 << (62 - 32))) != 0) { - for (i = 0; i < RANDOMNESS_NEEDED; i += sizeof(s)) { - s = OPENSSL_ia32_rdrand(); - if (s == 0) - break; - cb(arg, &s, (int)sizeof(s), sizeof(s)); - } - if (i >= RANDOMNESS_NEEDED) + if (OPENSSL_ia32_rdrand_bytes(buff, sizeof(buff)) == sizeof(buff)) { + cb(arg, buff, (int)sizeof(buff), sizeof(buff)); return 1; + } } return 0; @@ -186,7 +180,10 @@ static int setup_drbg(RAND_DRBG *drbg) drbg->lock = CRYPTO_THREAD_lock_new(); ret &= drbg->lock != NULL; drbg->size = RANDOMNESS_NEEDED; - drbg->randomness = OPENSSL_malloc(drbg->size); + drbg->secure = CRYPTO_secure_malloc_initialized(); + drbg->randomness = drbg->secure + ? OPENSSL_secure_malloc(drbg->size) + : OPENSSL_malloc(drbg->size); ret &= drbg->randomness != NULL; /* If you change these parameters, see RANDOMNESS_NEEDED */ ret &= RAND_DRBG_set(drbg, @@ -199,7 +196,10 @@ static int setup_drbg(RAND_DRBG *drbg) static void free_drbg(RAND_DRBG *drbg) { CRYPTO_THREAD_lock_free(drbg->lock); - OPENSSL_clear_free(drbg->randomness, drbg->size); + if (drbg->secure) + OPENSSL_secure_clear_free(drbg->randomness, drbg->size); + else + OPENSSL_clear_free(drbg->randomness, drbg->size); RAND_DRBG_uninstantiate(drbg); } @@ -223,9 +223,10 @@ DEFINE_RUN_ONCE_STATIC(do_rand_init) ret &= rand_bytes.lock != NULL; rand_bytes.curr = 0; rand_bytes.size = MAX_RANDOMNESS_HELD; - /* TODO: Should this be secure malloc? */ - rand_bytes.buff = malloc(rand_bytes.size); - + rand_bytes.secure = CRYPTO_secure_malloc_initialized(); + rand_bytes.buff = rand_bytes.secure + ? OPENSSL_secure_malloc(rand_bytes.size) + : OPENSSL_malloc(rand_bytes.size); ret &= rand_bytes.buff != NULL; ret &= setup_drbg(&rand_drbg); ret &= setup_drbg(&priv_drbg); @@ -244,7 +245,10 @@ void rand_cleanup_int(void) #endif CRYPTO_THREAD_lock_free(rand_meth_lock); CRYPTO_THREAD_lock_free(rand_bytes.lock); - OPENSSL_clear_free(rand_bytes.buff, rand_bytes.size); + if (rand_bytes.secure) + OPENSSL_secure_clear_free(rand_bytes.buff, rand_bytes.size); + else + OPENSSL_clear_free(rand_bytes.buff, rand_bytes.size); free_drbg(&rand_drbg); free_drbg(&priv_drbg); } diff --git a/crypto/rand/rand_win.c b/crypto/rand/rand_win.c index 5685ee84b7..457e2ade39 100644 --- a/crypto/rand/rand_win.c +++ b/crypto/rand/rand_win.c @@ -43,34 +43,35 @@ int RAND_poll_ex(RAND_poll_fn cb, void *arg) { # ifndef USE_BCRYPTGENRANDOM HCRYPTPROV hProvider; + int ok = 0; # endif - DWORD w; BYTE buf[RANDOMNESS_NEEDED]; - int ok = 0; # ifdef OPENSSL_RAND_SEED_RDTSC rand_read_tsc(cb, arg); # endif # ifdef OPENSSL_RAND_SEED_RDCPU if (rand_read_cpu(cb, arg)) - ok++; + return 1; # endif # ifdef USE_BCRYPTGENRANDOM if (BCryptGenRandom(NULL, buf, (ULONG)sizeof(buf), - BCRYPT_USE_SYSTEM_PREFERRED_RNG) != STATUS_SUCCESS) - return 0; - cb(arg, buf, sizeof(buf), sizeof(buf)); - return 1; + BCRYPT_USE_SYSTEM_PREFERRED_RNG) == STATUS_SUCCESS) { + cb(arg, buf, sizeof(buf), sizeof(buf)); + return 1; + } # else /* poll the CryptoAPI PRNG */ if (CryptAcquireContextW(&hProvider, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT | CRYPT_SILENT) != 0) { if (CryptGenRandom(hProvider, (DWORD)sizeof(buf), buf) != 0) { cb(arg, buf, sizeof(buf), sizeof(buf)); - ok++; + ok = 1; } CryptReleaseContext(hProvider, 0); + if (ok) + return 1; } /* poll the Pentium PRG with CryptoAPI */ @@ -78,16 +79,18 @@ int RAND_poll_ex(RAND_poll_fn cb, void *arg) CRYPT_VERIFYCONTEXT | CRYPT_SILENT) != 0) { if (CryptGenRandom(hProvider, (DWORD)sizeof(buf), buf) != 0) { cb(arg, buf, sizeof(buf), sizeof(buf)); - ok++; + ok = 1; } CryptReleaseContext(hProvider, 0); + if (ok) + return 1; } # endif - return ok ? 1 : 0; + return 0; } -#if OPENSSL_API_COMPAT < 0x10100000L +# if OPENSSL_API_COMPAT < 0x10100000L int RAND_event(UINT iMsg, WPARAM wParam, LPARAM lParam) { RAND_poll(); @@ -98,6 +101,6 @@ void RAND_screen(void) { RAND_poll(); } -#endif +# endif #endif -- 2.25.1