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.

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

ENGINE_get_pkey_meths() fails for EC keys #243

Closed
ansasaki opened this issue Aug 23, 2018 · 2 comments
Closed

ENGINE_get_pkey_meths() fails for EC keys #243

ansasaki opened this issue Aug 23, 2018 · 2 comments
Assignees

Comments

@ansasaki
Copy link
Contributor

This bug was originally reported in https://bugzilla.redhat.com/show_bug.cgi?id=1619184

The root cause for the bug above is that ENGINE_set1_engine() is called in load_privkey(), but its return value is not checked. The failure in ENGINE_get_pkey_meths(), called internally in ENGINE_set1_engine(), is ignored.

So the returned EVP_PKEY does not contain a reference to the engine in pkey_meths (but contains all other data necessary to perform signatures).

When ENGINE_finish() is called, the reference counter for the engine reaches zero, and the structures are freed.

Then, when the openssl application exits, the destructors are called and they try to free the engine structures again, causing a double free and core dump.

The fix is to make ENGINE_get_pkey_meths() to report properly if the PKCS11 module supports EC methods.

@dengert
Copy link
Member

dengert commented Aug 23, 2018

Do you mean EVP_PKEY_set1_engine rather then ENGINE_set1_engine?

Or are you suggesting that EVP_PKEY_set1_engine should not be called for EC keys because
the EC KEYS processing is at a lower level then the engine?

Are you suggesting something like this (that may leak memory)? */

diff --git a/src/eng_front.c b/src/eng_front.c
index 853fa5a..ea44105 100644
--- a/src/eng_front.c
+++ b/src/eng_front.c
@@ -234,7 +234,10 @@ static EVP_PKEY *load_privkey(ENGINE *engine, const char *s_key_id,
        /* EVP_PKEY_set1_engine() is required for OpenSSL 1.1.x,
         * but otherwise setting pkey->engine breaks OpenSSL 1.0.2 */
        if (pkey)
-               EVP_PKEY_set1_engine(pkey, engine);
+               if (!EVP_PKEY_set1_engine(pkey, engine) {
+                       openssl_free(pkey);
+                       pkey = NULL;
+               }
 #endif /* EVP_F_EVP_PKEY_SET1_ENGINE */
        return pkey;
 }

@mtrojnar mtrojnar self-assigned this Aug 23, 2018
@mtrojnar
Copy link
Member

@ansasaki Great analysis. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants