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

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

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

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

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.
My patch is here:
https://github.com/commonism/engine_pkcs11/commits/memoryleaks
https://github.com/commonism/libp11/commits/memoryleaks

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.

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

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


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