NOTICE: test-cac fails because the reference file was not updated, but this update will conflict with the changed reference file in PR #2475. The reference file will be modified accordingly after PR #2475 will be merged.

I just merged the #2475.

I am not sure this is a good idea to change the meaning of the salt length in the pkcs11 API. I think the original intention was for pkcs11-tool to handle these special values similarly as the openssl, but the pkcs11 API should not change and accept only "positive" values for the salt length to adhere to the pkcs11 specification. But its already some time since I was looking into this code.

Also the passing of the pkcs11 mechanisms information down the libopensc is something that I do not like much. Please, see how I did the salt length handling in the p11test:

https://github.com/OpenSC/OpenSC/blob/master/src/tests/p11test/p11test_case_pss_oaep.c#L449-L473

I am not sure this is a good idea to change the meaning of the salt length in the pkcs11 API.

sLen is paramer for PSS .. from: https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__12__1__9__PKCS____1__RSA__PSS__MECHANISM__PARAMETERS.html

"sLen length, in bytes, of the salt value used in the PSS encoding; typical values are the length of the message hash and zero"

Actual OpenSC code does not allow us to use arbitrary sLen ... sLen is always same as hash length.

In this PR sLen is always positive value, but there is one special value - maximal value for CK_ULONG - an this value is handled in special way only in PSS verify. In this case autorecovery of sLen from signature is requested from openssl. I don't think there will be a situation where someone needs to enter such a large value for sLen (for verify operation). pkcs11-tool accepts negative values for salt_lenght, but negative value is never passed to pkcs#11 module.

As discussed in #2474 (comment) - there is no way to pass the salt_len to sc_pkcs15_compute_signature ..

I am also not excited about transferring pMechanism to libopensc code. But let's try to build on
#2474 (comment) - in the future we will need to transfer a lot of different parameters to the low level functions. If transfering (void *) pointer to pMechanism is not acceptable, we can find another way to convert pMechanism to some structure accessible from all libopensc functions (decrypt / encrypt / wrap / sign ..etc.).

I don't see a problem in using a standard PKCS11 structure in libopensc. It is a way to pass the information to a lower level.
One could define a SC_PSS_PARMS to hold the same information and have framework-pkcs15.c create it.
Either way would allow pkcs15 tools (and maybe minidriver) to pass in pss_params with having to use PKCS11.

The use of the pkcs11.h is already used in a number of routines.

#include "../pkcs11/pkcs11.h"
./pkcs15-coolkey.c
#include "../pkcs11/pkcs11.h"
./card-coolkey.c
#include "pkcs11/pkcs11.h"
./pkcs15-sec.c
#include "pkcs11/pkcs11.h"
./pkcs15-skey.c

I am not sure this is a good idea to change the meaning of the salt length in the pkcs11 API.

sLen is paramer for PSS .. from: https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__12__1__9__PKCS____1__RSA__PSS__MECHANISM__PARAMETERS.html

"sLen length, in bytes, of the salt value used in the PSS encoding; typical values are the length of the message hash and zero"

Actual OpenSC code does not allow us to use arbitrary sLen ... sLen is always same as hash length.

Oh. I forgot this limitation.

In this PR sLen is always positive value, but there is one special value - maximal value for CK_ULONG - an this value is handled in special way only in PSS verify. In this case autorecovery of sLen from signature is requested from openssl. I don't think there will be a situation where someone needs to enter such a large value for sLen (for verify operation). pkcs11-tool accepts negative values for salt_lenght, but negative value is never passed to pkcs#11 module.

Thanks for clarification. If this is only the ULONG_MAX value, it should be quite ok.

It would be probably easier to catch it if the PR would have some description from the beginning though.

As discussed in #2474 (comment) - there is no way to pass the salt_len to sc_pkcs15_compute_signature ..

I am also not excited about transferring pMechanism to libopensc code. But let's try to build on #2474 (comment) - in the future we will need to transfer a lot of different parameters to the low level functions. If transfering (void *) pointer to pMechanism is not acceptable, we can find another way to convert pMechanism to some structure accessible from all libopensc functions (decrypt / encrypt / wrap / sign ..etc.).

Thanks for clarification. Makes sense. I was not aware of that discussion. But note, that using this will prevent any card driver control to signalize support for some parameters. Right now, the driver can decide if the card supports particular digest/padding on card.

I'm with @dengert here, and don't share @Jakuje concerns. I say again: let's merge.

But note, that using this will prevent any card driver control to signalize support for some parameters. Right now, the driver can decide if the card supports particular digest/padding on card.

I agree, it would be appropriate for the card driver to be able to signal arbitrary sLen value support for PSS.

PSS is (partially) supported in card-idprime.c, card-sc-hsm.c. If someone requires a PSS operation with a special sLen (positive value), these drivers need to get the sLen value (from framework-pkcs15init.c). It is then up to the card driver to support this special sLen value. Due to an unsupported sLen value, the operation may be rejected before the APDU (set security environment) is sent to the card.

It would be a good idea to add sLen to struct sc_security_env (as one of sc_sec_env_param) and set it from pMechanism in sc_pkcs15_compute_signature ().

src/libopensc/opensc.h:#define SC_SEC_ENV_PARAM_IV              1
src/libopensc/opensc.h:#define SC_SEC_ENV_PARAM_TARGET_FILE     2

I can add:

#define SC_SEC_ENV_PARAM_SALT_LENGTH 3

And do something similar in sc_pkcs15_compute_signature():

senv_param = (sc_sec_env_param_t) { SC_SEC_ENV_PARAM_SALT_LENGTH, (void*) NULL, sLen };

Can you rebase on top of current master? There is a new conflict.

Rebased, fixed build with libressl.

FYI, this broke the OpenSCToken build on master, because it is using the sc_pkcs15_compute_signature(), which changed signature.

https://github.com/OpenSC/OpenSC/runs/5019743639?check_suite_focus=true

We can either change the OpenSCToken to use the new signature (preferred -- unless it needs to work with older OpenSC versions) or provide some compatibility API (we are not making any commitments about the libopensc API/ABI stability). What do you think @frankmorgner ?

fixed.

fixed.

Thanks! I was not sure if using just NULL would work in there. Seems like the last CI run is green so it solved the issue.

NULL was used as default in minidriver, so I hope we're fine here as well. the token doesn't have support for PSS, so the parameter is currently of no use. We'll see how to integrate CK_MECHANISM and CK_RSA_PKCS_PSS_PARAMS (via pMechanism) once we start using this outside PKCS#11... 😉