From: Matt Caswell Date: Fri, 8 Jan 2021 13:48:13 +0000 (+0000) Subject: Enable locking on the primary DRBG when we create it X-Git-Tag: openssl-3.0.0-alpha11~96 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f5a50c2a07e288187c14b784be253b3a2a23483b;p=thirdparty%2Fopenssl.git Enable locking on the primary DRBG when we create it The primary DRBG may be shared across multiple threads and therefore we must use locking to access it. Previously we were enabling that locking lazily when we attempted to obtain one of the child DRBGs. Part of the process of enabling the lock, is to create the lock. But if we create the lock lazily then it is too late - we may race with other threads where each thread is independently attempting to enable the locking. This results in multiple locks being created - only one of which "sticks" and the rest are leaked. Instead we enable locking on the primary when we first create it. This is already locked and therefore we cannot race. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/13660) --- diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index bb200a79608..40f93ba0cd8 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2609,7 +2609,7 @@ EVP_R_PRIVATE_KEY_ENCODE_ERROR:146:private key encode error EVP_R_PUBLIC_KEY_NOT_RSA:106:public key not rsa EVP_R_SET_DEFAULT_PROPERTY_FAILURE:209:set default property failure EVP_R_TOO_MANY_RECORDS:183:too many records -EVP_R_UNABLE_TO_ENABLE_PARENT_LOCKING:212:unable to enable parent locking +EVP_R_UNABLE_TO_ENABLE_LOCKING:212:unable to enable locking EVP_R_UNABLE_TO_GET_MAXIMUM_REQUEST_SIZE:215:unable to get maximum request size EVP_R_UNABLE_TO_GET_RANDOM_STRENGTH:216:unable to get random strength EVP_R_UNABLE_TO_LOCK_CONTEXT:211:unable to lock context diff --git a/crypto/evp/evp_err.c b/crypto/evp/evp_err.c index e08c373b33e..ab6e3e879d2 100644 --- a/crypto/evp/evp_err.c +++ b/crypto/evp/evp_err.c @@ -152,7 +152,7 @@ static const ERR_STRING_DATA EVP_str_reasons[] = { {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_SET_DEFAULT_PROPERTY_FAILURE), "set default property failure"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_TOO_MANY_RECORDS), "too many records"}, - {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_UNABLE_TO_ENABLE_PARENT_LOCKING), + {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_UNABLE_TO_ENABLE_LOCKING), "unable to enable parent locking"}, {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_UNABLE_TO_GET_MAXIMUM_REQUEST_SIZE), "unable to get maximum request size"}, diff --git a/crypto/evp/evp_rand.c b/crypto/evp/evp_rand.c index 6a4f57414c3..42e51b4ea1e 100644 --- a/crypto/evp/evp_rand.c +++ b/crypto/evp/evp_rand.c @@ -333,12 +333,6 @@ EVP_RAND_CTX *EVP_RAND_CTX_new(EVP_RAND *rand, EVP_RAND_CTX *parent) return NULL; } if (parent != NULL) { - if (!EVP_RAND_enable_locking(parent)) { - ERR_raise(ERR_LIB_EVP, EVP_R_UNABLE_TO_ENABLE_PARENT_LOCKING); - CRYPTO_THREAD_lock_free(ctx->refcnt_lock); - OPENSSL_free(ctx); - return NULL; - } if (!evp_rand_ctx_up_ref(parent)) { ERR_raise(ERR_LIB_EVP, ERR_R_INTERNAL_ERROR); CRYPTO_THREAD_lock_free(ctx->refcnt_lock); diff --git a/crypto/rand/rand_lib.c b/crypto/rand/rand_lib.c index f0284aab089..01927401abf 100644 --- a/crypto/rand/rand_lib.c +++ b/crypto/rand/rand_lib.c @@ -571,6 +571,17 @@ EVP_RAND_CTX *RAND_get0_primary(OSSL_LIB_CTX *ctx) dgbl->primary = rand_new_drbg(ctx, dgbl->seed, PRIMARY_RESEED_INTERVAL, PRIMARY_RESEED_TIME_INTERVAL); + /* + * The primary DRBG may be shared between multiple threads so we must + * enable locking. + */ + if (dgbl->primary != NULL && !EVP_RAND_enable_locking(dgbl->primary)) { + ERR_raise(ERR_LIB_EVP, EVP_R_UNABLE_TO_ENABLE_LOCKING); + EVP_RAND_CTX_free(dgbl->primary); + dgbl->primary = NULL; + CRYPTO_THREAD_lock_free(dgbl->lock); + return NULL; + } CRYPTO_THREAD_unlock(dgbl->lock); } return dgbl->primary; diff --git a/doc/man3/EVP_RAND.pod b/doc/man3/EVP_RAND.pod index e53cddff2ff..4e1eb88afd2 100644 --- a/doc/man3/EVP_RAND.pod +++ b/doc/man3/EVP_RAND.pod @@ -152,7 +152,9 @@ appropriate size. EVP_RAND_enable_locking() enables locking for the RAND I and all of its parents. After this I will operate in a thread safe manner, albeit -more slowly. +more slowly. This function is not itself thread safe if called with the same +I from multiple threads. Typically locking should be enabled before a +I is shared across multiple threads. EVP_RAND_get_params() retrieves details about the implementation I. diff --git a/include/openssl/evperr.h b/include/openssl/evperr.h index c25cc49025d..309f4cd341e 100644 --- a/include/openssl/evperr.h +++ b/include/openssl/evperr.h @@ -243,7 +243,7 @@ # define EVP_R_PUBLIC_KEY_NOT_RSA 106 # define EVP_R_SET_DEFAULT_PROPERTY_FAILURE 209 # define EVP_R_TOO_MANY_RECORDS 183 -# define EVP_R_UNABLE_TO_ENABLE_PARENT_LOCKING 212 +# define EVP_R_UNABLE_TO_ENABLE_LOCKING 212 # define EVP_R_UNABLE_TO_GET_MAXIMUM_REQUEST_SIZE 215 # define EVP_R_UNABLE_TO_GET_RANDOM_STRENGTH 216 # define EVP_R_UNABLE_TO_LOCK_CONTEXT 211