-
Notifications
You must be signed in to change notification settings - Fork 742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trustonic pkcs11 #1600
Trustonic pkcs11 #1600
Conversation
…n 1025 bytes. In this case, the signature length given to C_VerifyFinal() was incorrect.
… value in call to util_fatal().
… value in call to util_fatal(). fix formatting.
…RSA and supports decryption, otherwise skip it.
…OAEP params for CKM_RSA_PKCS_OAEP, I had an issue with a variable not initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. I added few comments to the code that would be nice to have improved before merging.
For the tests, I see that you actually updated the pkcs11-tool --test
itself, but that is not ran during the tests itself yet, but a test like the following should be enough (just running the --test
against the softhsm and checking the exit code):
https://github.com/Jakuje/OpenSC/commits/tests-oaep
This obviously fails with current master.
src/tools/pkcs11-tool.c
Outdated
@@ -4650,7 +4652,9 @@ static int test_signature(CK_SESSION_HANDLE sess) | |||
CKM_RSA_PKCS, | |||
CKM_SHA1_RSA_PKCS, | |||
CKM_MD5_RSA_PKCS, | |||
#ifndef OPENSSL_NO_RIPEMD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do not indent the #ifdef
and #endif
macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by 3d09823
src/tools/pkcs11-tool.c
Outdated
@@ -5231,7 +5235,9 @@ static int test_unwrap(CK_SESSION_HANDLE sess) | |||
errors += wrap_unwrap(sess, EVP_des_cbc(), privKeyObject); | |||
errors += wrap_unwrap(sess, EVP_des_ede3_cbc(), privKeyObject); | |||
errors += wrap_unwrap(sess, EVP_bf_cbc(), privKeyObject); | |||
#ifndef OPENSSL_NO_CAST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do not indent the #ifdef
and #endif
macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by 3d09823
src/tools/pkcs11-tool.c
Outdated
EVP_PKEY_free(pkey); | ||
encrypted_len = outlen; | ||
|
||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also explicitly check for CKM_RSA_PKCS
as the decryption does so we can fail earlier if the user provided unsupported mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, we need to check explicitly for CKM_RSA_PKCS and properly handle the case of CKM_RSA_X_509.
I will try to fix this issue during the week.
src/tools/pkcs11-tool.c
Outdated
case CKM_RSA_PKCS_OAEP: | ||
case CKM_RSA_X_509: | ||
//case CKM_RSA_PKCS_TPM_1_1: | ||
//case CKM_RSA_PKCS_OAEP_TPM_1_1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these comments here? Do you have a pkcs11 that presents them? Does it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two mechanisms are defined in the "official" header files that I am using and described in the 2.40 specifications.
They don't exist in the header files of your project, this is why I have commented them.
My pkcs11 won't support them, so I won't be able to test.
I can remove these two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 lines removed @d25fbe3
…S and CKM_RSA_PKCS_OAEP. For these alogs, only CKM_SHA_1 is supported.
I tested my new changes with 2 keys: |
With OPENSSL_VERSION=1.0.1j, the digest of OAEP is hardcoded to SHA-1. |
OpenSSL 1.0.1 is outdated and should not be used in new designs. SHA1 has been deprecated and must not be used, let alone hardcoded. Please change to OpenSSL 1.0.2 (at least) and SHA2. |
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing my comments. It looks much better now. I did another read-through and noticed few more inconsistencies. It would be nice if you could check them too.
src/tools/pkcs11-tool.c
Outdated
hash_alg = CKM_SHA_1; | ||
} else if (opt_hash_alg != CKM_SHA_1) { | ||
printf("Only CKM_RSA_PKCS_OAEP with CKM_SHA_1 supported\n"); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I see right, here you reject any other opt_hash_alg
than CKM_SHA1
(default) and on the line 5346 you again check for all the other possible hashes that can not happen. I think you should allow other hashes to be used too here for the sake of universality (might need some more switches for the openssl).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this code now I am able to test against OpenSsl 1.1.a.
…ith OpenSsl 1.1.1a.
I have noticed something weird with OpenSsl 1.1.1a. |
https://travis-ci.org/OpenSC/OpenSC/jobs/489960037:
Can you debug the crash with gdb, valgrind or VS? |
Hi Frank, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more changes suggested. Anyway I see you are trying and learning :)
src/tools/pkcs11-tool.c
Outdated
if (hash_alg != CKM_SHA_1) { | ||
printf("This version of OpenSsl only supports SHA1 for OAEP, returning\n"); | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use the tabs for indentation here and above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is my editor which is defaulting to spaces instead of tabs as by default we don't use tabs in our software.
Do you have a kind of checkpatch or astyle script that I can run before submitting patches?
This is fixed by 9ae507c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not. If you have some recommendation for service or even just a script, contributions are welcomed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jakub asked about unifying encrypt_decrypt()
and decrypt()
, what do you think about that?
…n encrypt_decrypt().
It is true that I originally copied code from decrypt_data() but since I have done many changes and I don't think that it is worth to try to factorize it otherwise it will be less readable. Now it is working fine for me with my PKCS#11 implementation and the latest OpenSsl. |
Note, that it does not work now until OpenSC#1600 will get resolved. Then, move the test to TESTS in the Makefile.am
Note, that it does not work now until OpenSC#1600 will get resolved. Then, move the test to TESTS in the Makefile.am
I tried to figure out why I have a seg fault with the OpenSSL 1.1.1a. 01-21 23:14:33.801 23399 23399 F DEBUG : ABI: 'arm' |
@mtrojnar, any ideas regarding the problem with OpenSSL 1.1.1? |
@frankmorgner It is hard to draw any conclusions without a complete stack backtrace. |
When OPENSSL_init_crypto() is commented out in pkcs11-tool.c, engine_destroy() is called before err_cleanup() and I don't have the seg fault. OPENSSL_init_crypto() is not needed anymore with OpenSSL 1.1.1a. |
…a segmentation fault when the process exits.
both, pkcs11-tool and libp11 are calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the PR looks good. There were still two minor questions without response...
The segmentation fault is a problem, however. That may be a sign that we're getting some of OpenSSL's memory handling wrong. Would it be possible to debug with some other PKCS#11 module, maybe softhsm?
Note, that it does not work now until OpenSC#1600 will get resolved. Then, move the test to TESTS in the Makefile.am
Note, that it does not work now until #1600 will get resolved. Then, move the test to TESTS in the Makefile.am
Hi Frank, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now
When I try to build your branch I am getting the following failures:
|
When I fixed the above and tried to run the OEAP tests with pkcs11-tool, I am still getting failures:
(from |
I am trying to understand why mgf is not initialized. |
Maybe your PKCS11 implementation does not support this mechanism? |
OK, it looks like the SoftHSM supports only SHA1 hash and MGF1, which is the reason for the failure here. But the build issue should be addressed. From what I see, it really looks like false positive of my gcc, but assigning the default value during initialization of a variable is a good practice. |
I initialized the variable as suggested in my latest commit. |
Thanks. I think we are good. |
thanks everyone |
Thanks for your help Jakub and Frank! |
Hi,
In this pull request I am mainly adding the support for CKM_RSA_PKCS_OAEP for the "pkcs11-tool --test" command.
I am also fixing the "pkcs11-tool --verify" when the file to verify is larger than 1025 bytes.
Finally, I fixed a minor build issue because RIPEMD and CAST are not defined by my version of OpenSsl.
I have only tested with my own implementation of the PKCS#11 library.
Notice that this is my first contribution so let me know if I am not doing the things correctly.
Best Regards,
Alexandre.