From 2139145b72d084a3f974a94accd7d9812de286e4 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Wed, 11 Oct 2017 19:25:26 +0200 Subject: [PATCH] Add missing RAND_DRBG locking The drbg's lock must be held across calls to RAND_DRBG_generate() to prevent simultaneous modification of internal state. This was observed in practice with simultaneous SSL_new() calls attempting to seed the (separate) per-SSL RAND_DRBG instances from the global rand_drbg instance; this eventually led to simultaneous calls to ctr_BCC_update() attempting to increment drbg->bltmp_pos for their respective partial final block, violating the invariant that bltmp_pos < 16. The AES operations performed in ctr_BCC_blocks() makes the race window quite easy to trigger. A value of bltmp_pos greater than 16 induces catastrophic failure in ctr_BCC_final(), with subtraction overflowing and leading to an attempt to memset() to zero a very large range, which eventually reaches an unmapped page and segfaults. Provide the needed locking in get_entropy_from_parent(), as well as fixing a similar issue in RAND_priv_bytes(). There is also an unlocked call to RAND_DRBG_generate() in ssl_randbytes(), but the requisite serialization is already guaranteed by the requirements on the application's usage of SSL objects, and no further locking is needed for correct behavior. In that case, leave a comment noting the apparent discrepancy and the reason for its safety (at present). Reviewed-by: Paul Dale Reviewed-by: Kurt Roeckx Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/4328) --- crypto/rand/drbg_lib.c | 8 ++++++++ crypto/rand/rand_lib.c | 17 +++++++++++++++-- ssl/ssl_lib.c | 17 +++++++++++++++-- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/crypto/rand/drbg_lib.c b/crypto/rand/drbg_lib.c index 0042a931b0..c471b6eda4 100644 --- a/crypto/rand/drbg_lib.c +++ b/crypto/rand/drbg_lib.c @@ -116,6 +116,8 @@ void RAND_DRBG_free(RAND_DRBG *drbg) /* * Instantiate |drbg|, after it has been initialized. Use |pers| and * |perslen| as prediction-resistance input. + * + * Requires that drbg->lock is already locked for write, if non-null. */ int RAND_DRBG_instantiate(RAND_DRBG *drbg, const unsigned char *pers, size_t perslen) @@ -185,6 +187,8 @@ end: /* * Uninstantiate |drbg|. Must be instantiated before it can be used. + * + * Requires that drbg->lock is already locked for write, if non-null. */ int RAND_DRBG_uninstantiate(RAND_DRBG *drbg) { @@ -197,6 +201,8 @@ int RAND_DRBG_uninstantiate(RAND_DRBG *drbg) /* * Reseed |drbg|, mixing in the specified data + * + * Requires that drbg->lock is already locked for write, if non-null. */ int RAND_DRBG_reseed(RAND_DRBG *drbg, const unsigned char *adin, size_t adinlen) @@ -349,6 +355,8 @@ int rand_drbg_restart(RAND_DRBG *drbg, * to or if |prediction_resistance| is set. Additional input can be * sent in |adin| and |adinlen|. * + * Requires that drbg->lock is already locked for write, if non-null. + * * Returns 1 on success, 0 on failure. * */ diff --git a/crypto/rand/rand_lib.c b/crypto/rand/rand_lib.c index 6f8deca1f9..a290e5c1a2 100644 --- a/crypto/rand/rand_lib.c +++ b/crypto/rand/rand_lib.c @@ -155,12 +155,20 @@ size_t rand_drbg_get_entropy(RAND_DRBG *drbg, if (buffer != NULL) { size_t bytes = 0; - /* Get entropy from parent, include our state as additional input */ + /* + * Get random from parent, include our state as additional input. + * Our lock is already held, but we need to lock our parent before + * generating bits from it. + */ + if (drbg->parent->lock) + CRYPTO_THREAD_write_lock(drbg->parent->lock); if (RAND_DRBG_generate(drbg->parent, buffer, bytes_needed, 0, (unsigned char *)drbg, sizeof(*drbg)) != 0) bytes = bytes_needed; + if (drbg->parent->lock) + CRYPTO_THREAD_unlock(drbg->parent->lock); entropy_available = RAND_POOL_add_end(pool, bytes, 8 * bytes); } @@ -626,6 +634,7 @@ int RAND_priv_bytes(unsigned char *buf, int num) { const RAND_METHOD *meth = RAND_get_rand_method(); RAND_DRBG *drbg; + int ret; if (meth != RAND_OpenSSL()) return RAND_bytes(buf, num); @@ -634,7 +643,11 @@ int RAND_priv_bytes(unsigned char *buf, int num) if (drbg == NULL) return 0; - return RAND_DRBG_generate(drbg, buf, num, 0, NULL, 0); + /* We have to lock the DRBG before generating bits from it. */ + CRYPTO_THREAD_write_lock(drbg->lock); + ret = RAND_DRBG_generate(drbg, buf, num, 0, NULL, 0); + CRYPTO_THREAD_unlock(drbg->lock); + return ret; } int RAND_bytes(unsigned char *buf, int num) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index da74e96ecb..ce45a91613 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -5127,7 +5127,20 @@ uint32_t SSL_get_max_early_data(const SSL *s) int ssl_randbytes(SSL *s, unsigned char *rnd, size_t size) { - if (s->drbg != NULL) - return RAND_DRBG_generate(s->drbg, rnd, size, 0, NULL, 0); + if (s->drbg != NULL) { + /* + * Currently, it's the duty of the caller to serialize the generate + * requests to the DRBG. So formally we have to check whether + * s->drbg->lock != NULL and take the lock if this is the case. + * However, this DRBG is unique to a given SSL object, and we already + * require that SSL objects are only accessed by a single thread at + * a given time. Also, SSL DRBGs have no child DRBG, so there is + * no risk that this DRBG is accessed by a child DRBG in parallel + * for reseeding. As such, we can rely on the application's + * serialization of SSL accesses for the needed concurrency protection + * here. + */ + return RAND_DRBG_generate(s->drbg, rnd, size, 0, NULL, 0); + } return RAND_bytes(rnd, (int)size); } -- 2.25.1