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

[Markus K. <ko...@rr...>]

On 09/19/2013 10:31 PM, Douglas E. Engert wrote:
> Modifications to engine_pkcs11 and libp11 to support ECDSA
> are available at github for testing, and I am looking for
> comments.

I see the use of ECDSA_set_ex_data to associate the PKCS11_KEY with the 
EC_KEY/EVP_PKEY.
Yet, I'm not convinced this can free the resources claimed when loading 
a key.

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'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.
If 0 is used by something else as app_data already, you'll run into 
problems. Additionally, while index 0 is reserved for app_data, OpenSSL 
still returns it as valid entry.

The current ECDSA patch do not provide a destructor, so it is 'unlikely' 
the complete PKCS11_ structure set is free'd, most likely either just 
the PKCS11_KEY associated with the EC_KEY, or nothing at all as there is 
no destructor.

Internally EC_KEY has EC_KEY_insert_key_method_data which can be used 
with EC_EX_DATA_set_data, EC_KEY_insert_key_method_data is visible 
externally as well and could be used instead of ECDSA_get_ex_new_index.

Still, I'd use ECDSA_get_ex_new_index/ECDSA_set_ex_data to have the same 
code path as -to be- used by RSA.

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.
Once the key is loaded, the engine can attach all relevant data to the key.

And I even tried to do it, n with ECDSA but fixing the memory leaks of 
the RSA code.
Problems ...

idx 0 - which is returned by default, is used by p11 already as app_data.

ex data slots claimed with RSA_get_ex_new_index last forever.
Unload the engine, there is no way to drop the slot, so the callback 
will be called, if the memory is unmapped as the engine is unloaded, you 
are lost.
Reload the engine, get mapped in the same location, you get the 
callbacks for the unmapped engines as well.
The API exposed by OpenSSLs ex_data.c is not sufficient to remove the 
slot manually.

Calling PKCS11_release_all_slots with the required arguments in the ex 
data destructor of an EVP_PKEY/RSA results in recursion, as 
PKCS11_release_all_slots calls EVP_PKEY_free as well.


Ideas?


MfG
Markus Kötter