Hook into EVP_PKEY methods and add RSA-PSS support

[selvanair]

Currently RSA-PSS padding works if the token supports raw RSA signatures using pre-padded data provided by OpenSSL. Many hardware tokens do not support raw RSA and require the padding to be done internally. In those cases one is currently forced to downgrade to TLS 1.1 when applications are built with OpenSSL 1.1.1+

The approach used here may require improvements, so treat this more like an attempt at gauging interest, and as a request for comments.

Particularly, mechanism_type parameter is changed to CK_MECHANISM struct in several externally visible functions. None of those are used in OpenVPN, so for the use case of interest to me, this is still ABI compatible and a drop-in replacement. One could save the mechanism parameters inside a key-specific struct (like certificate object) and avoid this change, but I could not determine a good way to do it.

Only RSA key operations are intercepted at the EVP_PKEY level. Adding other key types would be easy if desired. Also the change is restricted to OpenSSL versions >= 1.1.1. Maybe one should also check LibreSSL version (not done here.)

See commit messages for more details.

TODO: modify and extend tests

Lightly tested on Linux + OpenVPN.

[alonbl]

Hi,

Nice workout!

I may be detached from latest progress in openssl... so forgive me for my ignorance...

You are saying that we need to use EVP_PKEY_RSA instead of the RSA method? If so, why did you leave the RSA methods and not replaced them in openssl 1.1.1? Is there any advantage in hooking the two?

I guess if we do so for RSA we need to do so for DSA and EC as well... this means that it is probably a better idea to have pkcs11h-openssl-1.1.1.c file with a totally different implementation for >=1.1.1 and add detection in autoconf.

I do not fully understand why you have changed the API:

-       IN const CK_MECHANISM_TYPE mech_type,
+       IN CK_MECHANISM * mech,

Also please configure with --enable-strict --enable-pedantic to avoid newer C feature use.

Thanks!

[selvanair]

Hi,

Nice workout!

I may be detached from latest progress in openssl... so forgive me for my ignorance...

You are saying that we need to use EVP_PKEY_RSA instead of the RSA method?

Yes, RSA methods are not capable of handling newer mechanisms like RSA-PSS. The data passed in (padding type) is not enough to determine all parameters required for signing.

If so, why did you leave the RSA methods and not replaced them in openssl 1.1.1? Is there any advantage in hooking the two?

I don't know :) The main reason for leaving them in is that I call them from pkey sign and decrypt methods for cases like RSA_PKCS1_PADDING or RSA_NO_PADDING to save some code duplication. In case of RSA_PKCS1 I actually call back OpenSSL's RSA_sign() which will use the RSA_methods hook to call us back later.

If we move it all to a new file for a cleaner implementation, we could get rid of them unless those hooks are required as per OpenSSL spec -- I will test and also check OpenSSL code.

I guess if we do so for RSA we need to do so for DSA and EC as well... this means that it is probably a better idea to have pkcs11h-openssl-1.1.1.c file with a totally different implementation for >=1.1.1 and add detection in autoconf.

That would be cleaner at the expense of having two files to maintain with some duplicated code. Also, 1.1.1+ is a just a practical choice as that's what makes PSS support mandatory. EVP_PKEY methods are supported in prior versions but would require many ifdefs to detect API changes in OpenSSL over the years.

I do not fully understand why you have changed the API:

-       IN const CK_MECHANISM_TYPE mech_type,
+       IN CK_MECHANISM * mech,

CK_MECHANISM has three fields: CK_MECHANISM_TYPE, a void * pointer for passing additional mechanism-specific parameters, and the parameter data length. The parameters is NULL for all mechanisms currently supported so just passing mechanism_type was okay. But not so for PSS. We need hash type and salt length to construct those parameters for PSS but those are available only where EVP_PKEY_CTX is accessible -- i.e. in the EVP_PKEY sign method callback.

So we need to have a way of passing mechansim parameter to doPrivateOperation from from the sign method callbacks as well as other consumers of the library. Currently CK_MECHANISM struct is constructed on the fly in doPrivateOperation as all but mechanism_type is NULL.

I have an alternate implementation where the mech.pParameter is saved in the certificate struct. But I think it makes the usage of the library less intuitive and error prone. The caller has to set this field before use and clear it after right after signing -- otherwise when the certificate is reused with a different signing mechanism that requires no parameters, the old one's may still be lurking around in the certificate struct.

In other words, signing mechanism is not a static feature of a certificate, but a signing-context-dependent one, and hence doesn't belong there. However, it does avoid changing the API.

However, if avoiding API change is important, I can submit the alternate implementation for comparison.

Also please configure with --enable-strict --enable-pedantic to avoid newer C feature use.

Will do.

[alonbl]

Hi, Nice workout! I may be detached from latest progress in openssl... so forgive me for my ignorance... You are saying that we need to use EVP_PKEY_RSA instead of the RSA method?

Yes, RSA methods are not capable of handling newer mechanisms like RSA-PSS. The data passed in (padding type) is not enough to determine all parameters required for signing.

If so, why did you leave the RSA methods and not replaced them in openssl 1.1.1? Is there any advantage in hooking the two?

I don't know :) The main reason for leaving them in is that I call them from pkey sign and decrypt methods for cases like RSA_PKCS1_PADDING or RSA_NO_PADDING to save some code duplication. In case of RSA_PKCS1 I actually call back OpenSSL's RSA_sign() which will use the RSA_methods hook to call us back later.

Yes, I saw that, and this is why I am confused, as I believe you can avoid that and call the PKCS#11 directly at this point.

If we move it all to a new file for a cleaner implementation, we could get rid of them unless those hooks are required as per OpenSSL spec -- I will test and also check OpenSSL code.

Thanks! My concern is that soon openssl-3 will be out and there is a significant change in this regard. Is it worth to do a rework now?

I guess if we do so for RSA we need to do so for DSA and EC as well... this means that it is probably a better idea to have pkcs11h-openssl-1.1.1.c file with a totally different implementation for >=1.1.1 and add detection in autoconf.

That would be cleaner at the expense of having two files to maintain with some duplicated code. Also, 1.1.1+ is a just a practical choice as that's what makes PSS support mandatory. EVP_PKEY methods are supported in prior versions but would require many ifdefs to detect API changes in OpenSSL over the years.

This is the reason I believe that once the method is different it is easier to maintain a different file than handling different versions and different implementations at a single file.

I do not fully understand why you have changed the API:

-       IN const CK_MECHANISM_TYPE mech_type,
+       IN CK_MECHANISM * mech,

CK_MECHANISM has three fields: CK_MECHANISM_TYPE, a void * pointer for passing additional mechanism-specific parameters, and the parameter data length. The parameters is NULL for all mechanisms currently supported so just passing mechanism_type was okay. But not so for PSS. We need hash type and salt length to construct those parameters for PSS but those are available only where EVP_PKEY_CTX is accessible -- i.e. in the EVP_PKEY sign method callback.

So we need to have a way of passing mechansim parameter to doPrivateOperation from from the sign method callbacks as well as other consumers of the library. Currently CK_MECHANISM struct is constructed on the fly in doPrivateOperation as all but mechanism_type is NULL.

I have an alternate implementation where the mech.pParameter is saved in the certificate struct. But I think it makes the usage of the library less intuitive and error prone. The caller has to set this field before use and clear it after right after signing -- otherwise when the certificate is reused with a different signing mechanism that requires no parameters, the old one's may still be lurking around in the certificate struct.

In other words, signing mechanism is not a static feature of a certificate, but a signing-context-dependent one, and hence doesn't belong there. However, it does avoid changing the API.

However, if avoiding API change is important, I can submit the alternate implementation for comparison.

I understand, but we cannot break the API. We should probably add another set of APIs and delegate the old API to the new.

Many hardware tokens do not support raw RSA and require the padding to be done internally.

Can you please tell me what hardware do you use which does not support raw RSA? I had this issue in the past and discovered that signRecover or private encrypt sometime provide workarounds.

[selvanair]

I don't know :) The main reason for leaving them in is that I call them from pkey sign and decrypt methods for cases like RSA_PKCS1_PADDING or RSA_NO_PADDING to save some code duplication. In case of RSA_PKCS1 I actually call back OpenSSL's RSA_sign() which will use the RSA_methods hook to call us back later.

Yes, I saw that, and this is why I am confused, as I believe you can avoid that and call the PKCS#11 directly at this point.

For PKCS#1-v1.5, what we get in the pkey_rsa_sign call back is the digest without the encoded digest-info added (unlike what rsa_private_enc gets). If we can ask doPrivkeyOperation to add digest-info and then sign, sure that would be better. Calling RSA_sign was my lazy approach as it does the right thing.

If we move it all to a new file for a cleaner implementation, we could get rid of them unless those hooks are required as per OpenSSL spec -- I will test and also check OpenSSL code.

Thanks! My concern is that soon openssl-3 will be out and there is a significant change in this regard. Is it worth to do a rework now?

OpenSSL 1.x is supported until Sept 2023, and may hang around much beyond that. And, this is a relatively small patch :)

I guess if we do so for RSA we need to do so for DSA and EC as well... this means that it is probably a better idea to have pkcs11h-openssl-1.1.1.c file with a totally different implementation for >=1.1.1 and add detection in autoconf.

That would be cleaner at the expense of having two files to maintain with some duplicated code. Also, 1.1.1+ is a just a practical choice as that's what makes PSS support mandatory. EVP_PKEY methods are supported in prior versions but would require many ifdefs to detect API changes in OpenSSL over the years.

This is the reason I believe that once the method is different it is easier to maintain a different file than handling different versions and different implementations at a single file.

Okay.

However, if avoiding API change is important, I can submit the alternate implementation for comparison.

I understand, but we cannot break the API. We should probably add another set of APIs and delegate the old API to the new.

Will do.

Many hardware tokens do not support raw RSA and require the padding to be done internally.

Can you please tell me what hardware do you use which does not support raw RSA? I had this issue in the past and discovered that signRecover or private encrypt sometime provide workarounds.

E.g., Safenet eToken 5110 FIPS doesn't seem to support RSA-X-509 for any operations.

[selvanair]

I'm mostly ready to push changes as discussed but not sure of a couple of things:

• Currenetly the method objects, ex_data_index etc are recreated each time pkcs11h_openssl_inititialize is called. Is this necessary or would it be okay to just return if already initialized?
• The session is locked before calling _signAny/_decryptAny only for RSA sign and decrypt. Is it not required for EC and DSA operations?

Also, I'm planning to split this into two PRs: (1) Add API to pass mechanism parameters (2) Implement PKEY methods for interfacing with OpenSSL in a new file selected via autoconf

(1) is enough for applications to use the library for PSS signing, (2) is nicety to have but harder to review, test and maintain.

[alonbl]

On Thu, May 6, 2021 at 7:40 PM Selva Nair @.***> wrote:

I'm mostly ready to push changes as discussed but not sure of a couple of things:

• Currenetly the method objects, ex_data_index etc are recreated each time pkcs11h_openssl_inititialize is called. Is this necessary or would it be okay to just return if already initialized?

We can probably define a new static variable with -1 and initialize it once, in the past I wished to avoid using magic numbers.

• The session is locked before calling _signAny/_decryptAny only for RSA sign and decrypt. Is it not required for EC and DSA operations?

Oh! you are correct, this is a huge mistake!

Also, I'm planning to split this into two PRs: (1) Add API to pass mechanism parameters

Great.

(2) Implement PKEY methods for interfacing with OpenSSL in a new file selected via autoconf

Great, if you need I can help you with the build tweaks.

(1) is enough for applications to use the library for PSS signing, (2) is nicety to have but harder to review, test and maintain.

I do not understand how will you implement (1) with openssl, this is interesting as if you can do this then we probably do not need (2)... :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/OpenSC/pkcs11-helper/pull/31#issuecomment-833674228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJURLPV63HDHIRLZ5R3JQDTMLA75ANCNFSM4373EIJQ .

[selvanair]

Thanks for the prompt reply :)

• The session is locked before calling _signAny/_decryptAny only for RSA sign and decrypt. Is it not required for EC and DSA operations? Oh! you are correct, this is a huge mistake!

Also there is a OPENSSL_NO_RSA check used in place of OPENSSLNO DSA.

Also, I'm planning to split this into two PRs: (1) Add API to pass mechanism parameters Great. (2) Implement PKEY methods for interfacing with OpenSSL in a new file selected via autoconf Great, if you need I can help you with the build tweaks. (1) is enough for applications to use the library for PSS signing, (2) is nicety to have but harder to review, test and maintain. I do not understand how will you implement (1) with openssl, this is interesting as if you can do this then we probably do not need (2)... :)

The application could add PKEY method callback to the app-method stack in OpenSSL using EVP_PKEY_meth_add0(). In the callback, construct mechanism params and then directly call pkcs11h_certificate_signAny_ex(). The certificate object can be saved in key ex_data or elsewhere.

It will be like implementing a part of pkcs11h_openssl.c within the application. Something like that is used for --cryptoapicert in OpenVPN to support newer mechanisms.

[alonbl]

On Thu, May 6, 2021 at 8:43 PM Selva Nair @.***> wrote:

Thanks for the prompt reply :)

• The session is locked before calling _signAny/_decryptAny only for RSA sign and decrypt. Is it not required for EC and DSA operations? Oh! you are correct, this is a huge mistake!

Also there is a OPENSSL_NO_RSA check used in place of OPENSSLNO DSA.

Oh, this is bad.

Also, I'm planning to split this into two PRs: (1) Add API to pass mechanism parameters Great. (2) Implement PKEY methods for interfacing with OpenSSL in a new file selected via autoconf Great, if you need I can help you with the build tweaks. (1) is enough for applications to use the library for PSS signing, (2) is nicety to have but harder to review, test and maintain. I do not understand how will you implement (1) with openssl, this is interesting as if you can do this then we probably do not need (2)... :)

The application could add PKEY method callback to the app-method stack in OpenSSL using EVP_PKEY_meth_add0(). In the callback, construct mechanism params and then directly call pkcs11h_certificate_signAny_ex(). The certificate object can be saved in key ex_data or elsewhere.

It will be like implementing a part of pkcs11h_openssl.c within the application. Something like that is used for --cryptoapicert in OpenVPN to support newer mechanisms.

This is not very nice for application developer, right?

[selvanair]

Another question: I want to support a fallback to RAW-RSA signing if PSS fails. This is required for legacy tokens continue to work. But when I do this, I'm asked to login a second time before the first call gives up. It seems the first failure leads to a login_retry. How can I avoid this?

[alonbl]

Another question: I want to support a fallback to RAW-RSA signing if PSS fails. This is required for legacy tokens continue to work. But when I do this, I'm asked to login a second time before the first call gives up. It seems the first failure leads to a login_retry. How can I avoid this?

you can probably check for capabilities/flags to check what is supported by the provider.

what is the error? why can't you loop within the sign method?

[selvanair]

The error depends on the token -- softhsm returns CKR_ARGUMENTS_BAD which is not really the correct err, a token I tested returns CKR_MECHANISM_INVALID or CKR_MECHANISM_PARAM_INVALID.

The best approach would be to test capabilities and decide how to call. But that would have been easier if we expose an API for that. In the absence of that we can check capabilities from doPrivateKeyOperation and return CKR_MECHANISM_INVALID without doing login-retry. Not ideal, but would avoid the second pin prompt.

Calling sign again from withing doPrivatekeyOperation does not work as we need to do pre processing (pss padding) or post processing (oaep decode) using openssl backend. That has to be done in _pkcs11h_pkey_sign(), not in ....certificate.c.

[selvanair]

In case the above is confusing: When C_SignInit() or C_Sign() fails we are almost always triggering login-retry from doPrivateKeyOperation. I think this should be done only in case of some errors -- like token was removed.

[mattcaswell]

If so, why did you leave the RSA methods and not replaced them in openssl 1.1.1? Is there any advantage in hooking the two?

I don't know :) The main reason for leaving them in is that I call them from pkey sign and decrypt methods for cases like RSA_PKCS1_PADDING or RSA_NO_PADDING to save some code duplication. In case of RSA_PKCS1 I actually call back OpenSSL's RSA_sign() which will use the RSA_methods hook to call us back later.

Applications that call the old legacy RSA_* functions directly will not go via EVP_PKEY_METHOD, so retaining the old RSA_METHOD for those applications is beneficial.

[selvanair]

This was superseded by #34. Closing.