Memory leaks with libp11 and engine-pkcs11

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Memory leaks with libp11 and engine-pkcs11

Douglas E. Engert
Anthony, Markus,
Both of you have submitted mods to libp11 and/or engine_pkcs11 to
cut down on memory leaks.
The main problem both of you are trying to address is
how to keep track of the extra structures that libp11 creates
so they can be cleaned up when the key or cert is freed
by the caller.

I would encourage both of you to work together on this.

Since the libp11 can (or could) be used outside of the engine
code, I would like to see the code to keep track of these
structures in libp11 and not in the engine.

I don't like the use of the new control function:
ENGINE_ctrl_cmd( engine, "RELEASE_KEY", 0,
                    (void *)pkey, NULL, 0 );


In Markus's code, you complained that the libp11 was using the
RSA_set_app_data and it was getting in the way of you using
the ex_data. But if you look closely, what libp11 is doing
is saving the pointer to the, PKCS11_KEY so the RSA sign, encrypt,
and decrypt functions can find the PKCS11_KEY.

In the PKCS11_RSA_CRYPTO_EX structure in the engine which you
store using the RSA_set_ex_data, has a pointer to the same
PKCS11_KEY that the libp11 needs.
So a single ex_data index could be used if the code to keep
track of all of this was in the libp11.

The  PKCS11_RSA_CRYPTO_EX is not particular to RSA.
and  could also be used for ECDSA and any other key types
we add in the future. So a more generic name would be better.
(It still needs to be added to the RSA, ECDSA, or what ever
using the *_ex_data for that type of key.)

(The ECDSA code I have working uses the ECDSA_set_ex_data
like the RSA_set_ex_data to store a pointer to the PKCS11_KEY.
Thus it would use the new version of the PKCS11_RSA_CRYPTO_EX.



--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

------------------------------------------------------------------------------
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Markus Kötter
Hi,

On 09/26/2013 06:48 PM, Douglas E. Engert wrote:
> In Markus's code, you complained that the libp11 was using the
> RSA_set_app_data and it was getting in the way of you using
> the ex_data. But if you look closely, what libp11 is doing
> is saving the pointer to the, PKCS11_KEY so the RSA sign, encrypt,
> and decrypt functions can find the PKCS11_KEY.

I was not really complaining, but ... lets say surprised, as app_data is
a #define to ex_data with index 0 and ... registering the first ex_data
hands you 0 as index, so initially my callback was free'ing p11 resources.
I'd complain if my application, which is supposed to use app_data would
clash with p11, but I do not have an application using app_data.

> In the PKCS11_RSA_CRYPTO_EX structure in the engine which you
> store using the RSA_set_ex_data, has a pointer to the same
> PKCS11_KEY that the libp11 needs.
> So a single ex_data index could be used if the code to keep
> track of all of this was in the libp11.

Yes, but in the engine loading the key happens, and everything related
to the key is available.
Which is why I put things there.
In p11 you do not know which slots relate to which key.
If you want tracking in p11, loading keys and certs (by slot/id, label)
has to happen in p11, not in the engine.
Traversing things backwards from the key to the slots won't work.
The engine can call the p11 function, but p11 has to do the lifting.

If you think that is an acceptable change, let me know.

> The  PKCS11_RSA_CRYPTO_EX is not particular to RSA.
> and  could also be used for ECDSA and any other key types
> we add in the future. So a more generic name would be better.
> (It still needs to be added to the RSA, ECDSA, or what ever
> using the *_ex_data for that type of key.)

Renaming to PKCS11_CRYPTO_EX is trivial.

> (The ECDSA code I have working uses the ECDSA_set_ex_data
> like the RSA_set_ex_data to store a pointer to the PKCS11_KEY.
> Thus it would use the new version of the PKCS11_RSA_CRYPTO_EX.



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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Douglas E. Engert


On 9/26/2013 12:07 PM, Markus Koetter wrote:

> Hi,
>
> On 09/26/2013 06:48 PM, Douglas E. Engert wrote:
>> In Markus's code, you complained that the libp11 was using the
>> RSA_set_app_data and it was getting in the way of you using
>> the ex_data. But if you look closely, what libp11 is doing
>> is saving the pointer to the, PKCS11_KEY so the RSA sign, encrypt,
>> and decrypt functions can find the PKCS11_KEY.
>
> I was not really complaining, but ... lets say surprised, as app_data is
> a #define to ex_data with index 0 and ... registering the first ex_data
> hands you 0 as index, so initially my callback was free'ing p11 resources.
> I'd complain if my application, which is supposed to use app_data would
> clash with p11, but I do not have an application using app_data.
>
>> In the PKCS11_RSA_CRYPTO_EX structure in the engine which you
>> store using the RSA_set_ex_data, has a pointer to the same
>> PKCS11_KEY that the libp11 needs.
>> So a single ex_data index could be used if the code to keep
>> track of all of this was in the libp11.
>
> Yes, but in the engine loading the key happens, and everything related
> to the key is available.
> Which is why I put things there.
> In p11 you do not know which slots relate to which key.

I believe you do. (But p11 does not keep track of what slots are allocated.)

  The libp11-int.h defines all the *_private part for the ctx, slot, token,
keys, and certs structures it allocates.  In the *_private there is a parent
pointer, and a #define to use the parent pointers. Thus given a PKCS11_KEY,
on can find the token and slot.

CERT-> TOKEN
KEY -> TOKEN
TOKEN -> SLOT
SLOT -> CTX

TOKEN has array of CERTs
TOKEN has array of KEYs.

SLOT does NOT have an array of TOKENS.
CTX does NOT have an array of SLOTS.

So the PKCS11_enumerate_slots leaves it up to the engine to
call PKCS11_release_all_slots which the engine can not do
if it found the key or cert is was looking, as these need to be
available when using the key or cert.

Is it possible to have the CTX contain an array of SLOTS,
and the SLOTS and array of TOKENS? Thus at engine finish,
any remaining SLOTS and TOKENS would be released.


What is also not clear is if the libp11 keeps track of sessions correctly.
It looks like it only allows one session, but may not clean up
correctly if a second session is opened.

(pkcs11-spy might help show what is going on.)

> If you want tracking in p11, loading keys and certs (by slot/id, label)

That part is OK as it is engine specific. But keeping track of
all the allocated slots and tokens should be in libp11.

> has to happen in p11, not in the engine.
> Traversing things backwards from the key to the slots won't work.
> The engine can call the p11 function, but p11 has to do the lifting.
>
> If you think that is an acceptable change, let me know.
>
>> The  PKCS11_RSA_CRYPTO_EX is not particular to RSA.
>> and  could also be used for ECDSA and any other key types
>> we add in the future. So a more generic name would be better.
>> (It still needs to be added to the RSA, ECDSA, or what ever
>> using the *_ex_data for that type of key.)
>
> Renaming to PKCS11_CRYPTO_EX is trivial.
>
>> (The ECDSA code I have working uses the ECDSA_set_ex_data
>> like the RSA_set_ex_data to store a pointer to the PKCS11_KEY.
>> Thus it would use the new version of the PKCS11_RSA_CRYPTO_EX.
>
>
>
> 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
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/opensc-devel
>

--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

------------------------------------------------------------------------------
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Markus Kötter
On 09/26/2013 09:21 PM, Douglas E. Engert wrote:
> On 9/26/2013 12:07 PM, Markus Koetter wrote:
>> Yes, but in the engine loading the key happens, and everything related
>> to the key is available.
>> Which is why I put things there.
>> In p11 you do not know which slots relate to which key.
>
> I believe you do. (But p11 does not keep track of what slots are allocated.)

What I said.

>    The libp11-int.h defines all the *_private part for the ctx, slot, token,
> keys, and certs structures it allocates.  In the *_private there is a parent
> pointer, and a #define to use the parent pointers. Thus given a PKCS11_KEY,
> on can find the token and slot.
>
> CERT-> TOKEN
> KEY -> TOKEN
> TOKEN -> SLOT
> SLOT -> CTX
>
> TOKEN has array of CERTs
> TOKEN has array of KEYs.
>
> SLOT does NOT have an array of TOKENS.
> CTX does NOT have an array of SLOTS.
>
> So the PKCS11_enumerate_slots leaves it up to the engine to
> call PKCS11_release_all_slots which the engine can not do
> if it found the key or cert is was looking, as these need to be
> available when using the key or cert.
>
> Is it possible to have the CTX contain an array of SLOTS,
> and the SLOTS and array of TOKENS? Thus at engine finish,
> any remaining SLOTS and TOKENS would be released.

If you rely on engine finish to free memory, your application code is
broken.
You can't reload the engine, as re-loading gost will destroy openssl.
Reloading gost is default when using the pkcs11 engine.
Cleaning up on exit while leaking at runtime is cosmetics.


To get back to the question, it *may* be possible, if you
  * keep track of the CTX per PKCS11_KEY
  * have only one slot, so basically only the virtual slot and no real
card reader

but - moving the load_*_key procedure to p11 solves the overall problem.
In a c&p fashion, way less complex and intrusive.

> What is also not clear is if the libp11 keeps track of sessions correctly.
> It looks like it only allows one session, but may not clean up
> correctly if a second session is opened.

Session as in CTX?
Cleanup what?

> (pkcs11-spy might help show what is going on.)

Please define the suspected problem to a level where reproducing is
possible.

>> If you want tracking in p11, loading keys and certs (by slot/id, label)
>> has to happen in p11, not in the engine.
>> Traversing things backwards from the key to the slots won't work.
>> The engine can call the p11 function, but p11 has to do the lifting.
>>
>> If you think that is an acceptable change, let me know.

Thats the real question.
If this is acceptable - moving code is easy.
Copy things to p11, rename things, get them exported properly, call
things in the engine, done.

Given the code is written already and just needs some rename, c&p - this
is trivial.
Given somebody with permissions to merge blesses the approach, expect a
merge req.

Who can bless/merge things?


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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Markus Kötter
On 09/26/2013 11:36 PM, Douglas E. Engert wrote:
> On 9/26/2013 3:02 PM, Markus Kötter wrote:
>> On 09/26/2013 09:21 PM, Douglas E. Engert wrote:
>>> On 9/26/2013 12:07 PM, Markus Koetter wrote:
...

>>> Is it possible to have the CTX contain an array of SLOTS,
>>> and the SLOTS and array of TOKENS? Thus at engine finish,
>>> any remaining SLOTS and TOKENS would be released.
>>
>> If you rely on engine finish to free memory, your application code is
>> broken.
>> You can't reload the engine, as re-loading gost will destroy openssl.
>> Reloading gost is default when using the pkcs11 engine.
>> Cleaning up on exit while leaking at runtime is cosmetics.
>>
>>
>> To get back to the question, it *may* be possible, if you
>>    * keep track of the CTX per PKCS11_KEY
>
> In libp11-int.h, for a key, the
> #define KEY2CTX(key)
> returns the CTX from the key.
>
>   KEY2CTX(key) returns the CTX

CTX is not a problem, there is a single CTX per engine.
As the CTX does not track the slots allocated, it does not helo.

>> but - moving the load_*_key procedure to p11 solves the overall problem.
>> In a c&p fashion, way less complex and intrusive.
>>
>>> What is also not clear is if the libp11 keeps track of sessions
>>> correctly.
>>> It looks like it only allows one session, but may not clean up
>>> correctly if a second session is opened.
>>
>> Session as in CTX?

If you think of session as in PKCS11 Session handle, I'm with you.

> No Sessions as in PKCS#11 sessions. The CTX has the session handle,
> but it can get replaced.

Currently - sessions are OpenSC global.
p11 closes the sessions on PKCS11_release_all_slots.

There is no reference counting for sessions.
Load 2 keys of the same card via the engine, free the slots for a key,
and the handle for the others gets invalidated.

PKCS11_release_all_slots
pkcs11_release_slot
C_CloseAllSessions

EVP_SignFinal: No such file or directory
140627597526688:error:800280B3:PKCS11
library:PKCS11_get_attribute:Session handle invalid:p11_attr.c:53:

So fixing the current memory leak without fixing the session handling
results in dead sessions.

>>>> If you want tracking in p11, loading keys and certs (by slot/id, label)
>>>> has to happen in p11, not in the engine.

I moved the code to p11, works as before.
Had to restructure the pin handling a little, but left the use of
UI_*_app_data as before.

But, as I said, free'ing memory kills sessions.

Not freeing memory keeps the sessions alive forever, they leak,
accompanied by the memory.


I'll try to work something out.


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=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Markus Kötter
On 10/05/2013 09:50 PM, Markus Kötter wrote:
> I'll try to work something out.

I did.

You can have a look by visiting my pull requests
https://github.com/OpenSC/engine_pkcs11/pull/7
https://github.com/OpenSC/engine_pkcs11/pull/7

and comment on them.


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=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Douglas E. Engert
In reply to this post by Markus Kötter
Sounds like you are are the right track.


On 10/5/2013 2:50 PM, Markus Kötter wrote:
On 09/26/2013 11:36 PM, Douglas E. Engert wrote:
On 9/26/2013 3:02 PM, Markus Kötter wrote:
On 09/26/2013 09:21 PM, Douglas E. Engert wrote:
On 9/26/2013 12:07 PM, Markus Koetter wrote:
...
Is it possible to have the CTX contain an array of SLOTS,
and the SLOTS and array of TOKENS? Thus at engine finish,
any remaining SLOTS and TOKENS would be released.
If you rely on engine finish to free memory, your application code is
broken.
You can't reload the engine, as re-loading gost will destroy openssl.
Reloading gost is default when using the pkcs11 engine.
Cleaning up on exit while leaking at runtime is cosmetics.


To get back to the question, it *may* be possible, if you
   * keep track of the CTX per PKCS11_KEY
In libp11-int.h, for a key, the
#define KEY2CTX(key)
returns the CTX from the key.

  KEY2CTX(key) returns the CTX
CTX is not a problem, there is a single CTX per engine.
As the CTX does not track the slots allocated, it does not helo.

but - moving the load_*_key procedure to p11 solves the overall problem.
In a c&p fashion, way less complex and intrusive.

What is also not clear is if the libp11 keeps track of sessions
correctly.
It looks like it only allows one session, but may not clean up
correctly if a second session is opened.
Session as in CTX?
If you think of session as in PKCS11 Session handle, I'm with you.

No Sessions as in PKCS#11 sessions. The CTX has the session handle,
but it can get replaced.
Currently - sessions are OpenSC global.
p11 closes the sessions on PKCS11_release_all_slots.

There is no reference counting for sessions.
Load 2 keys of the same card via the engine, free the slots for a key, 
and the handle for the others gets invalidated.

PKCS11_release_all_slots
pkcs11_release_slot
C_CloseAllSessions

EVP_SignFinal: No such file or directory
140627597526688:error:800280B3:PKCS11 
library:PKCS11_get_attribute:Session handle invalid:p11_attr.c:53:

So fixing the current memory leak without fixing the session handling 
results in dead sessions.

If you want tracking in p11, loading keys and certs (by slot/id, label)
has to happen in p11, not in the engine.
I moved the code to p11, works as before.
Had to restructure the pin handling a little, but left the use of 
UI_*_app_data as before.

But, as I said, free'ing memory kills sessions.

Not freeing memory keeps the sessions alive forever, they leak, 
accompanied by the memory.


I'll try to work something out.

Yes that is the concern I had, it was not clear if memory was leaked, or double frees. Maybe some reference counts could help.




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=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel


-- 

 Douglas E. Engert  [hidden email]
 Argonne National Laboratory
 9700 South Cass Avenue
 Argonne, Illinois  60439 
 (630) 252-5444

------------------------------------------------------------------------------
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=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Memory leaks with libp11 and engine-pkcs11

Douglas E. Engert
In reply to this post by Markus Kötter

On 10/6/2013 4:59 AM, Markus Kötter wrote:
> On 10/05/2013 09:50 PM, Markus Kötter wrote:
>> I'll try to work something out.
> I did.
>
> You can have a look by visiting my pull requests
> https://github.com/OpenSC/engine_pkcs11/pull/7
> https://github.com/OpenSC/engine_pkcs11/pull/7
>
> and comment on them.
I am not at work today, will try an look this week.

>
>
> 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=60134791&iu=/4140/ostg.clktrk
> _______________________________________________
> Opensc-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/opensc-devel
>

--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444


------------------------------------------------------------------------------
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=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel