Skip to content

Commit

Permalink
Correct EVP_PKEY ownership (#535)
Browse files Browse the repository at this point in the history 
* Correct EVP_PKEY ownership: soter_asym_ka
* Correct EVP_PKEY ownership: soter_asym_cipher
* Correct EVP_PKEY ownership: soter_rsa_key_pair_gen
* Correct EVP_PKEY ownership: soter_sign_ecdsa
* Correct EVP_PKEY ownership: soter_sign_rsa
* Correct EVP_PKEY ownership: soter_verify_ecdsa
* Correct EVP_PKEY ownership: soter_verify_rsa

We've got a bunch of functions which are currently doing tricky things
with EVP_PKEY and EVP_PKEY_CTX objects, sometimes incorrectly. Let's
make sure that the code is readable, that we don't leak objects in the
failure path, and that we don't free objects twice.

The general rules we follow are:

1. The function allocating the object is the one that frees it.

2. Unless the ownership is *transferred* to some other object,
   in which case we must not free the transferred object: the new
   owner is responsible for that. This includes constructors.

3. Set the field to NULL after freeing the object stored there.
   (It's okay to leave local variables as is.)

They all seem obvious in the hindsight, but the current code contains
a number of violations in these rules. Partially, due to magic macros
which make it *seem* like the error handling and resource management
are correct, while in fact they are not. Therefore we get rid of the
macros and replace them with plain old conditions.

We use "goto" pattern in order to make destruction of objects more
structured. Yeah, "goto" and "structured" in the same sentence,
but this is one of the few cases where using goto makes sense in C.
(We'd use proper destructors in modern language, but we aren't.)

Since we use goto, this may have weird effects on variable scoping
and initialization. Move all variable declarations to the top of
the function and make sure that all local variables are initialized.

Some of these changes make Soter return more accurate error codes:
for example, SOTER_INVALID_PARAMETER instead of SOTER_FAIL when the
issue is actually in the parameter. Update the test suite to expect
the new error codes. Strictly speaking, this *does* break backwards
compatibility (or at least, may break someone's weird use case),
but I feel that it's acceptable to make this change. Usually,
applications don't check concrete error values, and if they do they
expect to catch parameter errors that way then we should return
appropriate error codes.

* Use EVP_PKEY_assign_EC_KEY

We don't really need to retain the EC_KEY instances in these
cases so instead of calling EVP_PKEY_set1_EC_KEY() we can call
EVP_PKEY_assign_EC_KEY() which avoids unnecessary refcounting
in the happy path.

Investigating uses of EVP_PKEY_set1_EC_KEY() has uncovered a possible
memory leak in soter_ec_gen_key() when an error occurs.
  • Loading branch information
@ilammy
ilammy authored Sep 23, 2019
1 parent 340bf54 commit 3369780
Show file tree
Hide file tree
Showing 16 changed files with 478 additions and 318 deletions.
31 changes: 20 additions & 11 deletions src/soter/boringssl/soter_asym_cipher.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ soter_status_t soter_asym_cipher_init(soter_asym_cipher_t* asym_cipher,
const size_t key_length,
soter_asym_cipher_padding_t pad)
{
EVP_PKEY* pkey;
soter_status_t err = SOTER_FAIL;
EVP_PKEY* pkey = NULL;

if ((!asym_cipher) || (SOTER_ASYM_CIPHER_OAEP != pad)) {
return SOTER_INVALID_PARAMETER;
Expand All @@ -90,18 +91,29 @@ soter_status_t soter_asym_cipher_init(soter_asym_cipher_t* asym_cipher,

/* Only RSA supports asymmetric encryption */
if (!EVP_PKEY_set_type(pkey, EVP_PKEY_RSA)) {
EVP_PKEY_free(pkey);
return SOTER_FAIL;
goto free_pkey;
}

asym_cipher->pkey_ctx = EVP_PKEY_CTX_new(pkey, NULL);
if (!(asym_cipher->pkey_ctx)) {
EVP_PKEY_free(pkey);
return SOTER_FAIL;
err = SOTER_NO_MEMORY;
goto free_pkey;
}

err = soter_asym_cipher_import_key(asym_cipher, key, key_length);
if (err != SOTER_SUCCESS) {
goto free_pkey_ctx;
}
SOTER_IF_FAIL(soter_asym_cipher_import_key(asym_cipher, key, key_length) == SOTER_SUCCESS,
(EVP_PKEY_free(pkey), EVP_PKEY_CTX_free(asym_cipher->pkey_ctx)));

EVP_PKEY_free(pkey);
return SOTER_SUCCESS;

free_pkey_ctx:
EVP_PKEY_CTX_free(asym_cipher->pkey_ctx);
asym_cipher->pkey_ctx = NULL;
free_pkey:
EVP_PKEY_free(pkey);
return err;
}

SOTER_PRIVATE_API
Expand All @@ -112,11 +124,8 @@ soter_status_t soter_asym_cipher_cleanup(soter_asym_cipher_t* asym_cipher)
}

if (asym_cipher->pkey_ctx) {
EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(asym_cipher->pkey_ctx);
if (pkey) {
EVP_PKEY_free(pkey);
}
EVP_PKEY_CTX_free(asym_cipher->pkey_ctx);
asym_cipher->pkey_ctx = NULL;
}

return SOTER_SUCCESS;
Expand Down
40 changes: 23 additions & 17 deletions src/soter/boringssl/soter_asym_ka.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ static int soter_alg_to_curve_nid(soter_asym_ka_alg_t alg)
SOTER_PRIVATE_API
soter_status_t soter_asym_ka_init(soter_asym_ka_t* asym_ka_ctx, soter_asym_ka_alg_t alg)
{
EVP_PKEY* pkey;
soter_status_t err = SOTER_FAIL;
EVP_PKEY* pkey = NULL;
EC_KEY* ec = NULL;
int nid = soter_alg_to_curve_nid(alg);

if ((!asym_ka_ctx) || (0 == nid)) {
Expand All @@ -48,28 +50,35 @@ soter_status_t soter_asym_ka_init(soter_asym_ka_t* asym_ka_ctx, soter_asym_ka_al
}

if (!EVP_PKEY_set_type(pkey, EVP_PKEY_EC)) {
EVP_PKEY_free(pkey);
return SOTER_FAIL;
goto free_pkey;
}

asym_ka_ctx->pkey_ctx = EVP_PKEY_CTX_new(pkey, NULL);
if (!(asym_ka_ctx->pkey_ctx)) {
EVP_PKEY_free(pkey);
return SOTER_FAIL;
err = SOTER_NO_MEMORY;
goto free_pkey;
}
EC_KEY* ec = EC_KEY_new_by_curve_name(nid);

ec = EC_KEY_new_by_curve_name(nid);
if (!ec) {
EVP_PKEY_CTX_free(asym_ka_ctx->pkey_ctx);
return SOTER_FAIL;
goto free_pkey_ctx;
}

if (1 != EVP_PKEY_set1_EC_KEY(pkey, ec)) {
EVP_PKEY_CTX_free(asym_ka_ctx->pkey_ctx);
EC_KEY_free(ec);
return SOTER_FAIL;
if (EVP_PKEY_assign_EC_KEY(pkey, ec) != 1) {
goto free_ec_key;
}
EC_KEY_free(ec);

EVP_PKEY_free(pkey);
return SOTER_SUCCESS;

free_ec_key:
EC_KEY_free(ec);
free_pkey_ctx:
EVP_PKEY_CTX_free(asym_ka_ctx->pkey_ctx);
asym_ka_ctx->pkey_ctx = NULL;
free_pkey:
EVP_PKEY_free(pkey);
return err;
}

SOTER_PRIVATE_API
Expand All @@ -79,11 +88,8 @@ soter_status_t soter_asym_ka_cleanup(soter_asym_ka_t* asym_ka_ctx)
return SOTER_INVALID_PARAMETER;
}
if (asym_ka_ctx->pkey_ctx) {
EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(asym_ka_ctx->pkey_ctx);
if (pkey) {
EVP_PKEY_free(pkey);
}
EVP_PKEY_CTX_free(asym_ka_ctx->pkey_ctx);
asym_ka_ctx->pkey_ctx = NULL;
}
return SOTER_SUCCESS;
}
Expand Down
11 changes: 4 additions & 7 deletions src/soter/boringssl/soter_ecdsa_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,18 @@ soter_status_t soter_ec_gen_key(EVP_PKEY_CTX* pkey_ctx)
if (EVP_PKEY_EC != EVP_PKEY_id(pkey)) {
return SOTER_INVALID_PARAMETER;
}
/* ec = EVP_PKEY_get0_EC_KEY(pkey); */
/* if (NULL == ec){ */
/* return SOTER_INVALID_PARAMETER; */
/* } */
ec = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
if (!ec) {
return SOTER_ENGINE_FAIL;
}
if (!EC_KEY_generate_key(ec)) {
if (EC_KEY_generate_key(ec) != 1) {
EC_KEY_free(ec);
return SOTER_ENGINE_FAIL;
}
if (!EVP_PKEY_set1_EC_KEY(pkey, ec)) {
if (EVP_PKEY_assign_EC_KEY(pkey, ec) != 1) {
EC_KEY_free(ec);
return SOTER_ENGINE_FAIL;
}
EC_KEY_free(ec);
return SOTER_SUCCESS;
}

Expand Down
40 changes: 30 additions & 10 deletions src/soter/boringssl/soter_rsa_key_pair_gen.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,27 +35,47 @@ soter_rsa_key_pair_gen_t* soter_rsa_key_pair_gen_create(const unsigned key_lengt

soter_status_t soter_rsa_key_pair_gen_init(soter_rsa_key_pair_gen_t* ctx, const unsigned key_length)
{
EVP_PKEY* pkey;
soter_status_t err = SOTER_FAIL;
EVP_PKEY* pkey = NULL;

pkey = EVP_PKEY_new();
SOTER_CHECK(pkey);
if (!pkey) {
return SOTER_NO_MEMORY;
}

/* Only RSA supports asymmetric encryption */
SOTER_IF_FAIL(EVP_PKEY_set_type(pkey, EVP_PKEY_RSA), EVP_PKEY_free(pkey));
if (EVP_PKEY_set_type(pkey, EVP_PKEY_RSA) != 1) {
goto free_pkey;
}

ctx->pkey_ctx = EVP_PKEY_CTX_new(pkey, NULL);
SOTER_IF_FAIL(ctx->pkey_ctx, EVP_PKEY_free(pkey));
SOTER_IF_FAIL(soter_rsa_gen_key(ctx->pkey_ctx, key_length) == SOTER_SUCCESS,
EVP_PKEY_CTX_free(ctx->pkey_ctx));
if (!ctx->pkey_ctx) {
err = SOTER_NO_MEMORY;
goto free_pkey;
}

err = soter_rsa_gen_key(ctx->pkey_ctx, key_length);
if (err != SOTER_SUCCESS) {
goto free_pkey_ctx;
}

EVP_PKEY_free(pkey);
return SOTER_SUCCESS;

free_pkey_ctx:
EVP_PKEY_CTX_free(ctx->pkey_ctx);
ctx->pkey_ctx = NULL;
free_pkey:
EVP_PKEY_free(pkey);
return err;
}

soter_status_t soter_rsa_key_pair_gen_cleanup(soter_rsa_key_pair_gen_t* ctx)
{
SOTER_CHECK_PARAM(ctx);
if (ctx->pkey_ctx) {
EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx->pkey_ctx);
if (pkey) {
EVP_PKEY_free(pkey);
}
EVP_PKEY_CTX_free(ctx->pkey_ctx);
ctx->pkey_ctx = NULL;
}
return SOTER_SUCCESS;
}
Expand Down
58 changes: 34 additions & 24 deletions src/soter/boringssl/soter_sign_ecdsa.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,52 +30,66 @@ soter_status_t soter_sign_init_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx,
const void* public_key,
const size_t public_key_length)
{
/* pkey_ctx init */
EVP_PKEY* pkey;
soter_status_t err = SOTER_FAIL;
EVP_PKEY* pkey = NULL;

pkey = EVP_PKEY_new();
if (!pkey) {
return SOTER_NO_MEMORY;
}

if (!EVP_PKEY_set_type(pkey, EVP_PKEY_EC)) {
EVP_PKEY_free(pkey);
return SOTER_FAIL;
goto free_pkey;
}

ctx->pkey_ctx = EVP_PKEY_CTX_new(pkey, NULL);
if (!(ctx->pkey_ctx)) {
EVP_PKEY_free(pkey);
return SOTER_FAIL;
err = SOTER_NO_MEMORY;
goto free_pkey;
}

if ((!private_key) && (!public_key)) {
if (soter_ec_gen_key(ctx->pkey_ctx) != SOTER_SUCCESS) {
soter_sign_cleanup_ecdsa_none_pkcs8(ctx);
return SOTER_FAIL;
err = soter_ec_gen_key(ctx->pkey_ctx);
if (err != SOTER_SUCCESS) {
goto free_pkey_ctx;
}
} else {
if (private_key != NULL) {
if (soter_ec_import_key(pkey, private_key, private_key_length) != SOTER_SUCCESS) {
soter_sign_cleanup_ecdsa_none_pkcs8(ctx);
return SOTER_FAIL;
err = soter_ec_import_key(pkey, private_key, private_key_length);
if (err != SOTER_SUCCESS) {
goto free_pkey_ctx;
}
}
if (public_key != NULL) {
if (soter_ec_import_key(pkey, public_key, public_key_length) != SOTER_SUCCESS) {
soter_sign_cleanup_ecdsa_none_pkcs8(ctx);
return SOTER_FAIL;
err = soter_ec_import_key(pkey, public_key, public_key_length);
if (err != SOTER_SUCCESS) {
goto free_pkey_ctx;
}
}
}

/*md_ctx init*/
ctx->md_ctx = EVP_MD_CTX_create();
if (!(ctx->md_ctx)) {
soter_sign_cleanup_ecdsa_none_pkcs8(ctx);
return SOTER_NO_MEMORY;
err = SOTER_NO_MEMORY;
goto free_pkey_ctx;
}

if (EVP_DigestSignInit(ctx->md_ctx, NULL, EVP_sha256(), NULL, pkey) != 1) {
soter_sign_cleanup_ecdsa_none_pkcs8(ctx);
return SOTER_FAIL;
goto free_md_ctx;
}

EVP_PKEY_free(pkey);
return SOTER_SUCCESS;

free_md_ctx:
EVP_MD_CTX_destroy(ctx->md_ctx);
ctx->md_ctx = NULL;
free_pkey_ctx:
EVP_PKEY_CTX_free(ctx->pkey_ctx);
ctx->pkey_ctx = NULL;
free_pkey:
EVP_PKEY_free(pkey);
return err;
}

soter_status_t soter_sign_export_key_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx,
Expand Down Expand Up @@ -129,10 +143,6 @@ soter_status_t soter_sign_cleanup_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx)
ctx->md_ctx = NULL;
}
if (ctx->pkey_ctx) {
EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx->pkey_ctx);
if (pkey) {
EVP_PKEY_free(pkey);
}
EVP_PKEY_CTX_free(ctx->pkey_ctx);
ctx->pkey_ctx = NULL;
}
Expand Down
Loading

0 comments on commit 3369780

Please sign in to comment.