Reselection of DF after failure in sc_pkcs15_decipher function

[xhanulik]

Problem Description

When running p11test with the PIV test card and RSA2048 keys, I encountered a problem: the pss_oaep_test decryption test fails with ERROR: [ SKIP 02 ] Re-authentication failed. I've discovered that the sc_pkcs15_decipher() function is called twice from pkcs15_prkey_decrypt() in framework-pkcs15.c:

OpenSC/src/pkcs11/framework-pkcs15.c

Lines 4611 to 4619 in c153e2f

	rv = sc_pkcs15_decipher(fw_data->p15_card, prkey->prv_p15obj, flags,
	pEncryptedData, ulEncryptedDataLen, decrypted, sizeof(decrypted), pMechanism);
	/* skip for PKCS#1 v1.5 padding prevent side channel attack */
	if (!(flags & SC_ALGORITHM_RSA_PAD_PKCS1) &&
	rv < 0 && !sc_pkcs11_conf.lock_login && !prkey_has_path)
	if (reselect_app_df(fw_data->p15_card) == SC_SUCCESS)
	rv = sc_pkcs15_decipher(fw_data->p15_card, prkey->prv_p15obj, flags,
	pEncryptedData, ulEncryptedDataLen, decrypted, sizeof(decrypted), pMechanism);

In that failed test, the first sc_pkcs15_decipher() returns SC_ERROR_WRONG_PADDING, so it is called for a second time. Still, since the token requires authentication before doing the decipher operation, it returns SC_ERROR_SECURITY_STATUS_NOT_SATISFIED.

This error code is then converted into CKR_USER_NOT_LOGGED_IN, the login state is then reset, and p11test cannot continue since always_authenticate() in the next test case fails.

This problem also shows up by only decryption of data with invalid padding with pkcs11-tool as the error is

error: PKCS11 function C_Login failed: rv = CKR_OPERATION_NOT_INITIALIZED (0x91)

(probably) instead of

error: PKCS11 function C_DecryptUpdate failed: rv = CKR_ENCRYPTED_DATA_INVALID (0x40)

Proposed Resolution

Not sure; I think that the reselection and second call to sc_pkcs15_decipher() in pkcs15_prkey_decrypt()
shouldn't be done after every error code.

Steps to reproduce

Create some bogus data to be decrypted (so after decryption, there isn't valid padding):

openssl rand -out random.txt 256

Decrypt the data with a token requiring always authenticate:

pkcs11-tool --decrypt --pin xxxxxx --id xx -m RSA-PKCS-OAEP --mgf MGF1-SHA256 --hash-algorithm SHA-1 -i random.txt

Output:

Using slot 0 with a present token (0x0)
Using the decrypta algorithm RSA-PKCS-OAEP
OAEP parameters: hashAlg=SHA-1, mgf=MGF1-SHA256, source_type=0, source_ptr=(nil), source_len=0
error: PKCS11 function C_Login failed: rv = CKR_OPERATION_NOT_INITIALIZED (0x91)
Aborting.

Logs

Here is the log starting at the first call to sc_pkcs15_decrypt(); it fails due to invalid padding, and then the second attempt to sc_pkcs15_decrypt() fails with Security status not satisfied, which is the error propagated outside.

log.txt

[dengert]

With 0494e46 in pkcs15-framwork.c sc_pkcs15_decrypt is called twice if first fails and not using PKCS#1 v1.5.

So why limit the test to padding to PKCS#1 v1.5?

It looks like ca08e97 from 2011 was trying to: "Limit the number of cases when applicated re-selection of application DF to strict minimum." It calls sc_pkcs15_decrypt a second time if it failed for any reason trying to avoid having to call reselect_app_df. I don't know what cards are depending on this feature.

The PIV card does not depend on this, as it uses the sc_card_operations card_reader_lock_obtained callback to
select the PIV AID when the first lock is obtained after SCardBeginTransaction locks the reader. Nine other card drivers also use this callback.

So the real fix maybe removing most if not all of ca08e97 and making sure all card drivers use the card_reader_lock_obtained to reselect_app_df if needed. (A select AID is a form of select app df).

Part of 0494e46

@@ -4609,27 +4611,53 @@ pkcs15_prkey_decrypt(struct sc_pkcs11_session *session, void *obj,
 	rv = sc_pkcs15_decipher(fw_data->p15_card, prkey->prv_p15obj, flags,
 			pEncryptedData, ulEncryptedDataLen, decrypted, sizeof(decrypted), pMechanism);
 
-	if (rv < 0 && !sc_pkcs11_conf.lock_login && !prkey_has_path)
+	/* skip for PKCS#1 v1.5 padding prevent side channel attack */
+	if (!(flags & SC_ALGORITHM_RSA_PAD_PKCS1) &&
+			rv < 0 && !sc_pkcs11_conf.lock_login && !prkey_has_path)
 		if (reselect_app_df(fw_data->p15_card) == SC_SUCCESS)
 			rv = sc_pkcs15_decipher(fw_data->p15_card, prkey->prv_p15obj, flags,
 					pEncryptedData, ulEncryptedDataLen, decrypted, sizeof(decrypted), pMechanism);

[Jakuje]

The PIV card does not depend on this, as it uses the sc_card_operations card_reader_lock_obtained callback to
select the PIV AID when the first lock is obtained after SCardBeginTransaction locks the reader. Nine other card drivers also use this callback.

Correct. I think we were moving most of the card drivers to use the card_reader_lock_obtained, but it was much later than this commit was created so this might not be needed at all anymore

So the real fix maybe removing most if not all of ca08e97 and making sure all card drivers use the card_reader_lock_obtained to reselect_app_df if needed. (A select AID is a form of select app df).

Yes, that was also my thinking that we should be able to remove this part (not only because it clearly does not play well with the always authenticate keys in PIV).

Does anyone have the backup of the old issue tracker? The issue IDs in this old commit, I think, are from the opensc-project.org and not from github. Lets try our luck @viktorTarasov ?

[frankmorgner]

I agree that always reselecting the applet in case of an error destroys the correct error code. However, card_reader_lock_obtained is currently only used in the scope of the card driver and typically checks whether the card application associated with the card driver is selected. Native OS card drivers (e.g. cardos) are offering DF selection via AID and would all have to implement card_reader_lock_obtained including the state tracking in XXX_select_file on what AID is selected.

In the past, we have removed the use of struct sc_card_cache, because it was causing many issues when concurrently running applications were used. And thinking about this, I believe this could now propably be fixed by setting the cache to some unknown state in card_reader_lock_obtained, meaning that we cannot use a shortcut in path selection and we may have to recover the current DF in case of an error (not only in case of private key operation, but also in case of, for example, EF selection). However, I believe that tracking all this introduces a lot complexity that will be used very rarely.

In my opinion, our best option would be to remove the heuristic introduced with ca08e97, because a) we cannot fix this at the PKCS#11 level and b) use_key() uses sc_lock() to bind the private key selection together with the actual usage of the key so that no other application can interfere. I think sc_pkcs15_encrypt/decrypt_sym() are the only cryptographic operations that do not use use_key() in the end and we may have to review the locking here (it looks like they have a misplaced logging command about sc_lock without actually locking the card...). In short, yes remove it, but please in the next release.

[dengert]

Sounds like a plan. I would suggest that @xhanulik start with the decrypt, as reselect is causing problems with the "prevent side channel attack"

[xhanulik]

Sure, I will take look into it.