Re: [Opensc-devel] ECDSA, engine_pkcs11, libp11 and OpenSSL

[Douglas E. E. <dee...@an...>]

On 9/24/2013 12:00 PM, Markus Kötter wrote:
> On 09/24/2013 06:25 PM, Douglas E. Engert wrote:
>>> As comparison let's have a look on the RSA code.
>>> pkcs11_load_key
>>> https://github.com/OpenSC/engine_pkcs11/blob/master/src/engine_pkcs11.c#L541
>>>
>>> If the key is found, all allocated PKCS11_* structures are leaked.
>>> For every private key, public key, certificate loaded.
>>>
>>> This patch:
>>> https://github.com/tkil/engine_pkcs11/commit/91133b719c71d0c92c233a0c29b0d5b94c4ee102
>>> reduces the leak by caching the results, not fixing the real problem but
>>> masking it.
>>
>> I have been looking at the PKCS11_* structures too, including the _private parts.
>>
>> It looks like the PKCS11_TOKEN_private has the arrays of keys and certs,
>> and points to its parent PKCS11_SLOT.
>>
>> The PKCS11_KEY has a pointer to a created EVP_KEY.
>> The PKCS11_CERT has a pointer to the X509.
>>
>> The PKCS11_SLOT_private has info about any PKCS11 session, and points to
>> its parent PKCS11_CTX.
>>
>>
>> But the PKCS11_CTX does not have an array of slots,
>> and the PKCS11_SLOT does not have an array of tokens.
>
> Correct, you can use a single CTX to access multiple keys.
> Such session is a combination of slots and keys.
>
>> On the OpenSSL engine side there are a number of issues.
>> When the engine is loaded from the OpenSSL command, there
>> is no OpenSSL command to unload an engine, thus the engine
>> is never unloaded or keys or certs freeded.
>> (This is not a big issue, as the openssl command will exit...
>> Adding another engine with a bad parameter can cause
>> engine_finish to be called. Not perfect but makes a valgrind
>> report much smaller.)
>
> I attached testcases.

Thanks. I will give these a try. Looks like good cases to test
for memory leaks.


> For a good valgrind backtrace, sometimes you do not want to unload the engine, so the trace is proper.
> Unloading the engine un-mmaps the memory and you get nothing to look at.
>
>> But for engines used in other applications...
>> When an engine returns a key or cert to the caller is is not
>> clear who should free it, the engine of the application.
>
> The EVP_PKEY is owned by the application using EVP_PKEY_free, everything the engine associated with it has to be freed by the engine.

Correct, but as a last resort, the free could be done during the engine_finish
but the since the PKCS11_* structures are not all linked back to the PKCS11_CTX,
there is a problem in finding all of them.

>
>> When a key created by the engine is freed, the engine_finish
>> is called, but an engine could be used to load more then one
>> key, and the engine_finish does not know why it was called.
>
> No.
> The engine's finish is called if the engine is finished.
> That's not related to any EVP_PKEY things, it goes down to the engines structural and functional reference counts.

OK, so they are using reference counts.

>
> ENGINE_by_id s+
> ENGINE_init f+
> ENGINE_finish f-
> ENGINE_free s-
>
>> The engine_finish could free everything it has. To avoid
>> freeing the a cert or key that may still be in use by the
>> application, maybe reference counts could be used.
>
> No, if you ENGINE_finish & ENGINE_free an engine while using it, it is your fault.
>
>> The RSA_METHOD has a finish routine but the ECDSA_METHOD
>> does not which could help in the clean up. (This is Alan's complaint.)
>> maybe we can live without it...
>
> I'm not talking about RSA_METHOD.
>
>> So it looks like your patch above tries to address many of these
>> issues, but adds: "One must instead call a new control function:"
>
>
> That's not my patch, thats the patch which made my patch this on my own.

Sorry, I though that was yours.


> My patch is here:
> https://github.com/commonism/engine_pkcs11/commits/memoryleaks
> https://github.com/commonism/libp11/commits/memoryleaks

I will have to have a look at these.
>
> It gets a callback from openssl on EVP_PKEY_free and free's things.

>
>> Could your mod be combined so an EVP_KEY or X509 returned to the
>> the application had its reference count incremented?
>
> Taken care of by openssl by default.
> Don't touch it.
>
>> Then when engine_finish is called, it would free all the PKCS11
>> and EVP_KEYs and X509s
>> Note that engine_finish might be called more then once.
>
> Same misunderstanding.
>
>> You asked:
>> "If anyone knows how to properly "subclass" EVP_PKEYs from outside the
>> openssl code base, I'd love to learn how."
>
> Not me.

Those comments where in the other patch.

>
>> Is this what you mean...
>> The EVP_KEY type has EVP_PKEY_RSA , EVP_PKEY_EC and others...
>>
>> The libp11 p11_rsa.c and p11_ec.c do things like:
>> if (pk->type == EVP_PKEY_RSA)
>> rsa = EVP_PKEY_get1_RSA(pk)
>> EVP_PKEY_set1_RSA(pk, rsa)
>>
>> ec = EVP_PKEY_get1_EC_KEY(pk)
>> EVP_PKEY_set1_EC_KEY(pk, ec)
>>
>>
>>>
>>> I'd propose to allocate an index for the data using
>>> RSA_get_ex_new_index, and attach all data to the RSA structure using
>>> RSA_set_ex_data.
>>> The destructor provided when creating the index can be used to clean
>>> everything up.
>>>
>>>
>>> The current ECDSA patch do not allocate an index, it just claims index
>>> 0, which is the same as app_data(), collisions with other software are
>>> not that unlikely.
>>
>> The p11_rsa does the same?
>>    RSA_set_app_data(rsa, key);
>
> app_data is basically index 0 but does not register a callback for destruction.

Will have a look.

>
>> The OpenSSL ECDSA_METHOD does not have a finish() so we may have to live
>> without it.
>
> Not required, we do not mess with ECDSA_METHOD.


>
>>> And, I'd propose to have this functionality in engine_pkcs11 instead of
>>> p11. engine_pkcs11 allocates the PKCS11_ structures (by calling p11) and
>>> has to free them in time.
>>> p11 will not even know which RSA/ECDSA to associate the PKCS11_
>>> structures with, as they are allocated way before the getting to the key.
>>
>> But the EVP_KEY is added (or should be added)  to the PKCS11_*
>
> No, the EVP_PKEY is exposed to the user, he will take care of free'ing it via EVP_PKEY_free.
> Once this happens, we have to free the PKCS11_ things.

>
> Seems there was a misunderstanding, I did not author the patches you referred to, and I think he is doing it wrong.
> I'm sorry, could have been more explicit.

Will have to look at your patches. The patch I was looking at was very involved.

>
>
> Here is the logic what I tried to propose:
> compare
> https://github.com/commonism/engine_pkcs11/commit/fbae0727e88fd20e1cba6ec60799dc4fe705cf97
>
> 1) register an index with callback for destruction
> RSA_CRYPTO_EX_idx = RSA_get_ex_new_index(0, "OpenSC PKCS11 RSA key handle", NULL, NULL, PKCS11_RSA_CRYPTO_EX_free);
>
> 2) if a EVP_PKEY is loaded, attach the PKCS11_ data to the key
> +    RSA_set_ex_data(EVP_PKEY_get0(pk),
> +            RSA_CRYPTO_EX_idx,
> +            PKCS11_RSA_CRYPTO_EX_create(ctx, slot_list, slot_count, keys, key_count, selected_key));
>
> 3) we will get the callback once the key is free'd, free things
> +void PKCS11_RSA_CRYPTO_EX_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp)
> +{
> +  if (ptr == NULL || idx != RSA_CRYPTO_EX_idx)
> +    return;
> +  PKCS11_RSA_CRYPTO_EX_destroy(ptr);
>
>
> If you want to compare, use the pkey.c testcase, adjust your slot, id and pin and have it run for 100 and 1000 keys in valgrind.
> Apply my patches, rn again for 100 and 1000 keys.
> There is no more leaking PKCS11_ structures when accessing loading keys - or certs.
> Depending on the number of keys/certs on the card, number of slots, and number of cards the current default (0.13&git) is at least 7kbyte per key/cert.
> Reduced to 0.
>
>
>
> MfG
> Markus
>
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
>
>
>
> _______________________________________________
> Opensc-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/opensc-devel
>

-- 

  Douglas E. Engert  <DEE...@an...>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444