Re: [Opensc-devel] memory leaks with engine_pkcs11

[Anthony F. <ant...@gm...>]

Greetings, all --

On Thu, May 2, 2013 at 10:25 AM, Anthony Foiani
<ant...@gm...> wrote:
>
>
> On Thu, May 2, 2013 at 10:15 AM, Anthony Foiani
> <ant...@gm...> wrote:
>
> > Or does the private key structure rely on information held in
> > the enumeration?
>
> So apparently it does.
>
> > If the latter, do we need to somehow provide a way for the key to
> > release the entire enumeration when the key itself is freed?

I have a solution, but it's very ugly.

Please take a look at this branch:

  https://github.com/tkil/engine_pkcs11/commits/ajf-fixes-201305

It's mostly cleanup patches, but the punchline is in this commit:

  https://github.com/tkil/engine_pkcs11/commit/91133b719c71d0c92c233a0c29b0d5b94c4ee102
  (or: http://preview.tinyurl.com/cdabyyf)

The commit message tells the story:

Without this patch:

  ==25864== LEAK SUMMARY:
  ==25864==    definitely lost: 4,920 bytes in 27 blocks
  ==25864==    indirectly lost: 147,092 bytes in 3,137 blocks
  ==25864==      possibly lost: 0 bytes in 0 blocks
  ==25864==    still reachable: 14,231 bytes in 429 blocks
  ==25864==         suppressed: 0 bytes in 0 blocks

With this patch:

  ==25989== LEAK SUMMARY:
  ==25989==    definitely lost: 4,600 bytes in 23 blocks
  ==25989==    indirectly lost: 328 bytes in 5 blocks
  ==25989==      possibly lost: 0 bytes in 0 blocks
  ==25989==    still reachable: 14,231 bytes in 429 blocks
  ==25989==         suppressed: 0 bytes in 0 blocks

Note the change in "indirectly lost".

The ugly part is that I could not find any way to do it transparently.

When we get a key through the OpenSSL engine interface, all we get
back is an EVP_PKEY*.  I looked, and it doesn't seem that there's any
way to add on arbitrary data; even if there were, there's no obvious
way for external/third-party programs to modify the set of methods
used by a given key.

If there were, the right answer would have been:

1. Add the pointer to list of slots, and count of slots, as auxiliary
data on the EVP_PKEY;
2. Modify the pkey_free method to free those and then chain to the original.

I couldn't find anywhere to stick (1), and (2) was blocked because
that method is behind an opaque pointer (the structure itself is
defined in the openssl source package under crypto/asn1/asn1_locl.h,
but that is not installed into the system headers directory, and it's
explicitly mentioned that it's not for external use.)

There are ways to get a pointer to the methods, duplicate it, and set
a new free function -- but no way to get the original free function
(!), so I couldn't chain to the original.

Maybe I just missed something.

It looks like someone else had a similar idea; in the hw_pcsk11.c
file, there was an "#if 0" block with something that tries to chain
the rsa_finish method ... but it was obviously not in use.

Instead, I modified the engine to keep a separate list of EVP_PKEY* ->
slot_list, slot_count mappings, and added an ENGINE_ctrl method to
release the slot data given an EVP_PKEY*.

I thought that I would still have to free the EVP_PKEY*, but it turns
out that freeing the slots frees the keys:

PKCS11_release_all_slots calls
pkcs11_release_slot, which calls
pkcs11_destroy_token, which calls
pkcs11_destroy_keys, which calls
EVP_PKEY_free

This is also the reason that I could not just release the slots before
returning the EVP_PKEY* out of engine_pkcs11.c:pkcs11_load_key

Anyway.  Hopefully others will find this useful.  I can submit a
proper pull request if you would like.

Best regards,
Anthony Foiani