Incorrect slot for new associated object

[bukka]

Describe the bug

First of all I'm not really sure if this is actually reproducible in the current master but there is a chance it might happen too. I'm adding this mainly to not forget that this issue is there as it should be resolved before default PIN selection in #536 is merged.

My client has been reporting issue with blocking PIN when they use multiple slots with tokens having different PINs. I should note that they run a version with few patches including the default PIN selection implemented in #536

After digging through the logs (extended with some extra custom info) and code I noticed following.

After the object (public key in this case) is imported, it is followed by MATCH which tries to find associated object which means calling find operation. The issue is in the session (specifically slot) selection before this operation is started. The problem is that when such object is imported, the slotid is set to CK_UNAVAILABLE_INFORMATION so when session is selected, it uses following logic:

pkcs11-provider/src/session.c

Lines 947 to 978 in c8abb34

	/* caller is cycling through slots, find the next viable one */
	for (slot = p11prov_fetch_slot(slots, &slot_idx); slot != NULL;
	slot = p11prov_fetch_slot(slots, &slot_idx)) {
	CK_SLOT_ID slot_id = p11prov_slot_get_slot_id(slot);
	if (id != CK_UNAVAILABLE_INFORMATION && id != slot_id) {
	/* seek to next slot to check */
	continue;
	} else {
	/* Found "next" slot.
	* Reset the id, so from now on we check every following slot
	* and return the first one that successfully passes checks.
	*/
	id = CK_UNAVAILABLE_INFORMATION;
	}
	ret = check_slot(provctx, slot, uri, mechtype, rw);
	if (ret != CKR_OK) {
	/* keep going */
	continue;
	}
	if (reqlogin && !check_skip_login(provctx, slot)) {
	ret = slot_login(slot, uri, pw_cb, pw_cbarg, reqlogin, NULL);
	if (ret != CKR_OK) {
	/* keep going */
	continue;
	}
	}
	id = slot_id;
	P11PROV_debug("Found a slot %lu", id);
	break;
	}

The slot selected here is then set to the object that is then added to the pool. The selected session in this flow does not require login so it doesn't fail here.

The actual PIN failure happens later when verify is used and the key is duplicated and subsequently token login is required. It will use the slot selected above but because there is not pin in object (no refresh URI), it will use the configured PIN for default slot. However if the default slot does not match the slot for the session (which can easily happen when the default slot is configured and maybe there is a chance it happens anyway), then it result in incorrect PIN failure.

Currently I came up with following change ( bukka@cf89911 ), which is not yet confirmed that it fixes the issue but the idea is to select the default slot before getting the session when looking for an associated object and use that slot for the object. But I'm wondering what's the actual point of the current session slot selection above. Why doesn't it just always use the default slot?

To Reproduce

This is not easy to reproduce as it requires some slot preconditions. So far I haven't managed to recreate but it might be possible to recreate it artificially which I will attempt to do later.

Expected behavior

New associated object should be assigned to the default slot

Operating environment (please complete the following information):

• Linux otherwise does not matter

Token and application used (please complete the following information):

• Device: SoftHSMv2 but any device supporting multiple slot can show this issue
• Application: Nginx

[Jakuje]

New associated object should be assigned to the default slot

AFAIK the associated objects should be really searched only in the same slot. But I am not sure if this holds for the NSS softokn, which has two slots, one for private keys and one for public ones ...

Discussed with Simo, that this is about imported objects -- that should probably not search the token at all or just search just the internal cache.

[simo5]

Indeed the search should only be in the objects pool, and not hit the token at all if a slot is not available, IMO.

[bukka]

Yeah this was actually wrong assumption from my side. The problem is not in getting the session slot. The slot is already known at that time because it is looking for public key of the private key (so the searched object is a private key which has got slot set in the URI in my case).

The problem is that the public key that is found for the private key is added to the object pool. It has got slotid equal to the private key slot id but the pin for that slot is not kept anywhere. At the later stage during veryfication (p11prov_ecdsa_digest_verify_init), the cache_key is called which does the session login and requires correct slot and pin. The main branch code passes just the slotid so it will fail because it doesn't have the right PIN (it will use PIN for the default slot).

The fact that the PIN is not passed to the login session caused me problems already for private keys so I create #535 which addressed it for the private keys. But in this case there is not URI because it's a public key which didn't get URI copied from the private key. I just put together bukka@2422938 which might address it but will need to test it.

[simo5]

Just bluntly copying the URI would not be ok, because the URI can be very specific and include things like type=private, we'd have to re-derive the URI and add the PIN.
But at this point I think we just need to expand our PIN caches to index PINs by slot.
There will still be cases when the only available PIN is the default PIN though, but at least we can cache failures per-slot and keep using the good PIN for its slot.

[bukka]

Just bluntly copying the URI would not be ok, because the URI can be very specific and include things like type=private

That's why I introduced p11prov_uri_set_slot_type that changes the type. So the linked commit copies the URI and changes the type to match the class that is used in the search template (in this case it should be public).

But at this point I think we just need to expand our PIN caches to index PINs by slot.
There will still be cases when the only available PIN is the default PIN though, but at least we can cache failures per-slot and keep using the good PIN for its slot.

That might be a better solution but objects currently store just slotid so it will either require lookup and holding slot reference in the object. The thing is that ID can be currently different only if specified in the URI so for private keys that information is already there. For public keys that are not associated, it is also not needed because they should get imported into the default slot. So the associated objects might be the only ones that need this info - that's why I thought that it might be easier to just copy URI as the URI should be the same as for the private key except the type.

[simo5]

There should only be two cases for imported objects:

1. The key is ephemeral and there is no associated key in the token
2. OpenSSL is re-importing a previously exported public key, this should be searchable w/o a PIN.

So in which case is this happening?
Why there is no slot in the key if it comes from the token?

[bukka]

The key is added to object store during MATCH which is called on the private key. It tries to get the associated key of the private key (this is already present in the hsm so it wasn't exported by provider) which is found and added to the the object pool. Later when the key is used (during verify in my case), it is found in the pool and duplicated. Then during the cache_key (which runs as part of sig init), the full login is required - it uses the object slot but no PIN so it fails. Does that make sense?

I should note that I also added this change bukka@dc8d1e7 to actually copy the uri during duplication. As I said this is till untested so there might be more things missing.

[simo5]

So in this case the key has a proper slotid, but it is just missing the PIN for that slot.

I think the correct fix here, is per-slot PIN caching. Although it is unclear to me why searching the public key requires a PIN, is this token requiring login for any search?

[simo5]

Although another option here is to to session login early on in signature initialization.
After all without a successful login no signature can be performed.

[simo5]

Can you please let me know what line in signature.c triggers this issue ?

[bukka]

Sure p11prov_sig_op_init calls p11prov_obj_ref (key is the public key in this case as it is for verify) in

pkcs11-provider/src/signature.c

Line 768 in c8abb34

sigctx->key = p11prov_obj_ref(key);

That calls cache_key in

pkcs11-provider/src/objects.c

Line 448 in c8abb34

cache_key(obj);

which takes the login session:

pkcs11-provider/src/objects.c

Line 391 in c8abb34

ret = p11prov_take_login_session(obj->ctx, obj->slotid, &session);

[simo5]

Uhmm so this is trying to cache a public key ... it is ok if this operation fails, so why is that a problem ?

[simo5]

If we cache PINs (and failures) by slotid we should be able to handle this operation w/o issues.

[bukka]

Uhmm so this is trying to cache a public key ... it is ok if this operation fails, so why is that a problem ?

It sets error so it propagates as OpenSSL error to the application so it signals handshake failure for nginx in my case.

If we cache PINs (and failures) by slotid we should be able to handle this operation w/o issues.

Yeah this will work, it's just a bit more work to do :). Would you prefer to hold the slot reference in the object (instead of slotid) or doing lookup by id before each login?

[simo5]

Uhmm so this is trying to cache a public key ... it is ok if this operation fails, so why is that a problem ?

It sets error so it propagates as OpenSSL error to the application so it signals handshake failure for nginx in my case.

Then we just need to set guards around the operation so we can suppress that error stack, like we do in other places.

If we cache PINs (and failures) by slotid we should be able to handle this operation w/o issues.

Yeah this will work, it's just a bit more work to do :). Would you prefer to hold the slot reference in the object (instead of slotid) or doing lookup by id before each login?

I just realized we already store cached_pin and bad_pin per-slot in p11prov_slot, so I guess the only thing to do is to suppress the errors generated in cache_key as they should never be reported up the stack and cause TLS failures.