opencryptoki / openssl-ibmca

OpenSSL engine and provider for libica.
Apache License 2.0
6 stars 15 forks source link

ECC support #40

Closed p-steuer closed 6 years ago

p-steuer commented 6 years ago

this is joergs ecc engine code. i got it running with both openssl 1.0.2 (on rhel 7.5) and openssl 1.1.0 (internal driver) and impemented the fallback behavior.

I tested the following cenarios:

I also did some s_serve/s_client tests with ECDHE-ECDSA ciphersuites with both openssl 1.0.2 and 1.1.0, having engine enabled on both sides or on one side only.

So please review and try to provide some additional test coverage. Im still think the code might not be free of memleaks. will look into that tomorrow.

p-steuer commented 6 years ago

hi eduardo

  1. i ll address coding style once everything else is okay.
  2. ok thanks
  3. i didnt see warnings when building on fedora27 (internal), rhel 7.5 and ubuntu 18.04. Where do you see those warnings.
  4. Hmm strange. RSA shouldnt be affected at all. Ill look into this today.
p-steuer commented 6 years ago

okay

i found the rsa problem to be unrelated to this PR:

It has been introduced with de93db5217389a2444626e745965c9f206f6cda3 (affects ibmca 1.4.1). While the commits intention was to prevent cyclic loops with libicas openssl sw fallback (ossl->engine->libica->ossl->engine ...), it was ignored that crypto adapters can only handle up to 4k rsa. Previously, for >4k rsa the sw fallback would be used. We have to think of a solution here @hfreude .

p-steuer commented 6 years ago

okay, i integrated and pushed all feedback i got. Some remarks:

I wait for your review and/or testing feedback.

dodys commented 6 years ago

Can I go ahead and merge it or do we need to wait anything else?

p-steuer commented 6 years ago

yes you can go ahead. i squashed all memleak fixes for the ecc code with the ecc support patch and put one patch including minor memleak fixes not related to ecc on top.

I repeated the tests from my initial comment (speed test plus s_server/s_client tests on fedora and rhel7.5). Everything seems to work smoothly , if you have time run some tests in other environments/distros, your welcome.

Notice that the libica ecc code also has some memleaks and i opened a corresponding libica PR.

dodys commented 6 years ago

Hi Patrick

I'm hitting a failed test in make check on SLES12 SP3: FAIL: 3des-cfb-test.pl the log for this test is empty as well all the other log files. This is the only failure that I've saw. If you cannot reproduce it might be something on my environment. Let me know tomorrow.

p-steuer commented 6 years ago

i found the problem to be unrelated to this PR:

Note that sles12.3 comes with the engine (version 1.3.0) pre-configured (see /etc/ssl/openssl.cnf) per default.

The test cases (make check) do a pairwise-consistency check: 1) encrypt using the engine (which is build in openssl-ibmca/src/.libs ), decrypt using openssl, compare results 2) encrypt using openssl, decrypt using the engine, compare results.

A preconfigured engine means "using openssl" will use the engine v1.3.0 from the sles 12.3 package, so essential the test case will compare results of engine v1.3.0 vs engine v2.0.0/upstream.

So obviously the rseults do not match and question is, which engine is correct? If you disable the engine in /etc/ssl/openssl.cnf (and compare upstream engine to native openssl), the test will pass. That means the problem is not related to this PR i.e. upstream code is correct, but engine v1.3.0 that comes with sles has a bug.

Turns out the bug was "accidentially" fixed by commit c57f442730052ce0b5f08dae3ac0fa17c264ccf8 . So the aes-3des-cfb bug is present in engines up to/including v1.3.0.

From said commit:

-                case DES3_CFB:
-                        ibmca_cipher_lists.nids[*ciph_nid_cnt] = NID_des_ede3_cfb;
-                        ibmca_cipher_lists.crypto_meths[(*ciph_nid_cnt)++] = &ibmca_des_cfb;
-                        break;
[...]
+               case DES3_CFB:
+                       ibmca_cipher_lists.nids[*ciph_nid_cnt] = NID_des_ede3_cfb;
+#ifdef OLDER_OPENSSL
+                       ibmca_cipher_lists.crypto_meths[(*ciph_nid_cnt)++] = &ibmca_tdes_cfb;
+#else
+                       ibmca_cipher_lists.crypto_meths[(*ciph_nid_cnt)++] = ibmca_tdes_cfb();
+#endif

... obviously the latter is correct.

dodys commented 6 years ago

Totally missed the ibmca that was shipped with the distro... removing it the testing is passing. Thanks! Merging now.