299197 - Need CKA_EXTRACTABLE for PK11_GenerateKeyPair

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Thomas Kwan]

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.

Comment 1

[Wan-Teh Chang]

Attachment: Workaround: new function PK11_SetAttributes

Thomas, could you use this new function to set the
CKA_EXTRACTABLE attribute of the private key to true
after it has been created?

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Thomas Kwan]

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

[Robert Relyea]

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.

Comment 5

[Wan-Teh Chang]

Attachment: Expedient fix: add new version of PK11_GenerateKeyPair with "extractable" parameter

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.

Comment 6

[Julien Pierre]

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 .

Comment 7

[Wan-Teh Chang]

Attachment: Proper fix: flexible way to specify the key attributes when generating keys

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".

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Wan-Teh Chang]

Attachment: Proper fix v2

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.

Comment 10

[Robert Relyea]

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.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 12

[Wan-Teh Chang]

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

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 15

[Wan-Teh Chang]

Attachment: Finishing touches

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.

Comment 16

[Robert Relyea]

Comment on attachment 194706 [details] [diff] [review]
Finishing touches

r=relyea

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194706 [details] [diff] [review]
Finishing touches

sr=nelson@bolyard.  Very nice simplification.

Comment 18

[Wan-Teh Chang]

Attachment: Patch for NSS_3_10_BRANCH

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).

Comment 19

[Wan-Teh Chang]

Attachment: Fix comments

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.

Comment 20

[Robert Relyea]

Comment on attachment 195458 [details] [diff] [review]
Fix comments

r=relyea

Comment 21

[Wan-Teh Chang]

Comment on attachment 195458 [details] [diff] [review]
Fix comments

I checked in this comment patch on the NSS trunk (NSS 3.11).