Support OpenSSL 1.1

[mtrojnar]

The OpenSSL 1.1 API removes direct application access to most internal data structures in favour of getters and setters. This improves binary compatibility, but requires significant changes in the applications.

[dengert]

dengert/engine_pkcs11@222ca6a
dengert@571f4b2

Contain changes for engine and libp11 to get it to work with OpenSSL-1.1. These are not complete,
and OpenSSL-1.1 is not even in beta. The code compiles with OpenSSL 1.0.1 to 1.1.
The main reasone to use OpenSSL 1.1 is ECDH from the engine. This is not yet in thes mods.

Many of these OpenSSL changes are in hidding structures in favor of access functions.
OpenSSL 1.1 is still in development and the first beta has not been released.
In the engine area and especially with ECC there are major changes:

ECDSA_METHOD and ECDH_METHOD are combined into EC_KEY_METHOD, and some functions used to set the method
are missing, such as EC_KEY_set_method. Mods for these are under review and should be available by the end of Jan 2016
and in the first beta. (The changes in this patch are using a modified version of OpenSSL-1.1 that should be the same as those due to be released.

For testing I have been using openssl req and req.c needed some changes to get it to use an engine.
I will be submitting these to OpenSSL. It looks like many of the other apps may have the same problem.

The EC_KEY_METHOD for ECDSA now has 3 functions rather then 2.
I have not looked at the ECDH function.

Some libp11 routines are being renamed:
PKCS11_get_ecdsa_method to PKCS11_get_ec_key_method
PKCS11_ecdsa_method_free to PKCS11_ec_key_method_free
ecdsa_ex_index to ec_key_ex_index

ERR_remove_state is now deprecated, and to get these changes, compiled, It is replaced by ERR_remove_thread_state.
This is used in examples/decrypt and src/p11_load.c

BIGNUM is now hidden, and there is no way to set r and s directly into a ECDSA_SIG, so a new trick
of using d2i_ECDSA_SIG and ECDSA_SIG_get0 to get access to r and s has been added.

RSA_generate_key has changed and needs a BN_GENCB and the exponent is a BN now too.

Crypto_get_new_dynlockid has changed. So to get these change to compile, priv->lockid is not used.
The code gives errors, but does run. Look at src/crypto/engine/eng_dyn.c for
fns.static_state = ENGINE_get_static_state();
ctx->bind_engine(e, ctx->engine_id, &fns)
It looks like the fns can be passed to the engine.

RSA_FLAG_SIGN_VER is not longed needed in OpenSSL 1.1.

The original p11_ec.c was originally written to use the internal EC header files and structures. This is being dropped in favor of a OpenSSL-1.0.2 version for ECDSA only and the OpenSSL-1.1 that can support both ECDSA and ECDH.

I have been able to use a command like:
openssl req -config /tmp/genreq.25025.openssl.conf -engine pkcs11 -keyform e -sha256 -new -key slot_1-id_2 -out /tmp/selfsigned.pem -x509 -text

The openssl.conf needed some changes from previous versions.
Contact me if you want the openssl change or wait for a new release of 1.1.

[mtrojnar]

My roadmap is:

• Release libp11 and engine_pkcs11: next week
• Merge engine_pkcs11 into libp11 #50 Merge engine_pkcs11 into libp11 distribution: end of January
• Support OpenSSL 1.1 #52 Add support for OpenSSL 1.1: end of February

@dengert: I have pushed c219965 to gracefully handle 0 returned as an invalid dynamic lock ID.

[mtrojnar]

@dengert The dynamic locking callbacks from fns are already passed to the OpenSSL library used by engine_pkcs11. See the IMPLEMENT_DYNAMIC_BIND_FN macro in engine.h for details.

A similar interface could be implemented to pass the callbacks further down to libp11. This may be useful in case engine_pkcs11 and libp11 use separate instances of OpenSSL.

[dengert]

OK. Buit it would appear that engine_pkcs11 and libp11 must use the same instance of OpenSSL,
because they pass pointers to internal OpenSSL structures and functions.
In a note on Jan 12, 2016 you said:
"I guess OpenSC and other PKCS#11 providers may safely use a different version of OpenSSL, because OpenSSL objects are not exchanged over the PKCS#11 API. Please correct me if I'm wrong."
I believe your statement would also say engine_pkcs11 and libp11 have to use the same instance.

[mtrojnar]

"The same instance" also means "the same version". The other way around is not necessarily true.

Example: Imagine an executable with statically linked OpenSSL using a dynamic engine. Both the executable and the engine may use the same version of OpenSSL. It should be possible to pass objects created on the shared heap from one OpenSSL to the other (at least some of the objects depending on the internal references to other objects). On the other hand, the application and the engine use different instances of OpenSSL. Both libraries will keep any static data (for example the registered locking callbacks) in separate locations.

Also, it is possible that even different (but close) versions of OpenSSL will retain the same internal data representation for some objects.

[dengert]

What about one instance allocating something placed in a structure
that is passed to another instance  that then tries to free it. 
It is getting very messy. 

This sounds like the problem Mouse was having on MacOS. His were
different versions, but I could see other problems. 
There was a comment or a note I saw on the OpenSSL mail list about
trying to determine if two libs were using two diffirent instances.
 

On 1/15/2016 8:14 AM, Michał Trojnara
  wrote:


  "The same instance" also means "the same version". The other
    way around is not necessarily true.
  Example: Imagine an executable with statically linked OpenSSL
    using an engine. Both the executable and the engine may use the
    same version of OpenSSL. It should
    be possible to pass objects created on the shared heap from one
    OpenSSL to the other (at least some of the objects depending on
    the internal references to other objects). On the other hand,
    the application and the engine use different instances
    of OpenSSL. Both libraries will keep any static data (for
    example the registered locking callbacks) in separate locations.
  Also, it is possible that even different (but close) versions
    of OpenSSL will retain the same internal data representation for
    some objects.
  —
    Reply to this email directly or view
      it on GitHub.









-- 

Douglas E. Engert DEEngert@gmail.com

[mouse07410]

Rather than spending efforts on elaborate checking of version/instance compatibility, I'd prefer to see engine_pkcs11 and libp11 merged into one.

[mtrojnar]

What about one instance allocating something placed in a structure that is passed to another instance that then tries to free it. It is getting very messy.

@dengert: I beg to differ. I, however, fail to see the possible practical outcome of discussing this topic any further.
@mouse07410: Good point.

[mtrojnar]

@dengert I closely observe https://github.com/dengert/libp11/commits/prep-openssl-1.1 and I like your changes. I hope to be able to merge them next week.

[dengert]

Should decied on the API for PKCS11_ecdh_derive or make it an internal routine for now?

[mtrojnar]

I think the proper interface is to register (with EVP_PKEY_meth_set_derive()) a key derivation method for the value returned by pkcs11_get_evp_key_ec(). No new external API should be added, and the internal complexity should be hidden as much as possible.

One of my long-term goals is to deprecate, and subsequently remove any algorithm-specific functions (for example PKCS11_get_key_modulus() or PKCS11_sign()). The same functionality can be achieved with PKCS11_get_private_key()/PKCS11_get_public_key() and subsequent OpenSSL operations on the returned EVP object. The redundancy only introduces unnecessary complexity. Using algorithm-specific functions also reduces portability of applications. A well-written application should not even need to know the key types it uses, so it does not need to be rewritten every time someone introduces a new class of curves. The application should simply execute "sign" or "derive" method, and the underlying library should perform the operation as long as it is supported by the selected key type.

What do you think?

[dengert]

That has some merit.  Some  other considerations:

PKCS#11, OpenSSL, and LIBP11 are all trying to implement
  overlapping functionality. Including  hashing, key derivation, and
  generalized crypto functions. Since EC keys can not do
  encrypt/decrypt directly, but must use derivation and key wrapping
  there will be differences. And ECDH derivation requires the
  EC_POINT of the another key. 

It is not clear if registering with EVP_PKEY_meth_set_derive
  will work. I have not looked at it close enough to see if that
  could work. But then you would have to reproduce in libp11 much of
  the code in OpenSSL that determines the key types. 

  The approach of doing the hooks at the lowest possible level, lets
  OpenSSL do what it does best,
  and only requires a small amount of code in engine_pkcs11 and
  libp11.   

PKCS#11 uses mechanisms that are dependent on the type of key, and
PKCS#11 does not try and do the generic operations, but key type
specific operations. 
OpenSSL uses an IETF/ANSI/OID style mechanisms which OpenSSL does a
good job of  handling. It is much easier to let OPenSSL break these
up into
the actions needed, and then let the hook in the key method call out
key specific code in engine_pkcs11 and libp11.

There are IETF RFC on how to use EC in place of RSA, and these
require different approaches especially .  It is not clear how
generic you can get. 

In the short term, I think keeping the ECDSA and ECDH hooks for
engine at least should be done. 

On 1/20/2016 11:49 AM, Michał Trojnara
  wrote:


  I think the proper interface is to register (with EVP_PKEY_meth_set_derive())
    a key derivation method for the value returned by pkcs11_get_evp_key_ec().
    No new external API should be added, and the internal complexity
    should be hidden as much as possible.
  One of my long-term goals is to deprecate, and subsequently
    remove any algorithm-specific functions (for example PKCS11_get_key_modulus()
    or PKCS11_sign()). The same functionality can be
    achieved with PKCS11_get_private_key()/PKCS11_get_public_key()
    and subsequent OpenSSL operations on the returned EVP object.
    The redundancy only introduces unnecessary complexity. Using
    algorithm-specific functions also reduces portability of
    applications. A well-written application should not even need to
    know the key types it uses, so it does not need to be rewritten
    every time someone introduces a new class of curves. The
    application should simply execute "sign" or "derive" method, and
    the underlying library should perform the operation as long as
    it is supported by the selected key type.
  What do you think?
  —
    Reply to this email directly or view
      it on GitHub.









-- 

Douglas E. Engert DEEngert@gmail.com

[mtrojnar]

PKCS#11, OpenSSL, and LIBP11 are all trying to implement overlapping functionality.

Indeed. I guess the idea was to make PKCS#11 usable without any additional cryptographic library. This is essential to achieve higher levels of the FIPS 140-2 certification, but not really useful for libp11, which is already linked against OpenSSL.

It is not clear if registering with EVP_PKEY_meth_set_derive will work.

If it doesn't work, it is something to be fixed in OpenSSL, and not libp11.

But then you would have to reproduce in libp11 much of the code in OpenSSL that determines the key types.

Correct. We need to translate OpenSSL key types into PKCS#11 key type specific operations. At the most basic level we use the PKCS11_KEY_ops structure to keep a list of PKCS#11 methods for each OpenSSL key type. I expect PKCS11_KEY_ops to be extended in the future versions of libp11.

PKCS#11 uses mechanisms that are dependent on the type of key, and PKCS#11 does not try and do the generic operations, but key type specific operations.

This is exactly why libp11 is useful. 😃

There are IETF RFC on how to use EC in place of RSA, and these require different approaches especially . It is not clear how generic you can get.

Replacing RSA with EC does not sound like a problem specific to PKCS#11. This is something to be implemented in OpenSSL. I haven't studied the RFC yet, but I don't expect it to require any advanced primitives.

[dengert]

I have converted PKCS11_ecdh_derive to an internal function, and I think
https://github.com/dengert/libp11/tree/prep-openssl-1.1
is ready. Please have a look.

[mtrojnar]

The general idea looks good. The details can be cleaned up later. For example:

    CRYPTO_w_lock(PRIVSLOT(slot)->lockid);

should be removed for several reasons:

1. The implemented functionality does not really require locking. Maybe we should somehow limit the number of created concurrent temporary key objects, as PKCS#11 devices tend to have very limited key storage space...
2. The lock is never unlocked.
3. The libp11 locking functions are pkcs11_w_lock() and pkcs11_w_unlock() (for write locks).

It would be nice to get the same result without temporarily storing the newly created key on the device... Unfortunately, PKCS#11 does not seem to expose any interface to do it.

@dengert When do you expect to have your code ready for merging?

[mtrojnar]

A test case should be added before merging it into libp11.

Alternatively, you may wait we may wait until I merge engine_pkcs11 into libp11 (in about 3 weeks), and build the test cases with the OpenSSL commandline tools.

@dengert What do you prefer?

[dengert]

The code for engine and libp11 is ready now and works with OpenSSL-1.1-pre2
I will submit pull requests for engine and libp11.

Much of the patch is converting the code to use functions to access now hidden structures. This has been done is a way to make it compatable from versions 0.9.8 to 1.1-pre2.
#if OPENSSL_VERSION_NUMBER
is used a lot in the patch.

I would like to get the patch applied as it is now, as it is getting very difficult to rebase and eventially developers wil need to face the issues of converting to 1.1. Getting the code in place will allow a developer to still use previous versions of OpenSSL and to test against 1.1. This needs to be more then a one person effort. I have changed a lot of code that only others can test.

OpenSSL-1.1 will hides many previously exposed structures, such as BIGNUM, X509 and EVP_MD_CTX. Past versions of OpenSSL provided macros and functions to access member of the structures, but most code still used pointers to structure members or allocated a structure on the stack such as BIGNUM or EVP_MD_CTX on the stack.

Also note the patch removes the code to compile using the BUILD_WITH_ECS_LOCL_H
which used the internal ecdsa/ecs_locl.h from the OpenSSL source. Thus 1.0.2 is needed for ECDSA support with ECDH been added in 1.1-pre2.

Additional changes in OpenSSL github master for 1.1-pre3 have also hidden EVP_PKEY!
so statments like pubkey->pkey.rsa need to be replaced by EVP_PKEY_get0_RSA(pubkey).
and pubkey->type by EVP_PKEY_base_id(pubkey) I have additional patches to fixes these but am waiting till pre3 is released.

[mtrojnar]

@dengert It's a good plan. I'm waiting for your pull requests.

You likely have some code to test key derivation. Please consider submitting it examples/ or tests/ (depending on its potential educational value). I can take care of the further integration.

[mtrojnar]

I would like to get the patch applied as it is now, as it is getting very difficult to rebase

Okay. I merged it.

and eventially developers wil need to face the issues of converting to 1.1.

I guess at this point of time it mostly affects me. "Developers" will generally only touch the code after it's stabilized. I really need a testing application...

[mtrojnar]

    if (newkey != CK_INVALID_HANDLE && session != CK_INVALID_HANDLE);
        rv = CRYPTOKI_call(ctx, C_DestroyObject(session, newkey));

@dengert Have you performed any tests on this code?

[dengert]

Yes. Note that the object is a session object, and may be in memory depending the PKCS#11 module and the token. It is created as part of the C_DeriveKey from this template.

60 CK_ATTRIBUTE newkey_template[] = {
61 {CKA_TOKEN, &false, sizeof(false)}, /* session only object */
62 {CKA_CLASS, &newkey_class, sizeof(newkey_class)},
63 {CKA_KEY_TYPE, &newkey_type, sizeof(newkey_type)},
64 {CKA_ENCRYPT, &true, sizeof(true)},
65 {CKA_DECRYPT, &true, sizeof(true)}
66 };

Here is a GDB stack trace showing the OpenSC pkcs11 as it is about to clear and free the object. The line in question is at #4

#0 __pkcs15_release_object (obj=0x6ef620) at ../../../src/src/pkcs11/framework-pkcs15.c:435
#1 0x00007ffff68fa2f0 in __pkcs15_delete_object (fw_data=0x6c2de0, obj=0x6ef620) at ../../../src/src/pkcs11/framework-pkcs15.c:456
#2 0x00007ffff690206d in pkcs15_skey_destroy (session=0x6e6a70, object=0x6ef620) at ../../../src/src/pkcs11/framework-pkcs15.c:2891
#3 0x00007ffff68f0991 in C_DestroyObject (hSession=7236208, hObject=7271968) at ../../../src/src/pkcs11/pkcs11-object.c:179
#4 0x00007ffff6b23f0c in pkcs11_ecdh_derive_internal (out=0x7fffffffd3e0, outlen=0x7fffffffd3d8, ecdh_mechanism=4176, ec_params=0x7fffffffd410, outnewkey=0x0, key=0x6ed120)
at ../../src/src/p11_ops.c:137
#5 0x00007ffff6b254ee in pkcs11_ec_ckey (out=0x6efcf0, outlen=48, ecpointpeer=0x6ed9d0, ecdh=0x6e6af0, KDF=0x0) at ../../src/src/p11_ec.c:281
#6 0x00007ffff7800ccd in pkey_ec_derive (keylen=, key=0x6efcf0 "\240\374n", ctx=) at ec_pmeth.c:225
#7 pkey_ec_kdf_derive (ctx=, key=0x7fffffffd4d0 "@", keylen=0x7fffffffd4c0) at ec_pmeth.c:252
#8 0x00007ffff78d41e8 in cms_kek_cipher (kari=0x6ee3d0, kari=0x6ee3d0, enc=0, inlen=40,
in=0x6ee330 "}\364.\234\314\373\256\334?y\034T{p?S4\031\335n\321\356\005\240~\316\354)lh\377\036Hf\214\256\222", <incomplete sequence \325>, poutlen=,
pout=) at cms_kari.c:246
#9 CMS_RecipientInfo_kari_decrypt (cms=cms@entry=0x6ed860, ri=ri@entry=0x6ed680, rek=rek@entry=0x6ee250) at cms_kari.c:289
#10 0x00007ffff78ce3ea in cms_kari_set1_pkey (cert=0x6ac7e0, pk=0x6eb840, ri=0x6ed680, cms=0x6ed860) at cms_smime.c:636
#11 CMS_decrypt_set1_pkey (cms=cms@entry=0x6ed860, pk=pk@entry=0x6eb840, cert=cert@entry=0x6ac7e0) at cms_smime.c:667
#12 0x0000000000422d86 in cms_main (argc=, argv=) at cms.c:1027
#13 0x0000000000419280 in do_cmd (prog=prog@entry=0x6a7ba0, argc=14, argv=0x6a8d30) at openssl.c:644
#14 0x000000000041adac in main (argc=, argv=) at openssl.c:409

The test is run using OpenSSL-1.1-pre2.

[mtrojnar]

My question was about ";" and the end of "if" line.
Also, the returned "rv" does not really seem to be used anywhere.

[dengert]

Oh. 

That extra ";" should not be there. The only code path the engine
actually uses would always call the C_Delete. 
I can remove it  and run test again. 

and rv is not used at this point. 




On 1/26/2016 8:10 AM, Michał Trojnara
  wrote:


  My question was about ";" and the end of "if" line.
    Also, the returned "rv" does not really seem to be used
    anywhere.
  —
    Reply to this email directly or view
      it on GitHub.









-- 

Douglas E. Engert DEEngert@gmail.com

[mtrojnar]

libp11 successfully builds on the current OpenSSL 1.10-dev. The existing tests are also successful. I'm closing this issue.

Please open a new issue for each identified bug. Please include (as a pull request creating a file in the tests directory) an example code (C source or shell script) that demonstrates the bug.