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

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

On 9/21/2013 4:45 PM, Markus Kötter wrote:
> 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.

Thanks for the comments. I was not convinced either.
>
> 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.

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.)

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.

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.

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.

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...

So it looks like your patch above tries to address many of these
issues, but adds: "One must instead call a new control function:"


Could your mod be combined so an EVP_KEY or X509 returned to the
the application had its reference count incremented?

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.

You asked:
"If anyone knows how to properly "subclass" EVP_PKEYs from outside the
openssl code base, I'd love to learn how."

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);

> 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.

The OpenSSL ECDSA_METHOD does not have a finish() so we may have to live
without it.

Your mods combined with the engine_finish could handle this.


>
> 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.

But the EVP_KEY is added (or should be added)  to the PKCS11_*

> 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
>
>
> ------------------------------------------------------------------------------
> LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
> 1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
> 2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
> Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/22/13.
> http://pubads.g.doubleclick.net/gampad/clk?id=64545871&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