opensc-devel — discussion of developement of OpenSC and related projects

Re: [Opensc-devel] Memory leaks with libp11 and engine-pkcs11

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

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

Re: [Opensc-devel] Memory leaks with libp11 and engine-pkcs11

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

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

Re: [Opensc-devel] opensc projects and the old wiki content generation

[Alon Bar-L. <alo...@gm...>]

Done:

https://github.com/OpenSC/libp11/pull/5
https://github.com/OpenSC/engine_pkcs11/pull/6
https://github.com/OpenSC/pam_p11/pull/1

On Wed, Sep 25, 2013 at 6:34 PM, Douglas E. Engert <dee...@an...> wrote:
> On 9/25/2013 9:44 AM, Ludovic Rousseau wrote:
>> 2013/9/25 Alon Bar-Lev <alo...@gm...>:
>>> Hi,
>>
>> Hello,
>>
>>> In the past we embedded the wiki content within the tarballs of the
>>> sources, this is now not working.
>>>
>>> I suggest to remove this step from all projects, or find some other solution.
>>>
>>> What do you think?
>>
>> I is now rare to _not_ have access to the Internet. So including the
>> wiki in the tar ball has less value.
>> I propose to remove this (broken) step.
>
> I agree.
>
>>
>> Bye
>>
>
> --
>
>   Douglas E. Engert  <DEE...@an...>
>   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
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/opensc-devel

[Opensc-devel] concurrent use

[<J.W...@mi...>]

Hi all,


Perhaps someone can refresh my memory:
AFAICR, (fosdem 2012) one of the issues, was that use could not use simultaneously a smartcard (single threadiness or so)

Is that (still) so? Let me explain why: 
I currently use my Infineon card for setting up an openvpn-connection, and I wonder if I could set up (with the credentials on my card) parallel tunnels and/or nested tunnels.

Perhaps someone has done this before....
Or am I worrying too much, and is (re-)keying done so fast and is openvpn/strongswan immediately releasing the card..

Hans

______________________________________________________________________
Dit bericht kan informatie bevatten die niet voor u is bestemd. Indien u niet de geadresseerde bent of dit bericht abusievelijk aan u is toegezonden, wordt u verzocht dat aan de afzender te melden en het bericht te verwijderen. De Staat aanvaardt geen aansprakelijkheid voor schade, van welke aard ook, die verband houdt met risico's verbonden aan het electronisch verzenden van berichten.

This message may contain information that is not intended for you. If you are not the addressee or if this message was sent to you by mistake, you are requested to inform the sender and delete the message. The State accepts no liability for damage of any kind resulting from the risks inherent in the electronic transmission of messages.

Re: [Opensc-devel] Memory leaks with libp11 and engine-pkcs11

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

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

Re: [Opensc-devel] Memory leaks with libp11 and engine-pkcs11

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

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

Re: [Opensc-devel] Memory leaks with libp11 and engine-pkcs11

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

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

[Opensc-devel] Memory leaks with libp11 and engine-pkcs11

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

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  <DEE...@an...>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

Re: [Opensc-devel] Recently submitted pull requests

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

On 9/25/2013 4:17 PM, Tim Taylor wrote:
>
>
> On 09/25/2013 01:42 PM, Douglas E. Engert wrote:
>>
>>
>> On 9/24/2013 4:05 PM, Tim Taylor wrote:
>>>
>>>
>>> On 09/24/2013 04:04 PM, Douglas E. Engert wrote:
>>>
>>>> (You know that Windows 7 and above have a PIV driver from Micrsoft,
>>>> that works with login, IE, Outlook, and any application that can use
>>>> the certificate store, like Chrome.)
>>>
>>> Yes, that is true.  However, the MS-provided smartcard stack does not
>>> support using an external pinpad to enter the token pin.  Their
>>> middleware does not attempt to discover what features are supported by
>>> the reader connected to the system.  Even if the vendor reader driver is
>>> installed, the MS Card Services treat the pinpad reader as a basic USB
>>> CCID reader and prompt the user to enter their PIN via the system
>>> keyboard.
>>>
>>> That's why I was looking at using OpenSC in the first place.  Except the
>>> OpenSC minidriver (without my fix) sets the PIN Type to be AlphaNumeric
>>> instead of External.  This causes the MS base card service module to
>>> prompt the user for their token PIN via a dialog box, instead of having
>>> them enter it on the pin pad.  When the PIN Type is set to External, the
>>> minidriver has complete control over how the PIN is collected and
>>> verified.
>>>
>>>>
>>>>
>>>> I would rather see the piv_ops (*read_public_key) implemented in the
>>>> card_piv.c
>>>> as this will only do it when needed.
>>>
>>> I will look into doing it this way.  This was actually the path I
>>> started down, but I seem to recall running into difficulty, but can't
>>> remember exactly what went wrong.  I'll try again.
>>
>> Having looked at the options, the most straight forward way is to do
>> some like
>> what you have done.
>>
>> I would like to propose this attached simpler fix to pkcs15-piv.c
>>
>
> I reworked my pull request to use your suggested changes and verified
> that pkcs15-tool can read the public keys.

OK, will try it.

Since the PIV cards are not PKCS#15 cards the OpenSC emulated the PKCS#15.
Part of the emulation was to emulate a pubkey from the certificate
in case it was needed. The certificate is the only way to find out what
type of key (RSA or EC) and what size the key is on card, so
the pkcs15-piv.c got that info.

The emulation of returning a pubkey object for PKCS#11 is handled
in the pkcs11/framework-pkcs15.c, so pkcs11-tool could return a
key. But the pkcs15-tool could not.

This code was in the original 2005 code (for RSA only) long before
the mindriver and the fact that the pkcs15 code could not return a
pubkey was never a problem.

The current patch also allows the pkcs15-tool to return a pubkey.


>
> I also submitted a new pull request (#189) that addresses the root
> problem that led me down this path.  It turns out that the minidriver
> function CardGetContainerInfo() was extracting the public key from the
> certificate successfully, but the return value was still an error code
> (set after the attempt to read the public key directly failed).  My new
> pull request fixes that problem by resetting the return value to success
> if the attempt to get the public key from the cert succeeds.
>
>>>>
>>>
>>>> What type of pin pad reader are you using?
>>>
>>> We are using both an two different pin pad readers: the OmniKey 3821 and
>>> the Gemalto PC Pinpad (I think it's now called the IDBridge CT700).
>>>
>>> - Tim
>>>
>>
>>
>>
>
> ------------------------------------------------------------------------------
> 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
> 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

Re: [Opensc-devel] Recently submitted pull requests

[Tim T. <tt...@mi...>]

On 09/25/2013 01:42 PM, Douglas E. Engert wrote:
>
>
> On 9/24/2013 4:05 PM, Tim Taylor wrote:
>>
>>
>> On 09/24/2013 04:04 PM, Douglas E. Engert wrote:
>>
>>> (You know that Windows 7 and above have a PIV driver from Micrsoft,
>>> that works with login, IE, Outlook, and any application that can use
>>> the certificate store, like Chrome.)
>>
>> Yes, that is true.  However, the MS-provided smartcard stack does not
>> support using an external pinpad to enter the token pin.  Their
>> middleware does not attempt to discover what features are supported by
>> the reader connected to the system.  Even if the vendor reader driver is
>> installed, the MS Card Services treat the pinpad reader as a basic USB
>> CCID reader and prompt the user to enter their PIN via the system
>> keyboard.
>>
>> That's why I was looking at using OpenSC in the first place.  Except the
>> OpenSC minidriver (without my fix) sets the PIN Type to be AlphaNumeric
>> instead of External.  This causes the MS base card service module to
>> prompt the user for their token PIN via a dialog box, instead of having
>> them enter it on the pin pad.  When the PIN Type is set to External, the
>> minidriver has complete control over how the PIN is collected and
>> verified.
>>
>>>
>>>
>>> I would rather see the piv_ops (*read_public_key) implemented in the
>>> card_piv.c
>>> as this will only do it when needed.
>>
>> I will look into doing it this way.  This was actually the path I
>> started down, but I seem to recall running into difficulty, but can't
>> remember exactly what went wrong.  I'll try again.
>
> Having looked at the options, the most straight forward way is to do
> some like
> what you have done.
>
> I would like to propose this attached simpler fix to pkcs15-piv.c
>

I reworked my pull request to use your suggested changes and verified 
that pkcs15-tool can read the public keys.

I also submitted a new pull request (#189) that addresses the root 
problem that led me down this path.  It turns out that the minidriver 
function CardGetContainerInfo() was extracting the public key from the 
certificate successfully, but the return value was still an error code 
(set after the attempt to read the public key directly failed).  My new 
pull request fixes that problem by resetting the return value to success 
if the attempt to get the public key from the cert succeeds.

>>>
>>
>>> What type of pin pad reader are you using?
>>
>> We are using both an two different pin pad readers: the OmniKey 3821 and
>> the Gemalto PC Pinpad (I think it's now called the IDBridge CT700).
>>
>> - Tim
>>
>
>
>

Re: [Opensc-devel] Recently submitted pull requests

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

On 9/24/2013 4:05 PM, Tim Taylor wrote:
>
>
> On 09/24/2013 04:04 PM, Douglas E. Engert wrote:
>
>> (You know that Windows 7 and above have a PIV driver from Micrsoft,
>> that works with login, IE, Outlook, and any application that can use
>> the certificate store, like Chrome.)
>
> Yes, that is true.  However, the MS-provided smartcard stack does not
> support using an external pinpad to enter the token pin.  Their
> middleware does not attempt to discover what features are supported by
> the reader connected to the system.  Even if the vendor reader driver is
> installed, the MS Card Services treat the pinpad reader as a basic USB
> CCID reader and prompt the user to enter their PIN via the system keyboard.
>
> That's why I was looking at using OpenSC in the first place.  Except the
> OpenSC minidriver (without my fix) sets the PIN Type to be AlphaNumeric
> instead of External.  This causes the MS base card service module to
> prompt the user for their token PIN via a dialog box, instead of having
> them enter it on the pin pad.  When the PIN Type is set to External, the
> minidriver has complete control over how the PIN is collected and verified.
>
>>
>>
>> I would rather see the piv_ops (*read_public_key) implemented in the card_piv.c
>> as this will only do it when needed.
>
> I will look into doing it this way.  This was actually the path I
> started down, but I seem to recall running into difficulty, but can't
> remember exactly what went wrong.  I'll try again.

Having looked at the options, the most straight forward way is to do some like
what you have done.

I would like to propose this attached simpler fix to pkcs15-piv.c




>
>>
>
>> What type of pin pad reader are you using?
>
> We are using both an two different pin pad readers: the OmniKey 3821 and
> the Gemalto PC Pinpad (I think it's now called the IDBridge CT700).
>
> - Tim
>

-- 

  Douglas E. Engert  <DEE...@an...>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

Re: [Opensc-devel] opensc projects and the old wiki content generation

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

On 9/25/2013 9:44 AM, Ludovic Rousseau wrote:
> 2013/9/25 Alon Bar-Lev <alo...@gm...>:
>> Hi,
>
> Hello,
>
>> In the past we embedded the wiki content within the tarballs of the
>> sources, this is now not working.
>>
>> I suggest to remove this step from all projects, or find some other solution.
>>
>> What do you think?
>
> I is now rare to _not_ have access to the Internet. So including the
> wiki in the tar ball has less value.
> I propose to remove this (broken) step.

I agree.

>
> Bye
>

-- 

  Douglas E. Engert  <DEE...@an...>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

Re: [Opensc-devel] opensc projects and the old wiki content generation

[Ludovic R. <lud...@gm...>]

2013/9/25 Alon Bar-Lev <alo...@gm...>:
> Hi,

Hello,

> In the past we embedded the wiki content within the tarballs of the
> sources, this is now not working.
>
> I suggest to remove this step from all projects, or find some other solution.
>
> What do you think?

I is now rare to _not_ have access to the Internet. So including the
wiki in the tar ball has less value.
I propose to remove this (broken) step.

Bye

-- 
 Dr. Ludovic Rousseau

[Opensc-devel] opensc projects and the old wiki content generation

[Alon Bar-L. <alo...@gm...>]

Hi,

In the past we embedded the wiki content within the tarballs of the
sources, this is now not working.

I suggest to remove this step from all projects, or find some other solution.

What do you think?

Regards,
Alon Bar-Lev.

Re: [Opensc-devel] soft token

[Mart S. <mrt...@gm...>]

Resurrecting a really old thread here - now we have a smart card emulation
IFD handler driver at
https://bitbucket.org/mrts/pcsclite-softemu-ifdhandler

Hope that helps someone.

Best,
Mart Sõmermaa

>On 24 November 2010 14:52, Ludovic Rousseau <[hidden email]> wrote:
>
>> 2010/11/24 Vedran Bartonicek <[hidden email]>:
>>> Hello,
>>> Thanks for the comments.
>>>> Why OpenCT?
>>> So far I am researching which component (PC/SC - IFD Handler & OpenCT
>>> driver) would be easier to adapt to usage with SC emulation engine.
>>> IFDHandler interface for PC/SC I haven't studied yet. It seems to also
>>> support only USB and serial SCs.
>>> So any solutuion if fine for me, as long as I am able to find any way
>>> to use SC emulation engine.
>>
>> An IFDHandler can use anything. You can configure the driver using a
>> reader.conf file containing:
>> # Configuration file for pcsc-lite
>> FRIENDLYNAME  Generic Reader
>> DEVICENAME    /dev/ttyS0
>> LIBPATH       /usr/lib/pcsc/drivers/libgen_ifd.so
>> CHANNELID     1
>>
>> You can use anything for CHANNELID and omit the DEVICENAME
>>
>> See http://pcsclite.alioth.debian.org/api/group__IFDHandler.html
>>
>> --
>>  Dr. Ludovic Rousseau
>>
>
>Thanks for the hints.
>-Vedran

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

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

Douglas --

I think a few parts of this are aimed at me, but I can't speak to the
whole issue.

On Tue, Sep 24, 2013 at 10:25 AM, Douglas E. Engert <dee...@an...> wrote:
>
> On 9/21/2013 4:45 PM, Markus Kötter wrote:
>> 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.

Heh.  It fixed *my* problem.  :-)

> I have been looking at the PKCS11_* structures too, including the _private parts.

There are other infelicities in that code as well.  I ran into a
situation where the login state was weird (in my projects, users can
enter PIN at almost any time, which might trigger a logout etc).  I
ended up hitting the private section directly; ugly, but it works:

namespace // anonymous
{

struct pkcs11_slot_private {
    PKCS11_CTX *parent;
    unsigned char haveSession, loggedIn;
    /* CK_SLOT_ID */        unsigned long id;
    /* CK_SESSION_HANDLE */ unsigned long session;
};

#if DEBUG_LEVEL >= DEBUG_LEVEL_FINE

void
debugSlotPrivate( const PKCS11_SLOT * slot )
{
    const pkcs11_slot_private * priv =
      static_cast< pkcs11_slot_private * >( slot->_private );

    FINE( "dsp: " << std::boolalpha <<
          "hs=" << !!priv->haveSession << ", "
          "li=" << !!priv->loggedIn << ", "
          "sess=" << priv->session );
}

#else

void
debugSlotPrivate( const PKCS11_SLOT * )
{
}

#endif

} // end namespace [anonymous]

...

    {
        // uhg, this is a huge hack.  the problem seems to be that
        // releasing the key frees all the slots, which in turn
        // terminates all active sessions.  since the session is gone,
        // we have to make sure that libp11 doesn't try to end the
        // session again.

        pkcs11_slot_private * priv =
          static_cast< pkcs11_slot_private * >( m_pUsedSlot->_private );

        if ( priv->haveSession )
        {
            priv->loggedIn = 0;
            priv->haveSession = 0;
        }
    }

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

Don't forget to latch into the global engine cleanup at program exit.

The OpenSSL object lifetime rules are HORRIBLE.

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

Or we could propose its addition.  The more similar all the key
objects are (especially for issues like this that are truly
independent of key type), the better off everyone is.

As my own experience is a prefect example, however, much of OpenSSL is
formed by someone hacking at it enough to get their use case working,
then moving on to the next thing.

I can't see any other reason why the lifetime / auxilary data
management of EC vs. RSA keys should differ at all.

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

As above: it worked for me.  :-/

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

More or less.  What I was really trying to describe (poorly,
apparently) is true subclassing with virtual functions (specifically,
virtual destructors) as available in C++.

To phrase it in C++, I would love to see something like:

struct EVP_KEY { ... virtual ~EVP_KEY(); ... }
struct EVP_PKEY_RSA : public EVP_KEY { ... virtual ~EVP_PKEY_RSA(); .... }
struct EVP_PKEY_EC: public EVP_KEY { ... virtual ~EVP_PKEY_EC(); ... }

Then, when any application does [the equivalent of]:

    EVP_KEY * key = get_my_fancy_key(...);
    ... // use key for whatever
    delete key; // trigger *virtual* destructor.

I'm not advocating use of C++ in this instance, I'm just trying to use
the well-defined C++ destructor semantics to explain my POV.

For a C-based example, there's the VFS and driver ops in Linux, where
the vtable is explicitly maintained, and subclassing is done by
assigning different pointers -- but we have access to the previous
value, so we can chain calls up to the superclass if necessary.

I did find that at least RSA has a finish method, and it can be
retrieved -- but when I looked, I'm pretty sure there was no way to
set it.  (Or, maybe it was the other way around -- there were public
methods for setting the 'finish' method, but no way to get the
*current* finish method, so it couldn't be chained, and therefore it
wasn't clear just how useful such a method could be.).

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

If you're successful in persuading the OpenSSL devs to make some of
these changes (expose structures for use by external code), then
perhaps you can start a discussion for how to help that external code
properly handle lifecycle issues like the ones we're discussing here.

> Your mods combined with the engine_finish could handle this.

As I've said a few times recently, I don't expect to have time to
pursue this work much further (if at all).  The project is of course
welcome to integrate as much of my code in the pull request as they
see fit -- that's why I submitted it, after all.  :)

Best regards (and good luck!),
Anthony "tkil" Foiani

Re: [Opensc-devel] OpenPGP card status?

[Crypto S. <cry...@pr...>]

Hi Daniel!
I just stumbled upon your unanswered post of June. Answer: OpenPGP Card
is fully supported by OpenSC.

Best regards
Jan

Am 17.06.2013 23:36, schrieb Daniel Pocock:
> 
> 
> Hi,
> 
> I just had a look at this page:
> 
> https://www.opensc-project.org/opensc/wiki/SupportedHardware
> 
> and it has OpenPGP card below the `Unsupported' heading
> 
> Is that still the case?  There appears to be a lot of detail on the
> OpenPGP page:
> https://www.opensc-project.org/opensc/wiki/OpenPGP
> 
> Would it be possible to annotate the unsupported cards with some
> comments to distinguish those that will never be supported from those
> that are work-in-progress?
> 
> Looking at it from the other angle, the OpenSC FAQ took me to this page:
>   https://sites.google.com/site/alonbarlev/gnupg-pkcs11
> 
> which has a very brief statement about "The GnuPG developers insist of
> implementing smartcard support from scratch, what makes a low smartcard
> variety" - for an outsider, it's not exactly clear what that means.
> 
> Is there any document the explains, at arm's length, the current state
> of play with free-software related smart card technology and with some
> practical comments about how people can mix-and-match all their
> different use cases (e.g. ssh keys, gpg keys, X.509 certs for web/email,
> VPN that use any of the above types of key, disk encryption, private
> root CA key)?
> 
> Regards,
> 
> Daniel
> 
> 
> ------------------------------------------------------------------------------
> This SF.net email is sponsored by Windows:
> 
> Build for Windows Store.
> 
> http://p.sf.net/sfu/windows-dev2dev
> _______________________________________________
> Opensc-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/opensc-devel
> 

Re: [Opensc-devel] Recently submitted pull requests

[Tim T. <tt...@mi...>]

On 09/24/2013 04:04 PM, Douglas E. Engert wrote:

> (You know that Windows 7 and above have a PIV driver from Micrsoft,
> that works with login, IE, Outlook, and any application that can use
> the certificate store, like Chrome.)

Yes, that is true.  However, the MS-provided smartcard stack does not 
support using an external pinpad to enter the token pin.  Their 
middleware does not attempt to discover what features are supported by 
the reader connected to the system.  Even if the vendor reader driver is 
installed, the MS Card Services treat the pinpad reader as a basic USB 
CCID reader and prompt the user to enter their PIN via the system keyboard.

That's why I was looking at using OpenSC in the first place.  Except the 
OpenSC minidriver (without my fix) sets the PIN Type to be AlphaNumeric 
instead of External.  This causes the MS base card service module to 
prompt the user for their token PIN via a dialog box, instead of having 
them enter it on the pin pad.  When the PIN Type is set to External, the 
minidriver has complete control over how the PIN is collected and verified.

>
>
> I would rather see the piv_ops (*read_public_key) implemented in the card_piv.c
> as this will only do it when needed.

I will look into doing it this way.  This was actually the path I 
started down, but I seem to recall running into difficulty, but can't 
remember exactly what went wrong.  I'll try again.

>

> What type of pin pad reader are you using?

We are using both an two different pin pad readers: the OmniKey 3821 and 
the Gemalto PC Pinpad (I think it's now called the IDBridge CT700).

- Tim

-- 
Tim Taylor
tt...@mi...
Office: 781-271-2099
Mobile: 617-893-0107

Principal Software Systems Engineer
The MITRE Corporation
202 Burlington Road
Bedford, MA 01730-1420

Re: [Opensc-devel] Recently submitted pull requests

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

On 9/24/2013 2:23 PM, Tim Taylor wrote:
> Greetings.
>
> I just submitted three pull requests (#184, #185, #186) for
> consideration that I developed while trying to use the OpenSC minidriver
> on Windows 7 with a pin pad capable reader, and a PIV token.  I'll
> provide a brief description of each pull request.
>
> #184: Set output buffer len variable if padding removed.
> Note: I started my development with the 0.13.0 source code tarball.
> When I got to the point where I was ready to submit pull requests, it
> appears that the issue addressed by this pull request has a "fix" in
> place.  However I think this pull request is preferable
>
> The sc_pkcs1_strip_02_padding function in padding.c takes a pointer to
> an output buffer (out) and a pointer to the max length of the buffer
> (out_len).  On input, out_len is expected to point to the maximum length
> of the output buffer.  On output, this value should be over written with
> the length of un-padded data copied to the out buffer, but this was not
> being done.
>
> #185: Extract public key from cert if no object on card
> The PIV token that I am using is a US DoD Common Access Card (CAC).
> These tokens do not have separate containers for the public keys.

PIV cards dont either.

> When I
> first started using these tokens with the windows minidriver, and the
> certutil command line tool to read the token, I was getting a bunch of
> messages in the debug log: "No way to get public key: -1416 (Not
> implemented)".

(You know that Windows 7 and above have a PIV driver from Micrsoft,
that works with login, IE, Outlook, and any application that can use
the certificate store, like Chrome.)


I would rather see the piv_ops (*read_public_key) implemented in the card_piv.c
as this will only do it when needed.

As I said in a note in response to the pull request, the PKCS11 code
can emulate a pubkey.

>
> With this patch, when the PIV pkcs#15 emulation object is being
> initialized, it will extract the public key for each private key from
> the corresponding certificate.
>
> #186: Use reader pin pad if available and allowed
> This patch allows this use of a pin pad reader with the Windows
> minidriver (if using Version 6 or higher of the MS Base Card Services
> implementation -- it requires support for CardAuthenticateEx).  This is
> accomplished by setting the PinType to ExternalPinType instead of
> AlphaNumericPinType.  This also requires handling some additional
> properties (CP_PARENT_WINDOW, and CP_PIN_CONTEXT_STRING).

What type of pin pad reader are you using?
>
> ===============
>
> I would appreciate the consideration of the project committers for each
> of these improvements.  I've tried to make sure I conformed to the
> project coding conventions.  If I fell short of the conventions, please
> let me know and I'd be happy to rework the pull requests.  Also, if
> there is another alternative for achieving any of these that would be
> more acceptable for inclusion, just let me know.
>
> - Tim
>

-- 

  Douglas E. Engert  <DEE...@an...>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444

[Opensc-devel] Recently submitted pull requests

[Tim T. <tt...@mi...>]

Greetings.

I just submitted three pull requests (#184, #185, #186) for 
consideration that I developed while trying to use the OpenSC minidriver 
on Windows 7 with a pin pad capable reader, and a PIV token.  I'll 
provide a brief description of each pull request.

#184: Set output buffer len variable if padding removed.
Note: I started my development with the 0.13.0 source code tarball. 
When I got to the point where I was ready to submit pull requests, it 
appears that the issue addressed by this pull request has a "fix" in 
place.  However I think this pull request is preferable

The sc_pkcs1_strip_02_padding function in padding.c takes a pointer to 
an output buffer (out) and a pointer to the max length of the buffer 
(out_len).  On input, out_len is expected to point to the maximum length 
of the output buffer.  On output, this value should be over written with 
the length of un-padded data copied to the out buffer, but this was not 
being done.

#185: Extract public key from cert if no object on card
The PIV token that I am using is a US DoD Common Access Card (CAC). 
These tokens do not have separate containers for the public keys. When I 
first started using these tokens with the windows minidriver, and the 
certutil command line tool to read the token, I was getting a bunch of 
messages in the debug log: "No way to get public key: -1416 (Not 
implemented)".

With this patch, when the PIV pkcs#15 emulation object is being 
initialized, it will extract the public key for each private key from 
the corresponding certificate.

#186: Use reader pin pad if available and allowed
This patch allows this use of a pin pad reader with the Windows 
minidriver (if using Version 6 or higher of the MS Base Card Services 
implementation -- it requires support for CardAuthenticateEx).  This is 
accomplished by setting the PinType to ExternalPinType instead of 
AlphaNumericPinType.  This also requires handling some additional 
properties (CP_PARENT_WINDOW, and CP_PIN_CONTEXT_STRING).

===============

I would appreciate the consideration of the project committers for each 
of these improvements.  I've tried to make sure I conformed to the 
project coding conventions.  If I fell short of the conventions, please 
let me know and I'd be happy to rework the pull requests.  Also, if 
there is another alternative for achieving any of these that would be 
more acceptable for inclusion, just let me know.

- Tim

-- 
Tim Taylor
tt...@mi...
Office: 781-271-2099

Principal Software Systems Engineer
The MITRE Corporation
202 Burlington Road
Bedford, MA 01730-1420

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

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

On 9/24/2013 12:00 PM, Markus Kötter wrote:
> 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.

Thanks. I will give these a try. Looks like good cases to test
for memory leaks.


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

Correct, but as a last resort, the free could be done during the engine_finish
but the since the PKCS11_* structures are not all linked back to the PKCS11_CTX,
there is a problem in finding all of them.

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

OK, so they are using 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.

Sorry, I though that was yours.


> My patch is here:
> https://github.com/commonism/engine_pkcs11/commits/memoryleaks
> https://github.com/commonism/libp11/commits/memoryleaks

I will have to have a look at these.
>
> 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.

Those comments where in the other patch.

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

Will have a look.

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

Will have to look at your patches. The patch I was looking at was very involved.

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

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

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

[Petr P. <pet...@at...>]

On Tue, Sep 24, 2013 at 11:25:12AM -0500, Douglas E. Engert wrote:
> 
> When an engine returns a key or cert to the caller is is not
> clear who should free it, the engine of the application.
> 
I also did not find any documentation who should be reponsible for the memory
management. There are a few paragraphs about reference counting in engine(3)
manual page, but I'm not much clever after reading them.

I want just to point out, that current engine_pkcs11 and other engines
delivered with the OpenSSL return a copy of X509, but they do not duplicate
returned EVP_PKEY. I guess this is because certificates are expected to be
exportable whereas private keys are not.

-- Petr

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

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

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

Hi,

sorry for the noise ...

I forgot to free the structures I use to free the PKCS11_ structures.
Updated numbers below.

On 09/22/2013 11:19 AM, Markus Kötter wrote:
> On 09/22/2013 10:43 AM, Markus Kötter wrote:
>> engine_pkcs11 free's the mallocs claimed by PKCS11_ structures.
>> https://github.com/commonism/engine_pkcs11/commit/fbae0727e88fd20e1cba6ec60799dc4fe705cf97

https://github.com/commonism/engine_pkcs11/commit/efb57ad95c5c23b3be148bc8078b11367a445625

> Combined savings when ...
..
> loading 1000 keys
>
> before:
> ==12675== LEAK SUMMARY:
> ==12675==    definitely lost: 82,329 bytes in 1,011 blocks
> ==12675==    indirectly lost: 7,044,290 bytes in 146,860 blocks
> ==12675==      possibly lost: 8,196 bytes in 151 blocks
> ==12675==    still reachable: 1,684 bytes in 4 blocks
> ==12675==         suppressed: 0 bytes in 0 blocks
>
> after:
> ==13261== LEAK SUMMARY:
> ==13261==    definitely lost: 50,409 bytes in 1,012 blocks
> ==13261==    indirectly lost: 406 bytes in 10 blocks
> ==13261==      possibly lost: 0 bytes in 0 blocks
> ==13261==    still reachable: 1,684 bytes in 4 blocks
> ==13261==         suppressed: 0 bytes in 0 blocks

after freeing my own memory:
==13686== LEAK SUMMARY:
==13686==    definitely lost: 2,409 bytes in 12 blocks
==13686==    indirectly lost: 406 bytes in 10 blocks
==13686==      possibly lost: 0 bytes in 0 blocks
==13686==    still reachable: 1,684 bytes in 4 blocks
==13686==         suppressed: 0 bytes in 0 blocks


> The remaining leaks are burried in OpenSC itself, and may depend on the
> card used, I used a Feitian PKI Smartcard.


MfG
Markus