Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't get a lock when querying the parent reseed_count (alternative version) #20970

Closed
wants to merge 3 commits into from
Closed

Don't get a lock when querying the parent reseed_count (alternative version) #20970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7265395
Refactor the DRBG implementations to manage locking themselves
mattcaswell May 15, 2023
e7c716e
Enable obtaining certain DRBG params without a lock
mattcaswell May 15, 2023
cf1211d
fixup! Refactor the DRBG implementations to manage locking themselves
mattcaswell May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
136 changes: 99 additions & 37 deletions providers/implementations/rands/drbg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,20 @@ static const OSSL_DISPATCH *find_call(const OSSL_DISPATCH *dispatch,

static int rand_drbg_restart(PROV_DRBG *drbg);

/*
* We interpret a call to this function as a hint only and ignore it. This
* occurs when the EVP layer thinks we should do some locking. In practice
* however we manage for ourselves when we take a lock or not on the basis
* of whether drbg->lock is present or not.
*/
int ossl_drbg_lock(void *vctx)
{
PROV_DRBG *drbg = vctx;

if (drbg == NULL || drbg->lock == NULL)
return 1;
return CRYPTO_THREAD_write_lock(drbg->lock);
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just wrong: the primary DRBG will no longer be protected by a lock and it is accessed by multiple threads. Chaos ensues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the case. The primary DRBG is still locked. It just does not lock in response to the evp lock/unlock calls any more. The DRBG manages its own locking instead.

Copy link
Contributor

@tmshort tmshort May 30, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Should this be incrementing a counter/metric or something, so we can "get a hint" if something needs to happen here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what purpose? It's slightly tricky to do. It would probably have to be implemented as an atomic to give reliable results. But I'm not sure what we then do with that information? It can't really be used for any decision making. An increment of the counter just tells you that a thread somewhere thinks a lock is necessary for the thing it wants to do. You could validate that the number of lock calls equals the number of unlock calls....but you can only really do that when the drbg is finished with. I suppose you could detect that we've had more unlock calls that lock calls immediately. But these are all "should not happen" scenarios which could only really be used for "assert" checking. I'm not sure the extra cost of an atomic is worth it for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, then, perhaps it should just be removed? If it's not actually be used to "hint at" anything, it's kinda pointless to keep it. Doesn't block my approval, though.

}

/* Interpreted as a hint only and ignored as for ossl_drbg_lock() */
Copy link
Contributor

@tmshort tmshort May 30, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: In a similar vein, a counter/metric to track this? Even if just in a debug build?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

void ossl_drbg_unlock(void *vctx)
{
PROV_DRBG *drbg = vctx;

if (drbg != NULL && drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);
}

static int ossl_drbg_lock_parent(PROV_DRBG *drbg)
Expand Down Expand Up @@ -484,16 +483,12 @@ int ossl_prov_drbg_uninstantiate(PROV_DRBG *drbg)
return 1;
}

/*
* Reseed |drbg|, mixing in the specified data
*
* Requires that drbg->lock is already locked for write, if non-null.
*
* Returns 1 on success, 0 on failure.
*/
int ossl_prov_drbg_reseed(PROV_DRBG *drbg, int prediction_resistance,
const unsigned char *ent, size_t ent_len,
const unsigned char *adin, size_t adinlen)
static int ossl_prov_drbg_reseed_unlocked(PROV_DRBG *drbg,
int prediction_resistance,
const unsigned char *ent,
size_t ent_len,
const unsigned char *adin,
size_t adinlen)
{
unsigned char *entropy = NULL;
size_t entropylen = 0;
Expand Down Expand Up @@ -595,12 +590,37 @@ int ossl_prov_drbg_reseed(PROV_DRBG *drbg, int prediction_resistance,
return 0;
}

/*
* Reseed |drbg|, mixing in the specified data
*
* Acquires the drbg->lock for writing, if non-null.
*
* Returns 1 on success, 0 on failure.
*/
int ossl_prov_drbg_reseed(PROV_DRBG *drbg, int prediction_resistance,
const unsigned char *ent, size_t ent_len,
const unsigned char *adin, size_t adinlen)
{
int ret;

if (drbg->lock != NULL && !CRYPTO_THREAD_write_lock(drbg->lock))
return 0;

ret = ossl_prov_drbg_reseed_unlocked(drbg, prediction_resistance, ent,
ent_len, adin, adinlen);

if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);

return ret;
}

/*
* Generate |outlen| bytes into the buffer at |out|. Reseed if we need
* 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.
* Acquires the drbg->lock for writing if available
*
* Returns 1 on success, 0 on failure.
*
Expand All @@ -611,35 +631,39 @@ int ossl_prov_drbg_generate(PROV_DRBG *drbg, unsigned char *out, size_t outlen,
{
int fork_id;
int reseed_required = 0;
int ret = 0;

if (!ossl_prov_is_running())
return 0;

if (drbg->lock != NULL && !CRYPTO_THREAD_write_lock(drbg->lock))
return 0;

if (drbg->state != EVP_RAND_STATE_READY) {
/* try to recover from previous errors */
rand_drbg_restart(drbg);

if (drbg->state == EVP_RAND_STATE_ERROR) {
ERR_raise(ERR_LIB_PROV, PROV_R_IN_ERROR_STATE);
return 0;
goto err;
}
if (drbg->state == EVP_RAND_STATE_UNINITIALISED) {
ERR_raise(ERR_LIB_PROV, PROV_R_NOT_INSTANTIATED);
return 0;
goto err;
}
}
if (strength > drbg->strength) {
ERR_raise(ERR_LIB_PROV, PROV_R_INSUFFICIENT_DRBG_STRENGTH);
return 0;
goto err;
}

if (outlen > drbg->max_request) {
ERR_raise(ERR_LIB_PROV, PROV_R_REQUEST_TOO_LARGE_FOR_DRBG);
return 0;
goto err;
}
if (adinlen > drbg->max_adinlen) {
ERR_raise(ERR_LIB_PROV, PROV_R_ADDITIONAL_INPUT_TOO_LONG);
return 0;
goto err;
}

fork_id = openssl_get_fork_id();
Expand All @@ -664,10 +688,10 @@ int ossl_prov_drbg_generate(PROV_DRBG *drbg, unsigned char *out, size_t outlen,
reseed_required = 1;

if (reseed_required || prediction_resistance) {
if (!ossl_prov_drbg_reseed(drbg, prediction_resistance, NULL, 0,
adin, adinlen)) {
if (!ossl_prov_drbg_reseed_unlocked(drbg, prediction_resistance, NULL,
0, adin, adinlen)) {
ERR_raise(ERR_LIB_PROV, PROV_R_RESEED_ERROR);
return 0;
goto err;
}
adin = NULL;
adinlen = 0;
Expand All @@ -676,12 +700,17 @@ int ossl_prov_drbg_generate(PROV_DRBG *drbg, unsigned char *out, size_t outlen,
if (!drbg->generate(drbg, out, outlen, adin, adinlen)) {
drbg->state = EVP_RAND_STATE_ERROR;
ERR_raise(ERR_LIB_PROV, PROV_R_GENERATE_ERROR);
return 0;
goto err;
}

drbg->generate_counter++;

return 1;
ret = 1;
err:
if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);

return ret;
}

/*
Expand Down Expand Up @@ -848,6 +877,10 @@ void ossl_rand_drbg_free(PROV_DRBG *drbg)
OPENSSL_free(drbg);
}

/*
* Helper function called by internal DRBG implementations. Assumes that at
* least a read lock has been taken on drbg->lock
*/
int ossl_drbg_get_ctx_params(PROV_DRBG *drbg, OSSL_PARAM params[])
{
OSSL_PARAM *p;
Expand All @@ -860,10 +893,6 @@ int ossl_drbg_get_ctx_params(PROV_DRBG *drbg, OSSL_PARAM params[])
if (p != NULL && !OSSL_PARAM_set_int(p, drbg->strength))
return 0;

p = OSSL_PARAM_locate(params, OSSL_RAND_PARAM_MAX_REQUEST);
if (p != NULL && !OSSL_PARAM_set_size_t(p, drbg->max_request))
return 0;

p = OSSL_PARAM_locate(params, OSSL_DRBG_PARAM_MIN_ENTROPYLEN);
if (p != NULL && !OSSL_PARAM_set_size_t(p, drbg->min_entropylen))
return 0;
Expand Down Expand Up @@ -900,10 +929,43 @@ int ossl_drbg_get_ctx_params(PROV_DRBG *drbg, OSSL_PARAM params[])
if (p != NULL && !OSSL_PARAM_set_time_t(p, drbg->reseed_time_interval))
return 0;

return 1;
}

/*
* Helper function to get certain params that require no lock to obtain. Sets
* *complete to 1 if all the params were processed, or 0 otherwise
*/
int ossl_drbg_get_ctx_params_no_lock(PROV_DRBG *drbg, OSSL_PARAM params[],
int *complete)
{
size_t cnt = 0;
OSSL_PARAM *p;

/* This value never changes once set */
p = OSSL_PARAM_locate(params, OSSL_RAND_PARAM_MAX_REQUEST);
if (p != NULL) {
if (!OSSL_PARAM_set_size_t(p, drbg->max_request))
return 0;
cnt++;
}

/*
* Can be changed by multiple threads, but we tolerate inaccuracies in this
* value.
*/
p = OSSL_PARAM_locate(params, OSSL_DRBG_PARAM_RESEED_COUNTER);
if (p != NULL
&& !OSSL_PARAM_set_uint(p, tsan_load(&drbg->reseed_counter)))
return 0;
if (p != NULL) {
if (!OSSL_PARAM_set_uint(p, tsan_load(&drbg->reseed_counter)))
return 0;
cnt++;
}

if (params[cnt].key == NULL)
*complete = 1;
else
*complete = 0;

return 1;
}

Expand Down
82 changes: 72 additions & 10 deletions providers/implementations/rands/drbg_ctr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ static OSSL_FUNC_rand_gettable_ctx_params_fn drbg_ctr_gettable_ctx_params;
static OSSL_FUNC_rand_get_ctx_params_fn drbg_ctr_get_ctx_params;
static OSSL_FUNC_rand_verify_zeroization_fn drbg_ctr_verify_zeroization;

static int drbg_ctr_set_ctx_params_locked(void *vctx, const OSSL_PARAM params[]);

/*
* The state of a DRBG AES-CTR.
*/
Expand Down Expand Up @@ -330,11 +332,20 @@ static int drbg_ctr_instantiate_wrapper(void *vdrbg, unsigned int strength,
const OSSL_PARAM params[])
{
PROV_DRBG *drbg = (PROV_DRBG *)vdrbg;
int ret = 0;

if (!ossl_prov_is_running() || !drbg_ctr_set_ctx_params(drbg, params))
if (drbg->lock != NULL && !CRYPTO_THREAD_write_lock(drbg->lock))
return 0;
return ossl_prov_drbg_instantiate(drbg, strength, prediction_resistance,
pstr, pstr_len);

if (!ossl_prov_is_running()
|| !drbg_ctr_set_ctx_params_locked(drbg, params))
goto err;
ret = ossl_prov_drbg_instantiate(drbg, strength, prediction_resistance,
pstr, pstr_len);
err:
if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);
return ret;
}

static int drbg_ctr_reseed(PROV_DRBG *drbg,
Expand Down Expand Up @@ -473,21 +484,41 @@ static int drbg_ctr_uninstantiate(PROV_DRBG *drbg)

static int drbg_ctr_uninstantiate_wrapper(void *vdrbg)
{
return drbg_ctr_uninstantiate((PROV_DRBG *)vdrbg);
PROV_DRBG *drbg = (PROV_DRBG *)vdrbg;
int ret;

if (drbg->lock != NULL && !CRYPTO_THREAD_write_lock(drbg->lock))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth moving the lock/unlock code into drbg.c?
It's the same code & functions for all three DRBGs, so seems like unnecessary duplication.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure it's worth it. It's like one if line for lock and similarly for unlock.

return 0;

ret = drbg_ctr_uninstantiate(drbg);

if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);

return ret;
}

static int drbg_ctr_verify_zeroization(void *vdrbg)
{
PROV_DRBG *drbg = (PROV_DRBG *)vdrbg;
PROV_DRBG_CTR *ctr = (PROV_DRBG_CTR *)drbg->data;
int ret = 0;

if (drbg->lock != NULL && !CRYPTO_THREAD_read_lock(drbg->lock))
return 0;

PROV_DRBG_VERYIFY_ZEROIZATION(ctr->K);
PROV_DRBG_VERYIFY_ZEROIZATION(ctr->V);
PROV_DRBG_VERYIFY_ZEROIZATION(ctr->bltmp);
PROV_DRBG_VERYIFY_ZEROIZATION(ctr->KX);
if (ctr->bltmp_pos != 0)
return 0;
return 1;
goto err;

ret = 1;
err:
if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);
return ret;
}

static int drbg_ctr_init_lengths(PROV_DRBG *drbg)
Expand Down Expand Up @@ -627,20 +658,35 @@ static int drbg_ctr_get_ctx_params(void *vdrbg, OSSL_PARAM params[])
PROV_DRBG *drbg = (PROV_DRBG *)vdrbg;
PROV_DRBG_CTR *ctr = (PROV_DRBG_CTR *)drbg->data;
OSSL_PARAM *p;
int ret = 0, complete = 0;

if (!ossl_drbg_get_ctx_params_no_lock(drbg, params, &complete))
return 0;

if (complete)
return 1;

if (drbg->lock != NULL && !CRYPTO_THREAD_read_lock(drbg->lock))
return 0;

p = OSSL_PARAM_locate(params, OSSL_DRBG_PARAM_USE_DF);
if (p != NULL && !OSSL_PARAM_set_int(p, ctr->use_df))
return 0;
goto err;

p = OSSL_PARAM_locate(params, OSSL_DRBG_PARAM_CIPHER);
if (p != NULL) {
if (ctr->cipher_ctr == NULL
|| !OSSL_PARAM_set_utf8_string(p,
EVP_CIPHER_get0_name(ctr->cipher_ctr)))
return 0;
goto err;
}

return ossl_drbg_get_ctx_params(drbg, params);
ret = ossl_drbg_get_ctx_params(drbg, params);
err:
if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);

return ret;
}

static const OSSL_PARAM *drbg_ctr_gettable_ctx_params(ossl_unused void *vctx,
Expand All @@ -655,7 +701,7 @@ static const OSSL_PARAM *drbg_ctr_gettable_ctx_params(ossl_unused void *vctx,
return known_gettable_ctx_params;
}

static int drbg_ctr_set_ctx_params(void *vctx, const OSSL_PARAM params[])
static int drbg_ctr_set_ctx_params_locked(void *vctx, const OSSL_PARAM params[])
{
PROV_DRBG *ctx = (PROV_DRBG *)vctx;
PROV_DRBG_CTR *ctr = (PROV_DRBG_CTR *)ctx->data;
Expand Down Expand Up @@ -712,6 +758,22 @@ static int drbg_ctr_set_ctx_params(void *vctx, const OSSL_PARAM params[])
return ossl_drbg_set_ctx_params(ctx, params);
}

static int drbg_ctr_set_ctx_params(void *vctx, const OSSL_PARAM params[])
{
PROV_DRBG *drbg = (PROV_DRBG *)vctx;
int ret;

if (drbg->lock != NULL && !CRYPTO_THREAD_write_lock(drbg->lock))
return 0;

ret = drbg_ctr_set_ctx_params_locked(vctx, params);

if (drbg->lock != NULL)
CRYPTO_THREAD_unlock(drbg->lock);

return ret;
}

static const OSSL_PARAM *drbg_ctr_settable_ctx_params(ossl_unused void *vctx,
ossl_unused void *provctx)
{
Expand Down