opensc-devel

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

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

Modifications to engine_pkcs11 and libp11 to support ECDSA
are available at github for testing, and I am looking for
comments.

https://github.com/dengert/libp11

https://github.com/dengert/engine_pkcs11

These can be used with OpenSC-0.13.0 and OpenSSL > 1.0.0
and have been tested using OpenSSL-1.0.1e both with and
without modifications for OpenSSL bug report #2459.
(See below.)

As interest in ECDSA and PKCS#11 has increased over the last
few months, I have sent out modifications developed in 2011
to a number of people, including Sanaullah, who reported he
has the older modifications working to allow OpenSSL to
generate certificate requests using EC keys and signed
using the EC key via PKCS#11 with softhsm.

I have tested using PIV smart cards that support EC keys.

There is also another OpenSSL modification that may be
needed if you tryn and use the OpenSSL dgst with the engine
and ECDSA. (See the attachment, and the reference
to the e-mail from 2010.)


Git commit comment for libp11 modification:

Experimental ECDSA support

Support for ECDSA is added for used with OpenSSL > 1.0.0.
OpenSSL has an outstanding bug report, #2459, that requests
the structure ecdsa_method be exposed. An engine needs to create
such a structure which internal to OpenSSL is static, which
has pointers into functions within the engine.

Engines using RSA do not have this problem, because the
rsa_method_st is exposed in rsa.h This allows an engine such
as the combination of libp11 and engine_pkcs11 to compile in
a static version of the rsa_method_st.

Modifications have been submitted to OpenSSL at the suggestion
of the OpenSSL developers to add a set of functions to build a
ecdsa_method structure and to set the needed functions into
the structure.

These libp11 modifications are designed to allow building
libp11 using either the internal OpenSSL
crypto/ecdsa/ecs_locl.h or the new ECDSA_METHOD_new
function in ecdsa.h

By default the ECDSA_METHOD_new will be used if present.

To build using the ecs_locl.h, one must have access to the
OpenSSL source and add to the libp11 build process,
-DBUILD_WITH_ECS_LOCL_H -I/<path.to.OpenSSL.source>/crypto/edcsa

(Note: that using an internal header file may require libp11
to be rebuilt to match the specific version of OpenSSL being
used, and may not work in future versions.)

Once the OpenSSL modifications for #2459 are accepted
libp11 will be changed to remove the old method.

The intent of this dual build it to allow people to use
ECDSA even if OpenSSL does not implement ECDSA_METHOD_new
or does not implement it in the near future.

-- 

   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

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

On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...> wrote:
> Modifications to engine_pkcs11 and libp11 to support ECDSA
> are available at github for testing, and I am looking for
> comments.
>
> https://github.com/dengert/libp11
>
> https://github.com/dengert/engine_pkcs11
>

Hi,

This is great, I also recently updated pkcs11-helper[1] to support
ecdsa as well.

What I am missing in the new solution[2] is finish method as in other
methods, this will allow cleanup method instance resources.

I am far from being openssl expert, but I did expect to see these
do_sign and sign_setup to accept ecdsa as parameter and not ec...

Regards,
Alon

[1] https://github.com/alonbl/pkcs11-helper/commits/ec
[2] http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e

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

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

On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...> wrote:
>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>> are available at github for testing, and I am looking for
>> comments.
>>
>> https://github.com/dengert/libp11
>>
>> https://github.com/dengert/engine_pkcs11
>>
>
> Hi,
>
> This is great, I also recently updated pkcs11-helper[1] to support
> ecdsa as well.
>
> What I am missing in the new solution[2] is finish method as in other
> methods, this will allow cleanup method instance resources.

Yes, OpenSSL had the init and finish with "#if 0".
and yes they may be needed, I think that is why they left them in the
code, but commented out, and why they do not want to expose
the ECDSA_METHOD structure.

>
> I am far from being openssl expert, but I did expect to see these
> do_sign and sign_setup to accept ecdsa as parameter and not ec...

Since both ECDSA and ECDH methods can use EC keys, they split up
some of that. Look at the  ecdsa_check()
  "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA structure"
That then points to the ECDSA_METHOD.
and the ecdh_check()

It might be the ecdsa_data_st, need to have the finish It has an init.
the inti or finish might depend  on how the key is used last.

I don't thing the OpenSSL developers have a good handle on what
the engine might need.

My ecdsa code may also have memory leaks too...

>
> Regards,
> Alon
>
> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
> [2] http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>

-- 

  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

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

On Fri, Sep 20, 2013 at 12:51 AM, Douglas E. Engert <dee...@an...> wrote:
>
>
> On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
>>
>> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...>
>> wrote:
>>>
>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>> are available at github for testing, and I am looking for
>>> comments.
>>>
>>> https://github.com/dengert/libp11
>>>
>>> https://github.com/dengert/engine_pkcs11
>>>
>>
>> Hi,
>>
>> This is great, I also recently updated pkcs11-helper[1] to support
>> ecdsa as well.
>>
>> What I am missing in the new solution[2] is finish method as in other
>> methods, this will allow cleanup method instance resources.
>
>
> Yes, OpenSSL had the init and finish with "#if 0".
> and yes they may be needed, I think that is why they left them in the
> code, but commented out, and why they do not want to expose
> the ECDSA_METHOD structure.

Right, you have an opened channel with openssl developers, can you
please ask them to add the finish to the interface?

>
>>
>> I am far from being openssl expert, but I did expect to see these
>> do_sign and sign_setup to accept ecdsa as parameter and not ec...
>
>
> Since both ECDSA and ECDH methods can use EC keys, they split up
> some of that. Look at the  ecdsa_check()
>  "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA structure"
> That then points to the ECDSA_METHOD.
> and the ecdh_check()

Yes, I saw that, but still, to be consistent and remove this check for
all functions they could have just have two set of "keys", as once EC
set with DSA or DH it cannot be used for the other.

> It might be the ecdsa_data_st, need to have the finish It has an init.
> the inti or finish might depend  on how the key is used last.
>
> I don't thing the OpenSSL developers have a good handle on what
> the engine might need.

Right... but I will be very happy to see that finish :)

Thanks!!!

>
> My ecdsa code may also have memory leaks too...
>
>
>>
>> Regards,
>> Alon
>>
>> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
>> [2]
>> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>>
>
> --
>
>  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

[Jean-Michel P. - G. <jm...@go...>]

Le jeudi 19 septembre 2013 à 15:31 -0500, Douglas E. Engert a écrit :
> Modifications to engine_pkcs11 and libp11 to support ECDSA
> are available at github for testing, and I am looking for
> comments. 

This is nice to have them on board.

My only comment is that, according to rumors, Elliptic curves are
reported broken by NSA crypto-analysts. The reason is that Elliptic
curves offer more space for mathematics and are quite new, offering
space for discoveries in factorization.

Kind regards,
-- 
                  Jean-Michel Pouré - Gooze - http://www.gooze.eu

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

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

On 9/20/2013 2:45 AM, Jean-Michel Pouré - GOOZE wrote:
> Le jeudi 19 septembre 2013 à 15:31 -0500, Douglas E. Engert a écrit :
>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>> are available at github for testing, and I am looking for
>> comments.
>
> This is nice to have them on board.
>
> My only comment is that, according to rumors, Elliptic curves are
> reported broken by NSA crypto-analysts. The reason is that Elliptic
> curves offer more space for mathematics and are quite new, offering
> space for discoveries in factorization.

I have not heard those rumors. I have heard there are some curves,
that should not be used.   On the contrary, there is more discussion
about breaking RSA in the next few years and the industry better be in
a position to have a replacement implemented, i.e. ECDSA and ECDH.

ECC is not that new it has been around for years. Its implementations
are new. RSA is wide use so interest in implementation EC has been low.

EC have an infinite number of curves, which complicated in implementation
and security. The industry appears to be settling on a small set of
named curves that can be trusted. This also makes it easier to implement.

Maybe a little out dated, but from 2009:
http://www.nsa.gov/business/programs/elliptic_curve.shtml

Implementation of EC is falling into place. The mods are a part of that,
implementing ECDSA in the engine, to supportsmart cards such as the PIV
card with ECDSA and ECDH.

I would rather have two crypto algorithms implemented, just in case.

>
> Kind regards,
>
>
>
> ------------------------------------------------------------------------------
> 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/20/13.
> http://pubads.g.doubleclick.net/gampad/clk?id=58041151&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

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

2013/9/20 Douglas E. Engert <dee...@an...>:
>
>
> On 9/20/2013 2:45 AM, Jean-Michel Pouré - GOOZE wrote:
>> Le jeudi 19 septembre 2013 à 15:31 -0500, Douglas E. Engert a écrit :
>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>> are available at github for testing, and I am looking for
>>> comments.
>>
>> This is nice to have them on board.
>>
>> My only comment is that, according to rumors, Elliptic curves are
>> reported broken by NSA crypto-analysts. The reason is that Elliptic
>> curves offer more space for mathematics and are quite new, offering
>> space for discoveries in factorization.
>
> I have not heard those rumors. I have heard there are some curves,
> that should not be used.   On the contrary, there is more discussion
> about breaking RSA in the next few years and the industry better be in
> a position to have a replacement implemented, i.e. ECDSA and ECDH.

Maybe Jean-Michel is revering to this article "La NSA est suspectée
d'avoir altéré un standard cryptographique" [1] (in French) that link
to a New York Times article "Government Announces Steps to Restore
Confidence on Encryption Standards" [2].

If we can't trust NIST any more then we can move to "the other side"
by using the GHOST cryptosystems [3] from the Soviet Union.

Bye

[1] http://www.numerama.com/magazine/26979-la-nsa-est-suspectee-d-avoir-altere-un-standard-cryptographique.html
[2] http://bits.blogs.nytimes.com/2013/09/10/government-announces-steps-to-restore-confidence-on-encryption-standards/?_r=0
[3] https://en.wikipedia.org/wiki/GOST

-- 
 Dr. Ludovic Rousseau

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

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

On 9/20/2013 1:19 AM, Alon Bar-Lev wrote:
> On Fri, Sep 20, 2013 at 12:51 AM, Douglas E. Engert <dee...@an...> wrote:
>>
>>
>> On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
>>>
>>> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...>
>>> wrote:
>>>>
>>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>>> are available at github for testing, and I am looking for
>>>> comments.
>>>>
>>>> https://github.com/dengert/libp11
>>>>
>>>> https://github.com/dengert/engine_pkcs11
>>>>
>>>
>>> Hi,
>>>
>>> This is great, I also recently updated pkcs11-helper[1] to support
>>> ecdsa as well.
>>>
>>> What I am missing in the new solution[2] is finish method as in other
>>> methods, this will allow cleanup method instance resources.
>>
>>
>> Yes, OpenSSL had the init and finish with "#if 0".
>> and yes they may be needed, I think that is why they left them in the
>> code, but commented out, and why they do not want to expose
>> the ECDSA_METHOD structure.
>
> Right, you have an opened channel with openssl developers, can you
> please ask them to add the finish to the interface?

I got my foot in the door, I will see what I can do.
Do you have some example of what you need to do during finish?
I need to make an argument that the finish is needed.

I believe the RSA finish is called when a key is freed.
There is an engine finish that could also be used.

>
>>
>>>
>>> I am far from being openssl expert, but I did expect to see these
>>> do_sign and sign_setup to accept ecdsa as parameter and not ec...
>>
>>
>> Since both ECDSA and ECDH methods can use EC keys, they split up
>> some of that. Look at the  ecdsa_check()
>>   "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA structure"
>> That then points to the ECDSA_METHOD.
>> and the ecdh_check()
>
> Yes, I saw that, but still, to be consistent and remove this check for
> all functions they could have just have two set of "keys", as once EC
> set with DSA or DH it cannot be used for the other.

Not really. A key is a key, you can use it with either. A prime
example: an EC key on the smart card that is intended for key derivation,
(ECDH) needs a matching certificate. The certificate request needs
to be signed using ECDSA. That is how I have been testing
the engine to sign a cert request.

>
>> It might be the ecdsa_data_st, need to have the finish It has an init.
>> the inti or finish might depend  on how the key is used last.
>>
>> I don't thing the OpenSSL developers have a good handle on what
>> the engine might need.
>
> Right... but I will be very happy to see that finish :)
>
> Thanks!!!
>
>>
>> My ecdsa code may also have memory leaks too...
>>
>>
>>>
>>> Regards,
>>> Alon
>>>
>>> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
>>> [2]
>>> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>>>
>>
>> --
>>
>>   Douglas E. Engert  <DEE...@an...>
>>   Argonne National Laboratory
>>   9700 South Cass Avenue
>>   Argonne, Illinois  60439
>>   (630) 252-5444
>

-- 

  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

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

On Fri, Sep 20, 2013 at 4:09 PM, Douglas E. Engert <dee...@an...> wrote:
>
>
> On 9/20/2013 1:19 AM, Alon Bar-Lev wrote:
>>
>> On Fri, Sep 20, 2013 at 12:51 AM, Douglas E. Engert <dee...@an...>
>> wrote:
>>>
>>>
>>>
>>> On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
>>>>
>>>>
>>>> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...>
>>>> wrote:
>>>>>
>>>>>
>>>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>>>> are available at github for testing, and I am looking for
>>>>> comments.
>>>>>
>>>>> https://github.com/dengert/libp11
>>>>>
>>>>> https://github.com/dengert/engine_pkcs11
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> This is great, I also recently updated pkcs11-helper[1] to support
>>>> ecdsa as well.
>>>>
>>>> What I am missing in the new solution[2] is finish method as in other
>>>> methods, this will allow cleanup method instance resources.
>>>
>>>
>>>
>>> Yes, OpenSSL had the init and finish with "#if 0".
>>> and yes they may be needed, I think that is why they left them in the
>>> code, but commented out, and why they do not want to expose
>>> the ECDSA_METHOD structure.
>>
>>
>> Right, you have an opened channel with openssl developers, can you
>> please ask them to add the finish to the interface?
>
>
> I got my foot in the door, I will see what I can do.
> Do you have some example of what you need to do during finish?
> I need to make an argument that the finish is needed.
>
> I believe the RSA finish is called when a key is freed.
> There is an engine finish that could also be used.

Engine finish does not get a reference to key when released and having
engine is not always required.

What I used so far is method override that handles multiple instances
of keys, attaching the key specific data to the key. Then when finish
is called release the key specific data.

Reference implementation[1]

I can probably create new instance of engine for each instance of key,
but I do think that adding that missing finish in ec method to match
other methods is the correct solution.

[1] https://github.com/alonbl/pkcs11-helper/blob/ec/lib/pkcs11h-openssl.c#L375

>
>>
>>>
>>>>
>>>> I am far from being openssl expert, but I did expect to see these
>>>> do_sign and sign_setup to accept ecdsa as parameter and not ec...
>>>
>>>
>>>
>>> Since both ECDSA and ECDH methods can use EC keys, they split up
>>> some of that. Look at the  ecdsa_check()
>>>   "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA
>>> structure"
>>> That then points to the ECDSA_METHOD.
>>> and the ecdh_check()
>>
>>
>> Yes, I saw that, but still, to be consistent and remove this check for
>> all functions they could have just have two set of "keys", as once EC
>> set with DSA or DH it cannot be used for the other.
>
>
> Not really. A key is a key, you can use it with either. A prime
> example: an EC key on the smart card that is intended for key derivation,
> (ECDH) needs a matching certificate. The certificate request needs
> to be signed using ECDSA. That is how I have been testing
> the engine to sign a cert request.

As far as I can see the EC_KEY can be initialized for one at a time,
and it uses casting of method to reference the right methods... so
these are really two wrapped in one.

>
>>
>>> It might be the ecdsa_data_st, need to have the finish It has an init.
>>> the inti or finish might depend  on how the key is used last.
>>>
>>> I don't thing the OpenSSL developers have a good handle on what
>>> the engine might need.
>>
>>
>> Right... but I will be very happy to see that finish :)
>>
>> Thanks!!!
>>
>>>
>>> My ecdsa code may also have memory leaks too...
>>>
>>>
>>>>
>>>> Regards,
>>>> Alon
>>>>
>>>> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
>>>> [2]
>>>>
>>>> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>>>>
>>>
>>> --
>>>
>>>   Douglas E. Engert  <DEE...@an...>
>>>   Argonne National Laboratory
>>>   9700 South Cass Avenue
>>>   Argonne, Illinois  60439
>>>   (630) 252-5444
>>
>>
>
> --
>
>  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

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

On Fri, Sep 20, 2013 at 7:08 PM, Alon Bar-Lev <alo...@gm...> wrote:
> On Fri, Sep 20, 2013 at 4:09 PM, Douglas E. Engert <dee...@an...> wrote:
>>
>>
>> On 9/20/2013 1:19 AM, Alon Bar-Lev wrote:
>>>
>>> On Fri, Sep 20, 2013 at 12:51 AM, Douglas E. Engert <dee...@an...>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
>>>>>
>>>>>
>>>>> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>>>>> are available at github for testing, and I am looking for
>>>>>> comments.
>>>>>>
>>>>>> https://github.com/dengert/libp11
>>>>>>
>>>>>> https://github.com/dengert/engine_pkcs11
>>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This is great, I also recently updated pkcs11-helper[1] to support
>>>>> ecdsa as well.
>>>>>
>>>>> What I am missing in the new solution[2] is finish method as in other
>>>>> methods, this will allow cleanup method instance resources.
>>>>
>>>>
>>>>
>>>> Yes, OpenSSL had the init and finish with "#if 0".
>>>> and yes they may be needed, I think that is why they left them in the
>>>> code, but commented out, and why they do not want to expose
>>>> the ECDSA_METHOD structure.
>>>
>>>
>>> Right, you have an opened channel with openssl developers, can you
>>> please ask them to add the finish to the interface?
>>
>>
>> I got my foot in the door, I will see what I can do.
>> Do you have some example of what you need to do during finish?
>> I need to make an argument that the finish is needed.
>>
>> I believe the RSA finish is called when a key is freed.
>> There is an engine finish that could also be used.
>
> Engine finish does not get a reference to key when released and having
> engine is not always required.
>
> What I used so far is method override that handles multiple instances
> of keys, attaching the key specific data to the key. Then when finish
> is called release the key specific data.
>
> Reference implementation[1]
>
> I can probably create new instance of engine for each instance of key,
> but I do think that adding that missing finish in ec method to match
> other methods is the correct solution.
>
> [1] https://github.com/alonbl/pkcs11-helper/blob/ec/lib/pkcs11h-openssl.c#L375

Also... reading[1].

What is the reason of forcing allocated? Let's say I want to add flags
to existing non-allocated method?

+void ECDSA_METHOD_set_flags(ECDSA_METHOD *ecdsa_method, int flags)
+       {
+       ecdsa_method->flags = flags | ECDSA_METHOD_FLAG_ALLOCATED;
+       }

Also:

When is free expected to be called? I would expect that for
EC_KEY_free or similar.
Even if we add finish method, do you expect it to call free using its
own reference?

+void ECDSA_METHOD_free(ECDSA_METHOD *ecdsa_method)
+       {
+       if (ecdsa_method->flags & ECDSA_METHOD_FLAG_ALLOCATED)
+               OPENSSL_free(ecdsa_method);
+       }

Solution is to never call ECDSA_METHOD_new(), but always overwrite
fields in existing method... but then the force flags is something in
the way.

Alon

[1] http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=94c2f77a62be7079ab1893ab14b18a30157c4532

>>
>>>
>>>>
>>>>>
>>>>> I am far from being openssl expert, but I did expect to see these
>>>>> do_sign and sign_setup to accept ecdsa as parameter and not ec...
>>>>
>>>>
>>>>
>>>> Since both ECDSA and ECDH methods can use EC keys, they split up
>>>> some of that. Look at the  ecdsa_check()
>>>>   "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA
>>>> structure"
>>>> That then points to the ECDSA_METHOD.
>>>> and the ecdh_check()
>>>
>>>
>>> Yes, I saw that, but still, to be consistent and remove this check for
>>> all functions they could have just have two set of "keys", as once EC
>>> set with DSA or DH it cannot be used for the other.
>>
>>
>> Not really. A key is a key, you can use it with either. A prime
>> example: an EC key on the smart card that is intended for key derivation,
>> (ECDH) needs a matching certificate. The certificate request needs
>> to be signed using ECDSA. That is how I have been testing
>> the engine to sign a cert request.
>
> As far as I can see the EC_KEY can be initialized for one at a time,
> and it uses casting of method to reference the right methods... so
> these are really two wrapped in one.
>
>>
>>>
>>>> It might be the ecdsa_data_st, need to have the finish It has an init.
>>>> the inti or finish might depend  on how the key is used last.
>>>>
>>>> I don't thing the OpenSSL developers have a good handle on what
>>>> the engine might need.
>>>
>>>
>>> Right... but I will be very happy to see that finish :)
>>>
>>> Thanks!!!
>>>
>>>>
>>>> My ecdsa code may also have memory leaks too...
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>> Alon
>>>>>
>>>>> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
>>>>> [2]
>>>>>
>>>>> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>>>>>
>>>>
>>>> --
>>>>
>>>>   Douglas E. Engert  <DEE...@an...>
>>>>   Argonne National Laboratory
>>>>   9700 South Cass Avenue
>>>>   Argonne, Illinois  60439
>>>>   (630) 252-5444
>>>
>>>
>>
>> --
>>
>>  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

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

On 9/21/2013 10:29 AM, Alon Bar-Lev wrote:
> On Fri, Sep 20, 2013 at 7:08 PM, Alon Bar-Lev <alo...@gm...> wrote:
>> On Fri, Sep 20, 2013 at 4:09 PM, Douglas E. Engert <dee...@an...> wrote:
>>>
>>>
>>> On 9/20/2013 1:19 AM, Alon Bar-Lev wrote:
>>>>
>>>> On Fri, Sep 20, 2013 at 12:51 AM, Douglas E. Engert <dee...@an...>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert <dee...@an...>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>>>>>> are available at github for testing, and I am looking for
>>>>>>> comments.
>>>>>>>
>>>>>>> https://github.com/dengert/libp11
>>>>>>>
>>>>>>> https://github.com/dengert/engine_pkcs11
>>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> This is great, I also recently updated pkcs11-helper[1] to support
>>>>>> ecdsa as well.
>>>>>>
>>>>>> What I am missing in the new solution[2] is finish method as in other
>>>>>> methods, this will allow cleanup method instance resources.
>>>>>
>>>>>
>>>>>
>>>>> Yes, OpenSSL had the init and finish with "#if 0".
>>>>> and yes they may be needed, I think that is why they left them in the
>>>>> code, but commented out, and why they do not want to expose
>>>>> the ECDSA_METHOD structure.
>>>>
>>>>
>>>> Right, you have an opened channel with openssl developers, can you
>>>> please ask them to add the finish to the interface?
>>>
>>>
>>> I got my foot in the door, I will see what I can do.
>>> Do you have some example of what you need to do during finish?
>>> I need to make an argument that the finish is needed.
>>>
>>> I believe the RSA finish is called when a key is freed.
>>> There is an engine finish that could also be used.
>>
>> Engine finish does not get a reference to key when released and having
>> engine is not always required.
>>
>> What I used so far is method override that handles multiple instances
>> of keys, attaching the key specific data to the key. Then when finish
>> is called release the key specific data.
>>
>> Reference implementation[1]
>>
>> I can probably create new instance of engine for each instance of key,
>> but I do think that adding that missing finish in ec method to match
>> other methods is the correct solution.
>>
>> [1] https://github.com/alonbl/pkcs11-helper/blob/ec/lib/pkcs11h-openssl.c#L375
>
> Also... reading[1].
>
> What is the reason of forcing allocated?

The OpenSSL developers asked for a ECDSA_METHOD_new and they wanted
a flag that said it was allocated, so the ECDSA_METHOD_free would
not try to free a static structure. The default method structures
are all currently static. And since the definition of the structure
is held in the internal ecs_locl.h, only engines built withing the
OpenSSL souce can have static method structures.

Just doing what they asked...

Let's say I want to add flags
> to existing non-allocated method?

Normally you would not do that, since the methods in effect is static,
and could be used by other routines for other keys.

>
> +void ECDSA_METHOD_set_flags(ECDSA_METHOD *ecdsa_method, int flags)
> +       {
> +    /*  ecdsa_method->flags = flags | ECDSA_METHOD_FLAG_ALLOCATED; */

/* More like this to preserve the value of ECDSA_METHOD_FLAG_ALLOCATED */

  ecdsa_method->flags = flags | (ecdsa_method->flags & ECDSA_METHOD_FLAG_ALLOCATED);

> +       }
>
> Also:
>
> When is free expected to be called? I would expect that for
> EC_KEY_free or similar.

No. The same method is normally shared with all keys using the same engine.
The method does not have any key data. It may have some engine
specific routines.

See:
https://github.com/dengert/engine_pkcs11/commit/4870dafd4ff1f586288a016af781498487427c20

The free is only called when the engine is destroyed.


> Even if we add finish method, do you expect it to call free using its
> own reference?

No, See above code in engine,
and
https://github.com/dengert/libp11/blob/master/src/p11_ec.c

lines 256 to 273 use new ECDSA_METHOD functions the OpenSSL
developers asked for.

The ECDSA_METHOD_new(NULL); is called only once when engine created,
and ECDSA_METHOD_free(ops) only once when engine is freed.

Lines 277 to 296 use the ecs_locl.h to create a static version,
with line 285 coping the structure, and lines 286 and 287
setting the two functions needed.

(On Monday, I am gint to run valgrind againts all this to see
if I have memory leaks which might require a finish routine
that is called when a key is freed.)

Think of the method as a C++ or JAVA class, with the key being
an instance of the class. And each engine being a virtual implementation
of the class. (note the correct C++ or Java terms, but I hope you
get the idea.)

>
> +void ECDSA_METHOD_free(ECDSA_METHOD *ecdsa_method)
> +       {
> +       if (ecdsa_method->flags & ECDSA_METHOD_FLAG_ALLOCATED)
> +               OPENSSL_free(ecdsa_method);
> +       }
>
> Solution is to never call ECDSA_METHOD_new(), but always overwrite
> fields in existing method... but then the force flags is something in
> the way.

No, if you had a complicate openssl script with multiple engines,
or where using keys from software and some keys from an engine, you
might be overwriting the software function pointers with the engines
functions pointer, causes all sorts of problems.



>
> Alon
>
> [1] http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=94c2f77a62be7079ab1893ab14b18a30157c4532
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> I am far from being openssl expert, but I did expect to see these
>>>>>> do_sign and sign_setup to accept ecdsa as parameter and not ec...
>>>>>
>>>>>
>>>>>
>>>>> Since both ECDSA and ECDH methods can use EC keys, they split up
>>>>> some of that. Look at the  ecdsa_check()
>>>>>    "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA
>>>>> structure"
>>>>> That then points to the ECDSA_METHOD.
>>>>> and the ecdh_check()
>>>>
>>>>
>>>> Yes, I saw that, but still, to be consistent and remove this check for
>>>> all functions they could have just have two set of "keys", as once EC
>>>> set with DSA or DH it cannot be used for the other.
>>>
>>>
>>> Not really. A key is a key, you can use it with either. A prime
>>> example: an EC key on the smart card that is intended for key derivation,
>>> (ECDH) needs a matching certificate. The certificate request needs
>>> to be signed using ECDSA. That is how I have been testing
>>> the engine to sign a cert request.
>>
>> As far as I can see the EC_KEY can be initialized for one at a time,
>> and it uses casting of method to reference the right methods... so
>> these are really two wrapped in one.
>>
>>>
>>>>
>>>>> It might be the ecdsa_data_st, need to have the finish It has an init.
>>>>> the inti or finish might depend  on how the key is used last.
>>>>>
>>>>> I don't thing the OpenSSL developers have a good handle on what
>>>>> the engine might need.
>>>>
>>>>
>>>> Right... but I will be very happy to see that finish :)
>>>>
>>>> Thanks!!!
>>>>
>>>>>
>>>>> My ecdsa code may also have memory leaks too...
>>>>>
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Alon
>>>>>>
>>>>>> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
>>>>>> [2]
>>>>>>
>>>>>> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>>>>>>
>>>>>
>>>>> --
>>>>>
>>>>>    Douglas E. Engert  <DEE...@an...>
>>>>>    Argonne National Laboratory
>>>>>    9700 South Cass Avenue
>>>>>    Argonne, Illinois  60439
>>>>>    (630) 252-5444
>>>>
>>>>
>>>
>>> --
>>>
>>>   Douglas E. Engert  <DEE...@an...>
>>>   Argonne National Laboratory
>>>   9700 South Cass Avenue
>>>   Argonne, Illinois  60439
>>>   (630) 252-5444
> .
>

-- 

  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

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

On Sat, Sep 21, 2013 at 10:21 PM, Douglas E. Engert <dee...@an...> wrote:
>
>
> On 9/21/2013 10:29 AM, Alon Bar-Lev wrote:
>>
>> On Fri, Sep 20, 2013 at 7:08 PM, Alon Bar-Lev <alo...@gm...>
>> wrote:
>>>
>>> On Fri, Sep 20, 2013 at 4:09 PM, Douglas E. Engert <dee...@an...>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 9/20/2013 1:19 AM, Alon Bar-Lev wrote:
>>>>>
>>>>>
>>>>> On Fri, Sep 20, 2013 at 12:51 AM, Douglas E. Engert <dee...@an...>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/19/2013 4:02 PM, Alon Bar-Lev wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Sep 19, 2013 at 11:31 PM, Douglas E. Engert
>>>>>>> <dee...@an...>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Modifications to engine_pkcs11 and libp11 to support ECDSA
>>>>>>>> are available at github for testing, and I am looking for
>>>>>>>> comments.
>>>>>>>>
>>>>>>>> https://github.com/dengert/libp11
>>>>>>>>
>>>>>>>> https://github.com/dengert/engine_pkcs11
>>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> This is great, I also recently updated pkcs11-helper[1] to support
>>>>>>> ecdsa as well.
>>>>>>>
>>>>>>> What I am missing in the new solution[2] is finish method as in other
>>>>>>> methods, this will allow cleanup method instance resources.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Yes, OpenSSL had the init and finish with "#if 0".
>>>>>> and yes they may be needed, I think that is why they left them in the
>>>>>> code, but commented out, and why they do not want to expose
>>>>>> the ECDSA_METHOD structure.
>>>>>
>>>>>
>>>>>
>>>>> Right, you have an opened channel with openssl developers, can you
>>>>> please ask them to add the finish to the interface?
>>>>
>>>>
>>>>
>>>> I got my foot in the door, I will see what I can do.
>>>> Do you have some example of what you need to do during finish?
>>>> I need to make an argument that the finish is needed.
>>>>
>>>> I believe the RSA finish is called when a key is freed.
>>>> There is an engine finish that could also be used.
>>>
>>>
>>> Engine finish does not get a reference to key when released and having
>>> engine is not always required.
>>>
>>> What I used so far is method override that handles multiple instances
>>> of keys, attaching the key specific data to the key. Then when finish
>>> is called release the key specific data.
>>>
>>> Reference implementation[1]
>>>
>>> I can probably create new instance of engine for each instance of key,
>>> but I do think that adding that missing finish in ec method to match
>>> other methods is the correct solution.
>>>
>>> [1]
>>> https://github.com/alonbl/pkcs11-helper/blob/ec/lib/pkcs11h-openssl.c#L375
>>
>>
>> Also... reading[1].
>>
>> What is the reason of forcing allocated?
>
>
> The OpenSSL developers asked for a ECDSA_METHOD_new and they wanted
> a flag that said it was allocated, so the ECDSA_METHOD_free would
> not try to free a static structure. The default method structures
> are all currently static. And since the definition of the structure
> is held in the internal ecs_locl.h, only engines built withing the
> OpenSSL souce can have static method structures.
>
> Just doing what they asked...

I do not understand why you force this flag when
ECDSA_METHOD_set_flags, this means that this method is unusable for
static allocated method, continuing that logic you should have set
this flag also when setting method callbacks.

> Let's say I want to add flags
>>
>> to existing non-allocated method?
>
>
> Normally you would not do that, since the methods in effect is static,
> and could be used by other routines for other keys.

and if you see my reference implementation I create a duplicate.

>>
>> +void ECDSA_METHOD_set_flags(ECDSA_METHOD *ecdsa_method, int flags)
>> +       {
>> +    /*  ecdsa_method->flags = flags | ECDSA_METHOD_FLAG_ALLOCATED; */
>
>
> /* More like this to preserve the value of ECDSA_METHOD_FLAG_ALLOCATED */
>
>  ecdsa_method->flags = flags | (ecdsa_method->flags &
> ECDSA_METHOD_FLAG_ALLOCATED);
>
>> +       }
>>
>> Also:
>>
>> When is free expected to be called? I would expect that for
>> EC_KEY_free or similar.
>
>
> No. The same method is normally shared with all keys using the same engine.
> The method does not have any key data. It may have some engine
> specific routines.

Right, and we need a cleanup method for the key releasing any resource
associate with, just like in RSA and DSA.

>
> See:
> https://github.com/dengert/engine_pkcs11/commit/4870dafd4ff1f586288a016af781498487427c20
>
> The free is only called when the engine is destroyed.

This is one form of implementation.

>> Even if we add finish method, do you expect it to call free using its
>> own reference?
>
>
> No, See above code in engine,
> and
> https://github.com/dengert/libp11/blob/master/src/p11_ec.c
>
> lines 256 to 273 use new ECDSA_METHOD functions the OpenSSL
> developers asked for.
>
> The ECDSA_METHOD_new(NULL); is called only once when engine created,
> and ECDSA_METHOD_free(ops) only once when engine is freed.

Again, this is good when engine is used, not when method is overridden
without engine.

Also, this is good if there is new instance of engine for every key,
which is something that I do not like.

> Lines 277 to 296 use the ecs_locl.h to create a static version,
> with line 285 coping the structure, and lines 286 and 287
> setting the two functions needed.
>
> (On Monday, I am gint to run valgrind againts all this to see
> if I have memory leaks which might require a finish routine
> that is called when a key is freed.)
>
> Think of the method as a C++ or JAVA class, with the key being
> an instance of the class. And each engine being a virtual implementation
> of the class. (note the correct C++ or Java terms, but I hope you
> get the idea.)

this is incorrect as there is no destructor (finish)...
and engine does not get the instance, unless is instantiate for every key.

>
>
>>
>> +void ECDSA_METHOD_free(ECDSA_METHOD *ecdsa_method)
>> +       {
>> +       if (ecdsa_method->flags & ECDSA_METHOD_FLAG_ALLOCATED)
>> +               OPENSSL_free(ecdsa_method);
>> +       }
>>
>> Solution is to never call ECDSA_METHOD_new(), but always overwrite
>> fields in existing method... but then the force flags is something in
>> the way.
>
>
> No, if you had a complicate openssl script with multiple engines,
> or where using keys from software and some keys from an engine, you
> might be overwriting the software function pointers with the engines
> functions pointer, causes all sorts of problems.

Again, I've never seen that problems, and as far as I can see the
method is duplicated.

>
>
>
>>
>> Alon
>>
>> [1]
>> http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=94c2f77a62be7079ab1893ab14b18a30157c4532
>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> I am far from being openssl expert, but I did expect to see these
>>>>>>> do_sign and sign_setup to accept ecdsa as parameter and not ec...
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Since both ECDSA and ECDH methods can use EC keys, they split up
>>>>>> some of that. Look at the  ecdsa_check()
>>>>>>    "checks whether ECKEY->meth_data is a pointer to a ECDSA_DATA
>>>>>> structure"
>>>>>> That then points to the ECDSA_METHOD.
>>>>>> and the ecdh_check()
>>>>>
>>>>>
>>>>>
>>>>> Yes, I saw that, but still, to be consistent and remove this check for
>>>>> all functions they could have just have two set of "keys", as once EC
>>>>> set with DSA or DH it cannot be used for the other.
>>>>
>>>>
>>>>
>>>> Not really. A key is a key, you can use it with either. A prime
>>>> example: an EC key on the smart card that is intended for key
>>>> derivation,
>>>> (ECDH) needs a matching certificate. The certificate request needs
>>>> to be signed using ECDSA. That is how I have been testing
>>>> the engine to sign a cert request.
>>>
>>>
>>> As far as I can see the EC_KEY can be initialized for one at a time,
>>> and it uses casting of method to reference the right methods... so
>>> these are really two wrapped in one.
>>>
>>>>
>>>>>
>>>>>> It might be the ecdsa_data_st, need to have the finish It has an init.
>>>>>> the inti or finish might depend  on how the key is used last.
>>>>>>
>>>>>> I don't thing the OpenSSL developers have a good handle on what
>>>>>> the engine might need.
>>>>>
>>>>>
>>>>>
>>>>> Right... but I will be very happy to see that finish :)
>>>>>
>>>>> Thanks!!!
>>>>>
>>>>>>
>>>>>> My ecdsa code may also have memory leaks too...
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Alon
>>>>>>>
>>>>>>> [1] https://github.com/alonbl/pkcs11-helper/commits/ec
>>>>>>> [2]
>>>>>>>
>>>>>>>
>>>>>>> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=HEAD;hp=96006022671b4db342a4dcfc3d96edbb3337bb4e
>>>>>>>
>>>>>>
>>>>>> --
>>>>>>
>>>>>>    Douglas E. Engert  <DEE...@an...>
>>>>>>    Argonne National Laboratory
>>>>>>    9700 South Cass Avenue
>>>>>>    Argonne, Illinois  60439
>>>>>>    (630) 252-5444
>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>>
>>>>   Douglas E. Engert  <DEE...@an...>
>>>>   Argonne National Laboratory
>>>>   9700 South Cass Avenue
>>>>   Argonne, Illinois  60439
>>>>   (630) 252-5444
>>
>> .
>>
>
> --
>
>  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/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.

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

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

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

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

On 09/21/2013 11:45 PM, Markus Kötter wrote:
> And I even tried to do it, n with ECDSA but fixing the memory leaks of
> the RSA code.

I forked and created a branches.
https://github.com/commonism/libp11/commits/memoryleaks
https://github.com/commonism/engine_pkcs11/commits/memoryleaks


libp11 leaking the ERR strings when unloading the engine:

https://github.com/commonism/libp11/commit/46dacbe8f5badde89d25289faab82232311822b4
https://github.com/commonism/engine_pkcs11/commit/67244a1cef3decc5b896be5adb9dd771262ab37a

engine_pkcs11 free's the mallocs claimed by PKCS11_ structures.
https://github.com/commonism/engine_pkcs11/commit/fbae0727e88fd20e1cba6ec60799dc4fe705cf97

> Problems ...
>
> idx 0 - which is returned by default, is used by p11 already as app_data.

I have a funny workaround in place.

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

Not fixable.
"Therefore - better not un-load the engine."
Currently this is broken for all dynamic engines.

> The API exposed by OpenSSLs ex_data.c is not sufficient to remove the
> slot manually.

Once OpenSSL comes up with a way to get rid of a claimed slot, I will 
update this.

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

Fixed, remembering the claimed PKCS11_KEY and setting the evp_key NULL 
before destroying the rest prevents the recursion easily.


If we can agree this is a proper approach to free the claimed memory, 
I'll make this work for certs as well, currently only keys are taken 
care of.


MfG
Markus

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

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

On 09/22/2013 10:43 AM, Markus Kötter wrote:
> libp11 leaking the ERR strings when unloading the engine:
>
> https://github.com/commonism/libp11/commit/46dacbe8f5badde89d25289faab82232311822b4
> https://github.com/commonism/engine_pkcs11/commit/67244a1cef3decc5b896be5adb9dd771262ab37a
>
> engine_pkcs11 free's the mallocs claimed by PKCS11_ structures.
> https://github.com/commonism/engine_pkcs11/commit/fbae0727e88fd20e1cba6ec60799dc4fe705cf97
>

Combined savings when ...
loading 100 keys

before:
==12577== LEAK SUMMARY:
==12577==    definitely lost: 10,409 bytes in 112 blocks
==12577==    indirectly lost: 705,606 bytes in 14,710 blocks
==12577==      possibly lost: 0 bytes in 0 blocks
==12577==    still reachable: 1,684 bytes in 4 blocks
==12577==         suppressed: 0 bytes in 0 blocks

after:
==11916== LEAK SUMMARY:
==11916==    definitely lost: 7,209 bytes in 112 blocks
==11916==    indirectly lost: 406 bytes in 10 blocks
==11916==      possibly lost: 0 bytes in 0 blocks
==11916==    still reachable: 1,684 bytes in 4 blocks
==11916==         suppressed: 0 bytes in 0 blocks


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

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


MfG
Markus

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

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

[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

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

[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