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