Closed
Bug 299197
Opened 16 years ago
Closed 16 years ago
Need CKA_EXTRACTABLE for PK11_GenerateKeyPair
Categories
(NSS :: Libraries, enhancement, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10.2
People
(Reporter: nkwan, Assigned: wtc)
Details
Attachments
(4 files, 3 obsolete files)
27.47 KB,
patch
|
rrelyea
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
15.64 KB,
patch
|
rrelyea
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
34.63 KB,
patch
|
Details | Diff | Splinter Review | |
1.89 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
We need to generate a private key that can be wrapped (i.e. exported). So we call PK11_GenerateKeyPair with sensitive parameter setting to "true", and then call PK_WrapPrivKey. This works on Chrysalis' Luna SA, but the wrapping function fails on nCipher's netHSM. I manually modify PK11_GenerateKeyPair to include CKA_EXTRACTABLE=TRUE, then both HSMs work.
Assignee | ||
Comment 1•16 years ago
|
||
Thomas, could you use this new function to set the CKA_EXTRACTABLE attribute of the private key to true after it has been created?
Assignee | ||
Comment 2•16 years ago
|
||
I found that my proposed new function PK11_SetAttributes won't help Thomas because PKCS #11 says the CKA_EXTRACTABLE attribute cannot be changed once set to CK_FALSE. It becomes a read only attribute. If it is not okay to always set CKA_EXTRACTABLE to true, I don't know how to fix this other than adding a new version of the PK11_GenerateKeyPair function that allows the caller to set the CKA_EXTRACTABLE attribute.
Severity: normal → enhancement
Version: 3.10.1 → 3.9.3
Reporter | ||
Comment 3•16 years ago
|
||
Tried this, here are the outputs: (a) nCipher's nethsm: -1218574016[8be4548]: C_SetAttributeValue -1218574016[8be4548]: hSession = 0x8cf -1218574016[8be4548]: hObject = 0x57c -1218574016[8be4548]: pTemplate = 0xbfffcc00 -1218574016[8be4548]: ulCount = 1 -1218574016[8be4548]: CKA_EXTRACTABLE = CK_TRUE [1] -1218574016[8be4548]: rv = 0xb5 (CKR_SESSION_READ_ONLY) ... -1218574016[8be4548]: C_WrapKey -1218574016[8be4548]: hSession = 0x8cf -1218574016[8be4548]: pMechanism = 0xbfffcb80 -1218574016[8be4548]: hWrappingKey = 0x57e -1218574016[8be4548]: hKey = 0x57c -1218574016[8be4548]: pWrappedKey = 0x8f760e8 -1218574016[8be4548]: pulWrappedKeyLen = 0xbfffcb9c -1218574016[8be4548]: mechanism = 0x136 -1218574016[8be4548]: *pulWrappedKeyLen = 0x800 -1218574016[8be4548]: rv = 0x6a (CKR_KEY_UNEXTRACTABLE) (b) SafeNet's Luna SA: -1218574016[9b7d548]: C_SetAttributeValue -1218574016[9b7d548]: hSession = 0x7 -1218574016[9b7d548]: hObject = 0x169 -1218574016[9b7d548]: pTemplate = 0xbfffc8d0 -1218574016[9b7d548]: ulCount = 1 -1218574016[9b7d548]: CKA_EXTRACTABLE = CK_TRUE [1] -1218574016[9b7d548]: rv = 0x0 ... -1218574016[9b7d548]: C_WrapKey -1218574016[9b7d548]: hSession = 0x7 -1218574016[9b7d548]: pMechanism = 0xbfffc850 -1218574016[9b7d548]: hWrappingKey = 0x164 -1218574016[9b7d548]: hKey = 0x169 -1218574016[9b7d548]: pWrappedKey = 0x9ef8090 -1218574016[9b7d548]: pulWrappedKeyLen = 0xbfffc86c -1218574016[9b7d548]: mechanism = 0x136 -1218574016[9b7d548]: *pulWrappedKeyLen = 0x800 -1218574016[9b7d548]: rv = 0x63 (CKR_KEY_TYPE_INCONSISTENT) (c) Software Token: -1218574016[84ca548]: C_SetAttributeValue -1218574016[84ca548]: hSession = 0x1000001 -1218574016[84ca548]: hObject = 0x9f411dfb -1218574016[84ca548]: pTemplate = 0xbfffc750 -1218574016[84ca548]: ulCount = 1 -1218574016[84ca548]: CKA_EXTRACTABLE = CK_TRUE [1] -1218574016[84ca548]: rv = 0xb5 (CKR_SESSION_READ_ONLY) ... -1218574016[84ca548]: C_WrapKey -1218574016[84ca548]: hSession = 0x1000001 -1218574016[84ca548]: pMechanism = 0xbfffc6d0 -1218574016[84ca548]: hWrappingKey = 0xb8b42576 -1218574016[84ca548]: hKey = 0x9f411dfb -1218574016[84ca548]: pWrappedKey = 0x884f408 -1218574016[84ca548]: pulWrappedKeyLen = 0xbfffc6ec -1218574016[84ca548]: mechanism = 0x136 -1218574016[84ca548]: *pulWrappedKeyLen = 0x280 -1218574016[84ca548]: rv = 0x0 Can we add a function that takes an additional parameter "PRBool extractable"? SECKEYPrivateKey * PK11_GenerateKeyPair(PK11SlotInfo *slot,CK_MECHANISM_TYPE type, void *param, SECKEYPublicKey **pubKey, PRBool token, PRBool sensitive, void *wincx)
Comment 4•16 years ago
|
||
Sigh, What we really want is to tell the PKCS #11 module "set extractable to true if you can, but don't fail the key gen if you can't". Since we can't have that, then we need a value that says 'explicitly set extractable to true' to satify Thomas's requirement. We should look at all the flags and figure out how we can allow the app to set them as necessary.
Assignee | ||
Comment 5•16 years ago
|
||
This is the quickest way to meet Thomas's requirement. It adds a new version of PK11_GenerateKeyPair that takes a new "extractable" parameter. That parameter is a Boolean with an additional "don't care" value. Suggestions for the new function's name are welcome. I defined a new enum type for key extractability. I welcome suggestions for a shorter name of this enum type. I could also abuse the PRBool type and just add a new value for it: #define PR_DONTCARE -1 We did something like this in SSL: the SSL_REQUIRE_CERTIFICATE option has the PRBool type but has four values (0-3). I will attach the proper fix next. The reason I prepared this expedient patch is that the proper fix is quite complicated due to the need to maintain backward compatibility with the current code.
Attachment #187737 -
Attachment is obsolete: true
Attachment #192880 -
Flags: superreview?(nelson)
Attachment #192880 -
Flags: review?(rrelyea)
Comment 6•16 years ago
|
||
Adding Nelson to the cc list . Wan-Teh, Hopefully you were joking about adding an enum value of -1 for PR_DONT_CARE . The PRBool type is used in so many APIs and products - and they don't expect a -1 value of PR_DONT_CARE to be meaningful ;) We don't want to define it and have bugs reported against us when this value is passed in . We could however technically use a different enum type just for this PK11 API without breaking binary compatibility, if the PR_TRUE and PR_FALSE values are defined in that new enum type and continue to be handled the same way .
Assignee | ||
Comment 7•16 years ago
|
||
This patch implements the proper fix. It is quite complicated. In this solution, the attributes for the keys to be generated are specified as a mask of bitflags. I also modified the recently added PK11_TokenKeyGenWithFlags function to take an attribute bitflags parameter. An alternative to using bitflags is to use a CK_ATTRIBUTE template. That'll be more general because bitflags can only specify Boolean attributes. Please do a quick review of this patch and the "expedient fix" patch and let me know which path I should go down. Then I'll polish the chosen patch and ask you to do a detailed review. Notes on this patch: 1. I renamed pk11_FlagsToAttributes to pk11_OpFlagsToAttributes, where OpFlags stands for "key operation flags".
Attachment #193010 -
Flags: superreview?(nelson)
Attachment #193010 -
Flags: review?(rrelyea)
Comment 8•16 years ago
|
||
Comment on attachment 193010 [details] [diff] [review] Proper fix: flexible way to specify the key attributes when generating keys I much prefer this "proper fix" approach. It touches more code but doesn't seem complicated, and I think we're less likely to regret it. I have two requests/suggestions; 1) Please give the new functions more descriptive names than merely a new Ex suffix. Ex tells me that the function was changed but doesn't give me any clues about what the change was, so it's more work to figure it out. Also, what does one do when making a second extension of a function? Use an ExEx suffix? (is PK11_GenerateKeyPairWithAttributes already taken?) 2) Please define a new typedef for the PK11_ATTR_XXX flags, and use that instead of PRUint32 where those flags are being stored or passed. typedef PRUint32 PK11_ATTRIBUTE_FLAGS; would work. This would be analagous to CK_ATTRIBUTE and CK_FLAGS, which are just ulongs. This has the advantage of makig the declarations of formal parmeters and definitions of variables self describing.
Assignee | ||
Comment 9•16 years ago
|
||
Here is a revised version of the proper fix. I assume this is the path we want to go down. 1. I studied all the attributes that apply to private keys and secret keys, and found that the attributes are the ones our users may want to specify. They all happen to be boolean. - CK_TOKEN - CK_PRIVATE - CK_MODIFIABLE - CK_SENSITIVE - CK_EXTRACTABLE Right now NSS sets CK_TOKEN, CK_PRIVATE, and CK_SENSITIVE, and allows NSS users to specify CK_TOKEN and CK_SENSITIVE. Although these attributes have boolean values, I found that I have to use two bitflags for each attribute because of the way these attributes are specified in PKCS #11: True: { CKA_FOO, &cktrue, sizeof(CK_BBOOL) } False: { CKA_FOO, &ckfalse, sizeof(CK_BBOOL) } Default: no CKA_FOO entry in the template But there are two exceptions. The default values for CKA_TOKEN and CKA_MODIFIABLE are fixed, so we only need one bitflag for each of them to specify the non-default values. These bitflags are OR'd together to form a bitmask, which can be passed to new NSS functions to specify the attributes for private keys and secret keys. I define a new type PK11AttrFlags for the bitmask of these attribute flags. 2. I name the new functions PK11_GenerateKeyPairWithFlags and pk11_loadPrivKeyWithFlags. Suggestions for better names are welcome. I also modified the recently added PK11_TokenKeyGenWithFlags function to take a PK11AttrFlags parameter. 3. I have to rename the existing "flags" parameters to "opFlags", for "key operation flags". 4. Bob suggested that I define the attribute flags in the name space of the CKF_XXX key operation flags. I didn't do that because I want to avoid potential conflicts with new CKF_XXX key operation flags in the future. The downside is that PK11_TokenKeyGenWithFlags now takes two "flags" parameters: opFlags and attrFlags. Note that PK11_GenerateKeyPairWithFlags doesn't take an opFlags parameter because we specify all the key operations the token supports for the type of keys. So, this is a difference between PK11_GenerateKeyPairWithFlags and PK11_TokenKeyGenWithFlags. I think this difference is fine. Alternatively, I can add an opFlags parameter to PK11_GenerateKeyPairWithFlags and require it to be 0 for now, which means the current semantics (specifying all the key operations the token supports for that type of key). 5. I reimplemented the old functions using the new WithFlags functions. If you look at the reimplemented old functions (pk11_loadPrivKey, PK11_GenerateKeyPair, and PK11_TokenKeyGen), you will see the inconsistency in how we set the CKA_PRIVATE attribute. It is controlled by the "token" parameter in pk11_loadPrivKey and PK11_TokenKeyGen, but controlled by the "sensitive" parameter in PK11_GenerateKeyPair. Very strange. I encourage you to study how these three functions set their attrFlags local variable based on their token and sensitive parameters, and explain the code to me :-) The reimplemented old functions are backward compatible with the exception of CKA_TOKEN. The old PK11_GenerateKeyPair function sets the CKA_TOKEN attribute to CK_FALSE. The new PK11_GenerateKeyPair function doesn't do that because the default of CKA_TOKEN is CK_FALSE. I do this to avoid having to define the PK11_ATTR_SESSION bitflag (for CKA_TOKEN=CK_FALSE, which means a session object). OK, that's. Sorry this is a long patch and I wrote a lot of comments. Please feel free to ask me if you have any questions or suggestions.
Attachment #193010 -
Attachment is obsolete: true
Attachment #193119 -
Flags: superreview?(nelson)
Attachment #193119 -
Flags: review?(rrelyea)
Assignee | ||
Updated•16 years ago
|
Attachment #193010 -
Flags: superreview?(nelson)
Attachment #193010 -
Flags: review?(rrelyea)
Comment 10•16 years ago
|
||
Comment on attachment 193119 [details] [diff] [review] Proper fix v2 comments: pk11_AttrFlagsToAttributes should be 'table driven' so new attributes can easily be added (a la pk11_OpFlagsToAttributes) There is a latent bug in pk11_loadPrivKey which is carried over in pk11_loadPrivKeyWithFlags. . The loading of the public key should only load as a token object if the private key was loaded as a token object. (not your bug, but near a change). The latent comment from TNH should be removed or answered. The answer is it is not redundant because the underlying call may target a token other than softoken. The default values for these flags are specified as token specific in the spec.
Attachment #193119 -
Flags: review?(rrelyea) → review+
Comment 11•16 years ago
|
||
Comment on attachment 193119 [details] [diff] [review] Proper fix v2 Review comments: one nit and a bigger question. Thanks for creating this: >+typedef PRUint32 PK11AttrFlags; And using it. I think it makes code much more readable. >+PRBool >+pk11_BadAttrFlags(PK11AttrFlags attrFlags) >+{ >+ PK11AttrFlags trueFlags = attrFlags & 0x55555555; >+ PK11AttrFlags falseFlags = attrFlags >> 1 & 0x55555555; Is that equivalent to (attrFlags >> 1) & 0x55555555 ? or to attrFlags >> (1 & 0x55555555) ? I don't have my copy of K&R handy to check. In any case, please put explicit parens in the code, just to eliminate this question in the mind of future code reviewers. >+ return (trueFlags & falseFlags) != 0 ? PR_TRUE : PR_FALSE; suggestion: I'd prefer return (PRBool)((trueFlags & falseFlags) != 0); >+ PK11_SETATTRS(attr, CKA_MODIFIABLE, ckFalse, sizeof *ckFalse); >+ PK11_SETATTRS(attr, CKA_EXTRACTABLE, ckTrue, sizeof *ckTrue); >@@ -537,15 +533,31 @@ > { CKA_EXPONENT_1, NULL, 0 }, > { CKA_EXPONENT_2, NULL, 0 }, > { CKA_COEFFICIENT, NULL, 0 }, >+ /* reserve space for the attributes that may be >+ * specified in attrFlags */ >+ { CKA_TOKEN, NULL, 0 }, >+ { CKA_PRIVATE, NULL, 0 }, >+ { CKA_MODIFIABLE, NULL, 0 }, >+ { CKA_SENSITIVE, NULL, 0 }, >+ { CKA_EXTRACTABLE, NULL, 0 }, >+#define NUM_RESERVED_ATTRS 5 /* number of reserved attributes above */ >@@ -678,6 +706,8 @@ > { CKA_UNWRAP, NULL, 0}, > { CKA_SIGN, NULL, 0}, > { CKA_DECRYPT, NULL, 0}, >+ { CKA_EXTRACTABLE, NULL, 0}, >+ { CKA_MODIFIABLE, NULL, 0}, > }; > CK_ATTRIBUTE rsaPubTemplate[] = { > { CKA_MODULUS_BITS, NULL, 0}, It appears that this patch will cause NSS to now always use certain new attributes that (IINM) were newly created in PKCS11 v2.20, and therefore are probably not recognized by older PKCS11 modules. This makes me wonder: Are customers who have older crypto accelerators going to find that their accelerators don't work with new products that use NSS 3.10.2 (or 3.11)? That would be bad. It would be perceived as a P1 release stopper regression. So, I wonder if we need code that detects failure due to unrecognized attributes and tries again without them for backwards compatbilitiy. I'd give this patch sr+, except for that concern.
Assignee | ||
Comment 12•16 years ago
|
||
Nelson:
When NSS initializes a PKCS #11 attribute template
like this:
>@@ -678,6 +706,8 @@
> { CKA_UNWRAP, NULL, 0},
> { CKA_SIGN, NULL, 0},
> { CKA_DECRYPT, NULL, 0},
>+ { CKA_EXTRACTABLE, NULL, 0},
>+ { CKA_MODIFIABLE, NULL, 0},
> };
> CK_ATTRIBUTE rsaPubTemplate[] = {
> { CKA_MODULUS_BITS, NULL, 0},
in *some* places the attribute types are just place holders
to ensure that we declare a template big enough to hold all
the attributes we may set. In those cases, the code will
replace the attribute types when it sets the attributes.
The templates you cited are both such cases.
Note that for each PKCS #11 attribute type, I declare
two PKCS #11 flags: one sets the attribute to true,
and the other sets the attribute to false. If neither
flag is specified, the attribute is not set. One reason
I did this is to maintain backward compatibility -- we
need to ability to leave an attribute unspecified.
(The only exception is CKA_TOKEN, for which I only define
one flag PK11_ATTR_TOKEN. The reason is that this attribute's
default is false, which means a session object. I believe
all tokens honor this default required by the standard.
If you prefer, I can add a PK11_ATTR_SESSION flag to pair
with PK11_ATTR_TOKEN.)
Comment 13•16 years ago
|
||
Comment on attachment 192880 [details] [diff] [review] Expedient fix: add new version of PK11_GenerateKeyPair with "extractable" parameter I think we've decided to do the "proper fix" instead
Attachment #192880 -
Flags: superreview?(nelson) → superreview-
Comment 14•16 years ago
|
||
Comment on attachment 193119 [details] [diff] [review] Proper fix v2 In your patch, you asked: >+ * PK11_ATTR_READONLY >+ * >+ * If this flag is set, the object is read-only. If this flag is >+ * not set, the object is *by default* modifiable. >+ * >+ * This flag specifies the value of the PKCS #11 CKA_MODIFIABLE >+ * attribute. >+ * >+ * XXX Should we name this flag PK11_ATTR_UNMODIFIABLE? >+ */ >+/* Reserved 0x00000010L */ >+#define PK11_ATTR_READONLY 0x00000020L I'd say yes, it should be nmaed UNMODIFIABLE. But sr=nelson, with or without that change
Attachment #193119 -
Flags: superreview?(nelson) → superreview+
Assignee | ||
Updated•16 years ago
|
Attachment #192880 -
Flags: review?(rrelyea)
Assignee | ||
Comment 15•16 years ago
|
||
This patch implements the changes suggested by Bob and Nelson in their code reviews. 1. secmodt.h I renamed PK11_ATTR_READONLY as PK11_ATTR_UNMODIFIABLE. I now define two bitflags for every PKCS #11 object attribute with no exceptions. So PK11_ATTR_SESSION is the opposite of PK11_ATTR_TOKEN, and PK11_ATTR_MODIFIABLE is the opposite of PK11_ATTR_UNMODIFIABLE. 2. pk11pub.h Updated the comment for PK11_GenerateKeyPairWithFlags to reflect the addition and renaming of bitflags. 3. pk11obj.c In pk11_OpFlagsToAttributes, back out a change I made before. Upon closer inspection, I found that the original code was safe (*pType cannot be NULL) because of the flags &= CKF_KEY_OPERATION_FLAGS; statement above. So the change was not necessary. In pk11_BadAttrFlags, I made the changes Nelson suggested. I made pk11_AttrFlagsToAttributes table-driven as Bob suggested. 4. pk11akey.c In pk11_loadPrivKeyWithFlags, I fixed the pre-existing bug (always loading the public key as a token object) that Bob pointed out. In pk11_loadPrivKey, we use the new PK11_ATTR_SESSION flag. In PK11_GenerateKeyPairWithFlags, I now call pk11_AttrFlagsToAttributes to set the CKA_TOKEN and CKA_MODIFIABLE attributes of the public key. In PK11_GenerateKeyPair, we use the new PK11_ATTR_SESSION flag. 5. pk11skey.c Remove the TNH comment as Bob suggested.
Attachment #194706 -
Flags: superreview?(nelson)
Attachment #194706 -
Flags: review?(rrelyea)
Comment 16•16 years ago
|
||
Comment on attachment 194706 [details] [diff] [review] Finishing touches r=relyea
Attachment #194706 -
Flags: review?(rrelyea) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #192880 -
Attachment is obsolete: true
Comment 17•16 years ago
|
||
Comment on attachment 194706 [details] [diff] [review] Finishing touches sr=nelson@bolyard. Very nice simplification.
Attachment #194706 -
Flags: superreview?(nelson) → superreview+
Assignee | ||
Comment 18•16 years ago
|
||
This patch ports the fix back to the NSS_3_10_BRANCH. In order to reduce risk, it does not reimplement the existing functions using the new functions. I got approval from Sun to add this new function in NSS 3.10.2, so I've checked in this patch on the NSS_3_10_BRANCH. It's not easy to review this patch by just looking at the patch itself. I've done a lot of diffs between the code I added to the NSS_3_10_BRANCH, the existing functions on the NSS_3_10_BRANCH, and the code on the tip (i.e., three-way diffs). So I took a lot of care to make sure that I adapted the patches for the NSS_3_10_BRANCH correctly. If you want to review this patch, I suggest that you just make sure that it doesn't change the existing functions (with the exception of the renaming of pk11_FlagsToAttributes).
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → 3.10.2
Assignee | ||
Comment 19•16 years ago
|
||
A simple patch to fix the comments for PK11_GenerateKeyPairWithFlags and PK11_GenerateKeyPair, and add a comment for PK11_TokenKeyGen. The comments for PK11_GenerateKeyPairWithFlags and PK11_GenerateKeyPair were apparently copied from the old comment for PK11_TokenKeyGen.
Attachment #195458 -
Flags: review?(rrelyea)
Comment 20•16 years ago
|
||
Comment on attachment 195458 [details] [diff] [review] Fix comments r=relyea
Attachment #195458 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 21•16 years ago
|
||
Comment on attachment 195458 [details] [diff] [review] Fix comments I checked in this comment patch on the NSS trunk (NSS 3.11).
You need to log in
before you can comment on or make changes to this bug.
Description
•