298957 - PK11_TokenKeyGen should add CKA_UNWRAP and CKA_WRAP attributes to object template

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Thomas Kwan]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

We are working on a project where we want to generate a symmetric key and
then use the key to unwrap another key into the hardware token. 
We would like to add the following to the PK11_TokenKeyGen function:

[nkwan@zion pk11wrap]$ cvs diff pk11skey.c
Index: pk11skey.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v
retrieving revision 1.83
diff -r1.83 pk11skey.c
1457c1457
<     CK_ATTRIBUTE genTemplate[6];
---
>     CK_ATTRIBUTE genTemplate[8];
1493a1494,1495
>     PK11_SETATTRS(attrs, CKA_WRAP, &cktrue, sizeof(cktrue));  attrs++;
>     PK11_SETATTRS(attrs, CKA_UNWRAP, &cktrue, sizeof(cktrue));  attrs++;
[nkwan@zion pk11wrap]$ vi pk11skey.c




Reproducible: Always

Comment 1

[Wan-Teh Chang]

Attachment: Thomas Kwan's patch

I am worried that adding the CKA_WRAP and CKA_UNWRAP
attributes to the C_GenerateKey call may break some
PKCS #11 modules.

Secret keys have other attributes, such as CKA_VERIFY.
Do we also need to set them?

Comment 2

[Thomas Kwan]

In our program, we need to use the generated symmetric to unwrap another key
into the hardware token. SafeNet's Luna SA does not perform unwrap if the key is
not marked with CKA_UNWRAP.

Maybe instead of keep adding new attributes to this function, we can write a new
function that takes a list of user-defined attributes, similar to
PK11_ImportSymKeyWithFlags where flags are user-definied attributes.

Comment 3

[Wan-Teh Chang]

Attachment: Add new function PK11_TokenKeyGenWithFlags (checked in)

This patch adds a new function PK11_TokenKeyGenWithFlags,
with has one more parameter (CF_FLAGS flags) than the
existing PK11_TokenKeyGen function.  PK11_TokenKeyGen
is reimplemented to call PK11_TokenKeyGenWithFlags.

Thomas, please try calling PK11_TokenKeyGenWithFlags with
"flags" equal to CKF_SIGN|CKF_WRAP|CKF_UNWRAP.

Comment 4

[Wan-Teh Chang]

Attachment: Alternate patch: have PK11_TokenKeyGen set CKA_WRAP and CKA_UNWRAP whenever possible

This patch doesn't require a new function and tries to
be backward compatible.  Thomas, could you test this
patch, too?

Comment 5

[Wan-Teh Chang]

Comment on attachment 187534 [details] [diff] [review]
Add new function PK11_TokenKeyGenWithFlags (checked in)

Thomas reported that the new function works for him.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187534 [details] [diff] [review]
Add new function PK11_TokenKeyGenWithFlags (checked in)

Looks right to me.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187537 [details] [diff] [review]
Alternate patch: have PK11_TokenKeyGen set CKA_WRAP and CKA_UNWRAP whenever possible

I gather that this second patch is intended to be an alternative to the 
first one.  It always adds the two attributes to the new keys.
I don't think we want to do that.  In particular, I think we do not
want to add wrap/unwrap to token keys that are intended for use in
signing-only certs.  So I think the approach in the first patch is right.

Comment 8

[Wan-Teh Chang]

Comment on attachment 187537 [details] [diff] [review]
Alternate patch: have PK11_TokenKeyGen set CKA_WRAP and CKA_UNWRAP whenever possible

Nelson, you are right.	This patch is an alternative to
the first patch.

We are generating symmetric keys, not public/private
key pairs in PK11_TokenKeyGen.

Comment 9

[Robert Relyea]

Comment on attachment 187534 [details] [diff] [review]
Add new function PK11_TokenKeyGenWithFlags (checked in)

This is the patch we want. The lack of a 'with flags' in for symkey keygen was
a dificiency in the original API. This fixes that deficiency.

Comment 10

[Robert Relyea]

Comment on attachment 187537 [details] [diff] [review]
Alternate patch: have PK11_TokenKeyGen set CKA_WRAP and CKA_UNWRAP whenever possible

The previous patch is preferable

Comment 11

[Wan-Teh Chang]

Comment on attachment 187534 [details] [diff] [review]
Add new function PK11_TokenKeyGenWithFlags (checked in)

Patch checked in on the NSS trunk for NSS 3.11.

The new function now has the NSS_3.11 version.	We
may need to change it to NSS_3.10.1 because we may
need to add this function to the NSS 3.10.1 release.

Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.147; previous revision: 1.146
done
Checking in pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v	<--  pk11pub.h
new revision: 1.6; previous revision: 1.5
done
Checking in pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.98; previous revision: 1.97
done

Comment 12

[Wan-Teh Chang]

Attachment: Code cleanup patch (checked in)

I found these problems while reviewing pk11skey.c to
ensure that MAX_TEMPL_ATTRS (16) is big enough.

1. pk11_FlagsToAttributes should not add an attribute
whose type is 0.  (Some of the elements in the attrTypes
array are 0.)  This lowers the maximum number of attributes
pk11_FlagsToAttributes may add from 12 to 9.

2. In PK11_DeriveWithFlagsPerm, the size of keyTemplate
array doesn't need to be MAX_TEMPL_ATTRS+1 because
MAX_TEMPL_ATTRS is big enough.

Comment 13

[Wan-Teh Chang]

Comment on attachment 187863 [details] [diff] [review]
Code cleanup patch (checked in)

I checked in the code cleanup patch on the NSS trunk for NSS 3.11.

Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.99; previous revision: 1.98
done
Checking in pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v	<--  pk11obj.c
new revision: 1.6; previous revision: 1.5
done

Comment 14

[Wan-Teh Chang]

Attachment: Move CKF_ENCRYPT and Fortezza hack code out of PK11_TokenKeyGenWithFlags (checked in)

In this patch, I propose to make the new function
PK11_TokenKeyGenWithFlags "legacy free" and move
the following code out of it to PK11_TokenKeyGen.

1. The code to set the CKF_ENCRYPT flag by default.
2. The Fortezza hack code.

What do you think?

Comment 15

[Robert Relyea]

Comment on attachment 188482 [details] [diff] [review]
Move CKF_ENCRYPT and Fortezza hack code out of PK11_TokenKeyGenWithFlags (checked in)

yes, I think it's a good idea to keep the fortezza weirdness out of the new
function.

Comment 16

[Wan-Teh Chang]

Comment on attachment 188482 [details] [diff] [review]
Move CKF_ENCRYPT and Fortezza hack code out of PK11_TokenKeyGenWithFlags (checked in)

I checked in this patch on the NSS trunk for NSS 3.11.

Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.100; previous revision: 1.99
done

Comment 17

[Wan-Teh Chang]

Attachment: Patch for NSS_3_10_BRANCH (checked in with changes)

This is the safer version of the patch, for NSS_3_10_BRANCH.

It merely adds the new PK11_TokenKeyGenWithFlag function without
touching any existing functions.

The new function has the version "NSS_3.10.1".

Comment 18

[Wan-Teh Chang]

Attachment: Export PK11_TokenKeyGenWithFlags in NSS 3.10.1 instead of 3.11 (checked in)

This patch is for the trunk.

Comment 19

[Robert Relyea]

Comment on attachment 190084 [details] [diff] [review]
Patch for NSS_3_10_BRANCH (checked in with changes)

I didn't give it a thorough review, assuming you cut and pasted the already
approved 3.11. All the parts seem to be there.

at that point it looks ok.

Comment 20

[Wan-Teh Chang]

Comment on attachment 190086 [details] [diff] [review]
Export PK11_TokenKeyGenWithFlags in NSS 3.10.1 instead of 3.11 (checked in)

Checked in on the trunk.

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.151; previous revision: 1.150
done

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 188482 [details] [diff] [review]
Move CKF_ENCRYPT and Fortezza hack code out of PK11_TokenKeyGenWithFlags (checked in)

Since this patch is already checked in, I will just comment on it and 
not give it r+ or r-.  

There is one aspect of this change that makes the behavior of PK11_TokenKeyGen 
after the patch not the same as before the patch.  It is this change:


>@@ -836,7 +827,7 @@
>     if (symKey == NULL) return NULL;
> 
>     symKey->size = keySize;
>-    symKey->origin = (!weird) ? PK11_OriginGenerated : PK11_OriginFortezzaHack;
>+    symKey->origin = PK11_OriginGenerated;
> 
>     /* Initialize the Key Gen Mechanism */
>     mechanism.mechanism = PK11_GetKeyGenWithSize(type, keySize);

With this patch in place, symKey->origin will never be PK11_OriginFortezzaHack
for fortezza any more.	If it were not for the fact that we're removing
fortezza
support in 3.11, I would have said this is a regression worthy of fixing (r-).  
Note that the approach taken on the 3.10 branch, which does not reimplement 
PK11_TokenKeyGen using PK11_TokenKeyGenWithFlags should not have this issue.

Perhaps it would be best to fix this by removing the fortezza support from 
PK11_TokenKeyGen alltogether.  I think we've agreed to remove fortezza 
support from NSS for 3.11.  I don't think we have to do that all at once.
I think we can begin to remove it where we find it, and continue to do that
until it's all gone.  

So, I invite you to remove the fortezza "hack" from PK11_TokenKeyGen on the
trunk.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190084 [details] [diff] [review]
Patch for NSS_3_10_BRANCH (checked in with changes)

The old function had this comment:

  * Use the token to Generate a key. keySize must be 'zero' for fixed key
  * length algorithms. NOTE: this means we can never generate a DES2 key
  * from this interface!

I always considered that to be flawed.	This API just doesn't work for
algs that allow variable size keys.  Since we're devising a new API here, 
and have an opportunity to make the necesssary changes to the function 
signature in the new function, shouldn't we take this opportunity 
to fix that limitation?  
If we don't fix it now, won't we have to add Yet Another API down the road?

Comment 23

[Robert Relyea]

nelson re comment 21

The patch removes support for fortezza hack in the
'PK11_TokenKeyGenWithFlags'(the new function), so that is not a regression. The
old API ('PK11_TokenKeyGen') now calls the 'PK11_TokenKeyGenWithFlags', then
calls 'PK11_SetFortezzaHack(symKey)' if 'weird' is set, so the semantics of the
old version of the function is maintained. I would have rejected the patch if
PK11_TokenKeyGen's semantics had changed.

re comment 22

Sigh, this is weirdness of the PKCS #11 interface. PKCS #11 modules barf if you
specify the CKA_KEY_LENGTH attribute for keys with fixed length. For DES, PKCS
#11 determines whether or not you want a 2des or a 3des key by whether or not
you specify CKM_DES2_KEY_GEN or CKM_DES3_KEY_GEN is supplied. The function is
using the value of keysize to determine if it should supply the key length.
Unfortunetly it's also using it to determin if you mean 2des or 3des when you
specify CKM_DES3_CBC as the 'target' mechanism.

It turns out the comment isn't exactly right, as you could specify
CKM_DES2_KEY_GEN as the 'target' mechanism directly. The solutions to this
problem include 1) specifying how could can generate DES2 keys, 2) move the
knowlege of key sizes down into the function itself so that it knows not to set
CKA_KEY_LENGTH for various mechanisms itself without the caller having  to tell it.

In either of these cases, we should develope some DES2 test cases. We don't test
DES2 in a lot of instances (including key import and actually using DES2 keys).

bob

Comment 24

[Wan-Teh Chang]

Attachment: Improve comment regarding generating DES2 keys (checked in)

Comment 25

[Robert Relyea]

Comment on attachment 190619 [details] [diff] [review]
Improve comment regarding generating DES2 keys (checked in)

r+

R+ still applies if the last sentence says 'currently doesn't work'.

bob

Comment 26

[Robert Relyea]

Comment on attachment 190084 [details] [diff] [review]
Patch for NSS_3_10_BRANCH (checked in with changes)

Make sure 3.11 has the export in a NSS_3.10.1 stanza (not in a NSS3.11 stanza).

If a 3.10.1 is shipped from the 3.10.1 branch rather than the 3.10 branch, then
be sure to update 3.10.1 to 3.10.2 both here and in the tip.

I wouldn't complain if the DES2 comments were fixed before checkin...;).

Comment 27

[Wan-Teh Chang]

Comment on attachment 190619 [details] [diff] [review]
Improve comment regarding generating DES2 keys (checked in)

I checked in the improved comment (with Bob's suggested addition
of "currently") on the NSS trunk.

Comment 28

[Wan-Teh Chang]

Comment on attachment 190084 [details] [diff] [review]
Patch for NSS_3_10_BRANCH (checked in with changes)

I checked in this patch on the NSS_3_10_BRANCH (for NSS 3.10.2)
with two changes:
- PK11_TokenKeyGenWithFlags is exported in NSS_3.10.2 instead
  of NSS_3.10.1.
- Improved comment for PK11_TokenKeyGenWithFlags and
  PK11_TokenKeyGen.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190084 [details] [diff] [review]
Patch for NSS_3_10_BRANCH (checked in with changes)

OK as checked in.  One nit:

>+        bestSlot = PK11_GetBestSlot(type,wincx); /* TNH: references the slot? */
>+        if (bestSlot == NULL) {
>+	    PORT_SetError( SEC_ERROR_NO_MODULE );
>+	    return NULL;
>+	}
>+
>+        symKey = pk11_CreateSymKey(bestSlot, type, !isToken, wincx);
>+
>+        PK11_FreeSlot(bestSlot);

This code snipped was originally written by Terry Hayes in rev 1.4 of 
this file.  I think his comment "references the slot?" was expressing
some doubt about the correctness of the subsequent PK11_FreeSlot call 
that he added.	This patch propagates that comment and its doubt.
We should resolve this question, and remove the comment or fix the 
code, in all places where this comment now appears.

Comment 30

[Robert Relyea]

remove the comment. PK11_GetBestSlot does return a reference to the slot.

Comment 31

[Wan-Teh Chang]

Attachment: Remove the comment "TNH: references the slot?" (checked in)

I verified that PK11_GetBestSlot references the slot, and
pk11_CreateSymKey gets its own reference for the slot, so
it is correct for the code to free the slot reference after
the pk11_CreateSymKey call.

With Bob's comment, I've checked in this patch on the NSS
trunk (3.11) and NSS_3_10_BRANCH (3.10.2).

There is another TNH comment in the file:

    /* TNH: Isn't this redundant, since "handleKey" will set defaults? */
    PK11_SETATTRS(attrs, (!weird)
	? CKA_ENCRYPT : CKA_DECRYPT, &cktrue, sizeof(CK_BBOOL)); attrs++;

Anyone know what he meant by "handleKey"?

Comment 32

[Wan-Teh Chang]

Bob answered my question about the TNH comment on "handleKey"
in bug 299197 comment 10.

The PK11_TokenKeyGenWithFlags function will appear in the NSS
3.10.2 release.  Its prototype has been changed to use the
new PK11AttrFlags type instead of the "PRBool isToken" parameter.
This allows us to specify other PKCS #11 object attributes
such as isSensitive and isPrivate.