OpenSC / libp11

PKCS#11 wrapper library
GNU Lesser General Public License v2.1
310 stars 189 forks source link

Private key received via ENGINE_load_private_key from pkcs11.so ENGINE not free’d by EVP_PKEY_free #522

Closed tougantium closed 2 weeks ago

tougantium commented 11 months ago

Hi, I get an EVP_PKEY from the pkcs11 ENGINE using the following code:

ENGINE *e = ENGINE_by_id("pkcs11");

if(e == NULL) {
    printf("pkcs11 ENGINE is NULL\n");
    return;
}

if(!ENGINE_init(e)) {
    printf("unable to initialize pkcs11 ENGINE\n");
    ENGINE_free(e);
    return;
}

EVP_PKEY *key = ENGINE_load_private_key(e, "1234", NULL, NULL);
if(key == NULL) {
    printf("Unable to load private key from ENGINE\n");
    ENGINE_finish(e);
    ENGINE_free(e);
    return;
}

/*use key*/

EVP_PKEY_free(key);
ENGINE_finish(e);
ENGINE_free(e);

After using the key, I free the key and ENGINE by calling the appropriate free/finish functions. However, what I notice is that the neither the key nor the ENGINE are actually free’d. When I step through the code with gdb I get after the last line:

(gdb) p e->struct_ref 
$1 = 2
(gdb) p e->funct_ref 
$2 = 1
(gdb) p key->references 
$1 = 1

So the ref counts of the ENGINE (structural and functional) and of the key are > 0. So neither the ENGINE nor the key are actually free’d.

After a quick look at the libp11 code, I got the impression that the ENGINE holds a reference to the key and the key holds a reference to the ENGINE (in the pmeth_engine field). If this is the case, it would be a cyclic reference that prevents ENGINE and key from being free’d.

If I add the line EVP_PKEY_set1_engine(key, NULL); just before the line EVP_PKEY_free(key); everything seems to work as expected. The EVP_PKEY_set1_engine function finishes the ENGINE referenced in the pmeth_engine field (reduces the ref counts of the ENGINE) and sets the pmeth_engine field to NULL, thus breaking the cycle.

After calling the free/finish functions as before, the ENGINE is free’d first, which ultimately also frees the key via its internal reference.

Am I missing some obvious point here? If so, could you please show me how to clean up correctly?

If my interpretation is correct and there is no straightforward cleanup strategy, I would find it irritating to be responsible for manually breaking the cycle to get a the desired cleanup.

The observed behavior could be produced with openSSL 1.1.1w and openSSL 3.0.2 and libp11 0.4.12.

I appreciate your help.

olszomal commented 2 weeks ago

This memory leak was fixed in PR #550 and is no longer present in the latest master branch. I recommend closing this issue.