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

Correct EVP_PKEY ownership #535

Merged
merged 8 commits into from
Sep 23, 2019
Merged

Correct EVP_PKEY ownership #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc6b159
Correct EVP_PKEY ownership: soter_asym_cipher
ilammy Sep 20, 2019
9a0c6b1
Correct EVP_PKEY ownership: soter_asym_ka
ilammy Sep 20, 2019
6b2a901
Correct EVP_PKEY ownership: soter_rsa_key_pair_gen
ilammy Sep 20, 2019
92fb4d9
Correct EVP_PKEY ownership: soter_sign_ecdsa
ilammy Sep 20, 2019
f68ad36
Correct EVP_PKEY ownership: soter_sign_rsa
ilammy Sep 20, 2019
878191a
Correct EVP_PKEY ownership: soter_verify_ecdsa
ilammy Sep 20, 2019
74271f9
Correct EVP_PKEY ownership: soter_verify_rsa
ilammy Sep 20, 2019
64a8349
Use EVP_PKEY_assign_EC_KEY
ilammy Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions src/soter/boringssl/soter_asym_cipher.c
View file
Open in desktop
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
Open in desktop
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)) {
ilammy marked this conversation as resolved.
Show resolved Hide resolved
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;
Copy link

Choose a reason for hiding this comment

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

It's ok, but for future better to set err type SOTER_INVALID_PARAMETER in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that we should return SOTER_FAIL in this case. EC_KEY_new_by_curve_name() could fail if the nid argument is invalid, but we’re checking for that here (nid == 0):

themis/src/soter/boringssl/soter_asym_ka.c

Lines 41 to 45 in 74271f9

int nid = soter_alg_to_curve_nid(alg);
if ((!asym_ka_ctx) || (0 == nid)) {
return SOTER_INVALID_PARAMETER;
}

That’s why the most likely reason for failure is some internal error in cryptographic backend. SOTER_INVALID_PARAMETER is used to indicate to the user that they broke some contract for expected function parameters and this error can be avoided by passing correct parameter values. It's used for ‘obvious’ errors like using NULL values where they are not expected, while SOTER_FAIL usually indicates deeper issues with arguments, or acts like a generic catch-all error code.

Copy link

Choose a reason for hiding this comment

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

Okay. I agree

}

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
Open in desktop
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
Open in desktop
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
Open in desktop
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);
Copy link

Choose a reason for hiding this comment

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

In this plase we get dereferencing a pointer. It would be good to check ctx before.

Copy link

Choose a reason for hiding this comment

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

This function is not in SOTER_PRIVATE_API and i think that it would be good make that check. Same in soter_sign_final_ecdsa_none_pkcs8 (in this file)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Functions that are not marked SOTER_API are considered to be private to Soter and they can make assumptions about arguments. Usually we check arguments for NULL in public functions and let the private ones assume that the arguments are more or less sane.

In this case soter_sign_init_ecdsa_none_pkcs8() is called by soter_sign_init() which does check that ctx is non-NULL beforehand. It’s also checked a couple of times more up the call stack, so we can be sure that it won’t be NULL in soter_sign_init_ecdsa_none_pkcs8().

SOTER_PRIVATE_API is kind of a hack that intentionally exposes private API for Themis. It’s not really pretty, but some code currently calls such functions and makes necessary precautions to ensure that they are safe to call. At some point we intend to get rid of SOTER_PRIVATE_API to avoid confusion over what’s public and what’s private.

Copy link

Choose a reason for hiding this comment

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

Thanks for your explanation. Now I have knowledge how this visibility works. This kind of bug is not connected with PR. But you have such functions in SOTER_API. Ex. is soter_rsa_key_pair_gen_init(). This is notice to be cerfull with this.

Copy link

Choose a reason for hiding this comment

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

Mb it will be good to make some rule how to work with this.

Copy link

Choose a reason for hiding this comment

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

Ok, with soter_rsa_key_pair_gen_init() is bad example (becouse it should be not in SOTER_API). But if we look at typtcal usage any public funtion it would be checked twice ore even more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, C does not really provide any amenities for null checks. Theoretically, we could use __attribute__((nonnull)) or something like that, but that’s not really portable and too much of a bother. Generally, the rules are that public functions (marked with SOTER_API or THEMIS_API) have to do paranoid thorough parameter checking while internal functions may expect the parameters to have reasonable values. And generally, it does not really hurt if internal functions have duplicated checks. Better safe than segfault.

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