search

parallaxsecond / parsec

Platform AbstRaction for SECurity service
https://parsec.community/
Apache License 2.0
471 stars 68 forks source link

Add PKCS11 provider export-attributes switch #462

Closed ionut-arm closed 3 years ago

ionut-arm commented 3 years ago

Creating keys in PKCS11 backends involves constructing templates for the keys, which limit or enhance the operations you can perform with them. Two of these attributes control the exportability of private keys - CKA_SENSITIVE and CKA_EXTRACTABLE - and thus we set these attributes according to the key policy provided by the Parsec client (based on the usage flags).

A problem arises when certain tokens (e.g. the PKCS11 implementation on NXP Layerscape boards) choose to restrict the usage of these attributes, making them read-only to never allow key exporting. Because of how read-only attributes are treated, the token is expected to fail creation if such attributes are ever set during key generation. This deviation from the spec is allowed and there is no way to check whether this is the case for the underlying platform through some discovery mechanism. This does, however, mean that currently we always fail to create asymmetric keys on such platforms.

A solution could be to remove the attributes from the templates and do the checks in Parsec alone, but this means that platforms that default those attributes to non-exportable, but would allow changes and key exporting, would also get non-exportable keys regardless of what the client specifies in the policy.

Another solution is to have another flag in the Parsec service config which effectively prohibits the service from setting CKA_SENSITIVE and CKA_EXTRACTABLE for PKCS11 keys. This issue covers the addition of this flag, along with tests that ensure everything still works with the new flag enabled.

hug-dev commented 3 years ago

I am wondering if we should be more general than that and think about other attributes that might not be supported on some platform (for example the other ones that we set in the key_pair_usage_flags_to_pkcs11_attributes).

One idea would be to have an "attribute blocklist" config flag which would contain a list of attributes that the service would ignore. CKA_SENSITIVE and CKA_EXTRACTABLE could be two example in that list for Layerscape boards.

wiktor-k commented 3 years ago

A problem arises when certain tokens (e.g. the PKCS11 implementation on NXP Layerscape boards) choose to restrict the usage of these attributes, making them read-only to never allow key exporting.

This is very bad and I hope you don't really mean "never allow key exporting" in all cases. For example for ECDH the temporary generic secret needs to be made exportable for quite a few cryptographic schemes. (Unless, of course, one would basically do the entire symmetric part on the card too but this is more complex than necessary).

Because of how read-only attributes are treated, the token is expected to fail creation if such attributes are ever set during key generation.

Wow, this is restrictive. Does that mean that the creation fails for any set of values for these attributes?

A solution could be to remove the attributes from the templates and do the checks in Parsec alone, but this means that platforms that default those attributes to non-exportable, but would allow changes and key exporting, would also get non-exportable keys regardless of what the client specifies in the policy.

Or try to create with correct values and if it fails fallback to creating with no values and log a warning :)

For me the explicit flag of not including these attributes in the template that the user needs to opt-in would be the best design but if the key creation fails mention this possible cause and the flag name in the error message because I guess people would have a hard time debugging it.

hug-dev commented 3 years ago

Thanks a lot for your comments!

Does that mean that the creation fails for any set of values for these attributes?

Yes, that was the case, even if those attributes were set to their default in the templates, that wouldn't work.

Or try to create with correct values and if it fails fallback to creating with no values and log a warning :)

True, we thought about this option, would be a bit cumbersome though... We thought about adding a config flag "forbid_exporting_keys" (or similar) which would make the key generation fail if keys are created with the possibility of being exported.

wiktor-k commented 3 years ago

True, we thought about this option, would be a bit cumbersome though... We thought about adding a config flag "forbid_exporting_keys" (or similar) which would make the key generation fail if keys are created with the possibility of being exported.

Well... if you created the key without these attributes and then read the actual values then it may still be that the key is not exportable thus forbid_exporting_keys=false would give a bad impression. (My only problem is with the naming, not with the idea)

I did some testing with my tokens and wrote a simple key generation CLI for tests setting some attributes from memory. AET Europe card was happy to accept that and it generated RSA key on the card with extractable=false and sensitive=true. Then I thought out testing it on Yubikey with their PKCS#11 driver (ykcs11) would be trivial... and here I am after several hours of getting it to work :)

First of all AET is happy to accept user pin for key generation, Yubikey requires SO PIN. But that's fine. Then Yubikey's driver performs some kind of attribute validation and it seems it's also not OK with extractable being set explicitly. If you check out the source code sensitive is being ignored and extractable falls into default-reject branch. I did verify that with my YubiKey 5C (firmware 5.2.7) that's quite recent. Apparently this issue with Yubikey's driver has been reported years ago.

I did verify OpenSC's PKCS#11 driver (that sees the Yubikey) but it seems key generation is not implemented (as if fails with this exact error).

Hope this helps folks, sorry that this is not exactly happy news! :wave:

hug-dev commented 3 years ago

Wow, thank you so much for testing this, it's really useful! Some attributes not being supported on some PKCS11 implementations is really annoying... For mechanisms it's fine because there is a function to request which ones are supported but not for attributes 😢

I checked the source code of the implementation in question here and SENSITIVE and EXTRACTABLE are actually allowed, but they can't be set to respectively false and true (the "unsecure" more I guess). Otherwise CKR_ATTRIBUTE_VALUE_INVALID is returned. In Yubikey's case, CKR_ATTRIBUTE_TYPE_INVALID is returned but reading the comment you link it seems more of a bug than a feature.

Also, if generate_key_pair or create_object fail with one of those error, there is no way to tell which one of the attributes in the template fail.

My point being that it would be fine to add a configuration flag, with a better name than the one proposed, and if that flag is set an error would be returned if a key is created with export set to true.