Skip to content

Commit

Permalink
Correctly free EVP_MD_CTX in OpenSSL (#501)
Browse files Browse the repository at this point in the history
soter_sign_ctx_t structures store both EVP_MD_CTX (to keep the digest
algorithm used for signatures) and EVP_PKEY_CTX (to keep the key used
for signature). The key is actually shared between those structures
but EVP_MD_CTX assumes ownership over it, with EVP_MD_CTX_destroy()
freeing the key. EVP_PKEY structures are refcounted so sharing the
key should be safe... if we make the correct precautions to avoid
freeing the key after it has been freed. EVP_PKEY_CTX keeps track
of that, but we need to free EVP_MD_CTX first and then proceed to
freeing EVP_PKEY_CTX and maybe the key it has been managing.

This sleeper bug is presumed to be responsible for occasional segfaults
when running JsThemis. Interestingly, the crashes do not reproduce with
any other wrapper. Furthermore, BoringSSL code uses correct freeing
order since the beginning. "Coincidence? I don't think so".
  • Loading branch information
ilammy authored Jul 22, 2019
1 parent eb6388e commit 914d799
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
8 changes: 4 additions & 4 deletions src/soter/openssl/soter_sign_ecdsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ soter_status_t soter_sign_cleanup_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx)
if (!ctx) {
return SOTER_INVALID_PARAMETER;
}
if (ctx->md_ctx) {
EVP_MD_CTX_destroy(ctx->md_ctx);
ctx->md_ctx = NULL;
}
if (ctx->pkey_ctx) {
EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx->pkey_ctx);
if (pkey) {
Expand All @@ -144,9 +148,5 @@ soter_status_t soter_sign_cleanup_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx)
EVP_PKEY_CTX_free(ctx->pkey_ctx);
ctx->pkey_ctx = NULL;
}
if (ctx->md_ctx) {
EVP_MD_CTX_destroy(ctx->md_ctx);
ctx->md_ctx = NULL;
}
return SOTER_SUCCESS;
}
8 changes: 4 additions & 4 deletions src/soter/openssl/soter_verify_ecdsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ soter_status_t soter_verify_cleanup_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx)
if (!ctx) {
return SOTER_INVALID_PARAMETER;
}
if (ctx->md_ctx) {
EVP_MD_CTX_destroy(ctx->md_ctx);
ctx->md_ctx = NULL;
}
if (ctx->pkey_ctx) {
EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx->pkey_ctx);
if (pkey) {
Expand All @@ -120,9 +124,5 @@ soter_status_t soter_verify_cleanup_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx)
EVP_PKEY_CTX_free(ctx->pkey_ctx);
ctx->pkey_ctx = NULL;
}
if (ctx->md_ctx) {
EVP_MD_CTX_destroy(ctx->md_ctx);
ctx->md_ctx = NULL;
}
return SOTER_SUCCESS;
}

0 comments on commit 914d799

Please sign in to comment.