PK11_GenerateKeyPair cannot automatically determine the key usage based on the key type. In some cases tokens only support a single key usage for a given key. In order to support those tokens, PK11_GenerateKeyPair needs to be able to determine that key usage. This problem is most accute for ECC keys, though it is, in theory, possible for this problem to manifest itself in RSA keys as well.
In addition to getting the key usage, PK11_GenerateKeyPair also examines the capabilities of the token to deside key usage. For RSA, it's a single mechanism (even though it can be used for Signing, Encryption, etc.). For ECC there are separate mechnisms for signing and encryption. This needs to be taken into account as well.
Created attachment 260525 [details] [diff] [review] Implement a version of PK11_GenerateKeyPair that allows us to pass key usage This patch fixes both issues identified above. It also fixes a bug where we are currently only setting the Derive key usage. This works with softoken today because softoken doesn't store the key usages and simply defaults them, so softoken always has both derive and sign for private ECC keys no matter how the key was generated. The most important part of this patch is the API for the new function. This proposed API allows the application to force some key usage values while defaulting on others. This is done by a mask, since PKCS #11 already specifies a Flag to attribute mapping (otherwise I would have used the same scheme we used for the attribute flags (sensitive, token, etc) where we had one bit that meant on and one bit that meant off. Another tact would be to add an actual keyusage parameter (instead of PKCS #11 flags) and do the mapping inside the function. In this case we would probably make the current function a private API (since we still need to 'default' the key usage) and call it from the new public API which takes a key usage. We could also include helper function which takes a key usage and kicks out PKCS #11 flags (and optionally a mask).
Comment on attachment 260525 [details] [diff] [review] Implement a version of PK11_GenerateKeyPair that allows us to pass key usage setting the patch flag
Comment on attachment 260525 [details] [diff] [review] Implement a version of PK11_GenerateKeyPair that allows us to pass key usage r=nelson. I have some suggested changes. >+ * opFlags give the actual setting while opFlagMask specifies if >+ * opFlags specifies the value, or wether we should take the >+ * default value. */ I read that sentence many times, and still don't understand what it said. After reading the code, I decided "default value" meant the value set by the PKCS#11 module. (also, whether is misspelled.) When I looked at the code, I decided that: opFlags specifies the bits that are to be set to 1. opFlagsMask specifies the bits that are to be reset to 0. When a bit is 1 in both opFlags and opFlagsMask, it is set to 1. I suggest you just say that. >+ /* if we are trying to turn on a flag, it better be in the mask */ >+ PORT_Assert ((opFlags & ~opFlagsMask) == 0); I see no reason for this. Why require this? How does it help? If the bit is set in opFlags, then it will be set in the result, whether it is set in the mask or not. >+ * EC keys are used in multiple different types of mechanism, if we >+ * are using dual use keys, we need to query the the second mechanism ^^^ ^^^ double the Paris in the the spring.
How about: * opFlagMask is set to one if the flag is specified in opFlags and zero if it * is to take on a default value calculated by PK11_GenerateKeyPair. * opFlags specifies the actual value of the flag 1 or 0. Bits not corresponding * to opFlagMask should be zero. >>+ /* if we are trying to turn on a flag, it better be in the mask */ >>+ PORT_Assert ((opFlags & ~opFlagsMask) == 0); > > I see no reason for this. > This saves a step later. The actual code without it would be: flags = (default_flags & ~opFlagMask) | (opFlags & opFlagMask); These are not set/reset flags as you surmised.
Checking in pk11akey.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v <-- pk11akey.c new revision: 1.17; previous revision: 1.16 done Checking in pk11pub.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v <-- pk11pub.h new revision: 1.16; previous revision: 1.15 done
code is checked in, marking fixed
Comment on attachment 260525 [details] [diff] [review] Implement a version of PK11_GenerateKeyPair that allows us to pass key usage The chief effect that this patch has had is NOT the purpose for which it was created. This patch was ostensibly created to allow the caller to explicitly specify the key usage flags for the pair to be generated, but NO CODE in NSS ever passes any value other than zero for those flags. So, the chief value of this patch was NOT that it allowed the caller to pass usage flags, because no NSS code does that. This patch also did another change that is NOT part of this bug's description and is not stated as an objective of this bug. It changes the way that the default key usage flags are computed/derived for EC keys. You can see that part of the patch at this URL: https://bugzilla.mozilla.org/attachment.cgi?id=260525&action=diff#mozilla/security/nss/lib/pk11wrap/pk11akey.c_sec5 That patch has had a good positive effect on the usability of NSS with third party ECC modules, but it didn't get checked in to NSS 3.11.x, because the ostensible purpose of this patch was to add a new API. So, we need to create a new patch for NSS 3.11.x that carries back into it the part of this patch that changes the default behavior for EC key pair usage flags. Without that patch, NSS 3.11.x is not able to generate useful ECC keys with some third party PKCS#11 modules. I will file a separate bug to do that.
So both issues were identified in the initial description. PK11_GenerateKeyPair is primarily used by applications. Before the new API, applications had no way of specifying the key usage. The idea is now PSM can specify the usage explicitly if the user has specified it in the keygen request. (though there needs to be a separate PSM bug for this). All other uses in NSS proper are simply wrappers for PK11_GenerateKeyPair Except for 1) pk11kea, which is used to move keys from one FIPS token to another). 2) SSL indirectly calls these functions (through the SECKEY_CreateXXXKey functions) to generate step down and ephemeral keys. Both of these generate ephermeral/session keys. We probably should change these 2 to the new PK11_GenerateKeyPairWithOpFlags() function, but most keys are generated externally from the application itself. bob