221067 - NSS needs to be able to create token symkeys from unwrap and derive.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Robert Relyea]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)

NSS currently has no way to create token keys when unwrapping or deriving keys
(only key import and generation).

NSS also has not public way to move a key across tokens.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Robert Relyea]

taking bug, ccing steve and wan-teh

Comment 2

[Robert Relyea]

Attachment: Add Move function, Add Perm options to Unwrap and Derive,

Add the ability to target a as a permanent key as a result of Unwrap & Derive.
Fix a problem where ImportKey as permanent wasn't working correctly.
Add a function to move a key from one token to another... even if the token is
a FIPS token.
Fix bug in softoken preventing secret keys from getting new labels (nicknames).

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132971 [details] [diff] [review]
Add Move function, Add Perm options to Unwrap and Derive,

Review comments:


>+++ lib/nss/nss.def	9 Oct 2003 23:30:31 -0000

I see that the NSS 3.9 section of nss.def on the trunk is not in ASCII 
sorting order.	Please correct that as part of these additions.

>Index: lib/pk11wrap/pk11func.h

>@@ -296,16 +296,45 @@
> 				PK11SymKey *symKey, SECItem *wrappedKey);
> SECStatus PK11_WrapSymKey(CK_MECHANISM_TYPE type, SECItem *params,
> 	 PK11SymKey *wrappingKey, PK11SymKey *symKey, SECItem *wrappedKey);
>+/* move a key to 'slot' optionally set the appropriate flags and making

Rather than "set the appropriate flags" (which doesn't tell the reader 
which flags are appropriate), this comment should explain that the operations
for which the new key is valid may be specified with operation, or with 
flags, or both.

>+ * the key permanent at the same time. If the key is moved to the same
>+ * slot, operation and flags are currently ignored */
>+PK11SymKey *PK11_MoveKey(PK11SlotInfo *slot, CK_ATTRIBUTE_TYPE operation, 
>+			CK_FLAGS flags, PRBool  perm, PK11SymKey *symKey);
>+/*
>+ * derive a new key from the base key.
>+ *  PK11_Derive returns a key which can doe exactly one operation, and is
					    ^
Notice the word "doe"

>+ *  PK11_Unwrap returns a key which can doe exactly one operation, and is
					    ^
doe again.

>+ *  PK11_PubUnwrap returns a key which can doe exactly one operation, and is
					       ^
doe again


>Index: lib/pk11wrap/pk11kea.c

>@@ -70,7 +70,7 @@
> 
> PK11SymKey *
> pk11_KeyExchange(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
>-		 	CK_ATTRIBUTE_TYPE operation, PK11SymKey *symKey)
>+ CK_ATTRIBUTE_TYPE operation, CK_FLAGS flags, PRBool isPerm, PK11SymKey *symKey)

please indent and fold that line.
Also, the prototype for this function does not appear in any header file.
Instead, the prototype is placed way down in pk11skey.c.  Please move that
prototype to the appropriate private header file.

>@@ -161,7 +161,7 @@
> 	SECItem Ra,wrap;
> 
> 	/* can only exchange skipjack keys */
>-	if (type != CKM_SKIPJACK_CBC64) {
>+	if ((type != CKM_SKIPJACK_CBC64) || (isPerm)) {
>     	    PORT_SetError( SEC_ERROR_NO_MODULE );
> 	    goto kea_failed;
> 	}

SEC_ERROR_NO_MODULE may be the correct error for type != SKIPJACK, but 
surely it is the wrong error for the isperm case.  No?


>Index: lib/pk11wrap/pk11skey.c

>@@ -125,13 +125,11 @@
> 	SECStatus rv = SECSuccess;
> 
> 	rwsession = session;

This line (above) is completely undone by the new code below.

>-	if (rwsession == CK_INVALID_SESSION) {
>-	    if (token) {
>-		rwsession =  PK11_GetRWSession(slot);
>-	    } else { 
>-		rwsession =  slot->session;
>-		PK11_EnterSlotMonitor(slot);
>-	    }
>+	if (token) {
>+	    rwsession =  PK11_GetRWSession(slot);
>+	} else if (rwsession == CK_INVALID_SESSION) {
>+	    rwsession =  slot->session;
>+	    PK11_EnterSlotMonitor(slot);
> 	}

This changes above have the effect that rwsession is never equal to session
at this point.	The session argument to this function is no longer used at
all.  Is that really what's intended?	If so, there should be a comment 
about this in the source, telling future readers that this argument is 
unused.


>@@ -487,15 +482,18 @@
>     PK11_SETATTRS(attrs, CKA_CLASS, &keyClass, sizeof(keyClass) ); attrs++;
>     PK11_SETATTRS(attrs, CKA_KEY_TYPE, &keyType, sizeof(keyType) ); attrs++;
>     if (isPerm) {
>-	PK11_SETATTRS(attrs, CKA_TOKEN, &cktrue, sizeof(keyType) ); attrs++;
>+	PK11_SETATTRS(attrs, CKA_TOKEN, &cktrue, sizeof(cktrue) ); attrs++;
>     }
>     templateCount = attrs - keyTemplate;
>     templateCount += pk11_FlagsToAttributes(flags, attrs, &cktrue);
>+    if (flags == 0) {
>+        PK11_SETATTRS(attrs, operation, &cktrue, 1); attrs++;
>+    }

The above 3 lines appear to attempt to correct the error that "operation"
was not being used.  But there are several problems here.

a) This change makes "operation" and "flags" mutually exclusive.  Instead,
it should create attribute templates for the union of the two.	Perhaps it 
should follow the example of the code at line 2610

b) The last argument to PK11_SETATTRS should be "sizeof cktrue", not "1".


>@@ -1319,16 +1317,19 @@
> 
> PK11SymKey *
> pk11_KeyExchange(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
>-		 	CK_ATTRIBUTE_TYPE operation, PK11SymKey *symKey);
>+		 	CK_ATTRIBUTE_TYPE operation, CK_FLAGS flags, 
>+					PRBool perm, PK11SymKey *symKey);

This declaration of a function (in another file) should be moved out of 
this source file and into a private header file.


>@@ -1369,6 +1381,23 @@
>     return newKey;
> }
> 
>+PK11SymKey *
>+PK11_MoveKey(PK11SlotInfo *slot, CK_ATTRIBUTE_TYPE operation, 
>+			CK_FLAGS flags, PRBool  perm, PK11SymKey *symKey)
>+{
>+    if (symKey->slot == slot) {
>+	if (perm) {
>+	   return PK11_ConvertSessionSymKeyToTokenSymKey(symKey,symKey->cx);
>+	} else {
>+	   return PK11_ReferenceSymKey(symKey);
>+	}

the sequence "return; else" (not else if) is generally misleading.  
Please remove the else, and the brackes around the second return.

Might a user make a second copy of a symkey in a token for performance?
Each symkey would have its own session.  
Two sessions can operate simultaneously.


>@@ -2565,7 +2614,8 @@
> static PK11SymKey *
> pk11_DeriveWithTemplate( PK11SymKey *baseKey, CK_MECHANISM_TYPE derive, 
> 	SECItem *param, CK_MECHANISM_TYPE target, CK_ATTRIBUTE_TYPE operation, 
>-	int keySize, CK_ATTRIBUTE *userAttr, unsigned int numAttrs)
>+	int keySize, CK_ATTRIBUTE *userAttr, unsigned int numAttrs,
>+							 PRBool isPerm)

>@@ -2648,10 +2699,19 @@
>     }
>     symKey->origin=PK11_OriginDerive;
> 
>-    pk11_EnterKeyMonitor(symKey);
>-    crv = PK11_GETTAB(slot)->C_DeriveKey(symKey->session, &mechanism,
>+    if (isPerm) {
>+	session =  PK11_GetRWSession(slot);
>+    } else {
>+        pk11_EnterKeyMonitor(symKey);
>+	session = symKey->session;
>+    }

The changes to pk11_DeriveWithTemplate get a RWSession when isPerm is true, 
but they do NOT set the CKA_TOKEN attribute in the template.  So, the 
derived key will be a session key.  This code needs to also set the 
CKA_TOKEN attribute, IFF it is not set in the template that is passed in.


>@@ -2967,7 +3027,7 @@
> pk11_AnyUnwrapKey(PK11SlotInfo *slot, CK_OBJECT_HANDLE wrappingKey,
>     CK_MECHANISM_TYPE wrapType, SECItem *param, SECItem *wrappedKey, 
>     CK_MECHANISM_TYPE target, CK_ATTRIBUTE_TYPE operation, int keySize, 
>-    void *wincx, CK_ATTRIBUTE *userAttr, unsigned int numAttrs)
>+    void *wincx, CK_ATTRIBUTE *userAttr, unsigned int numAttrs, PRBool isPerm)
> {
>     PK11SymKey *    symKey;
>     SECItem *       param_free	= NULL;
>@@ -2976,6 +3036,7 @@
>     CK_KEY_TYPE     keyType	= CKK_GENERIC_SECRET;
>     CK_ULONG        valueLen	= 0;
>     CK_MECHANISM    mechanism;
>+    CK_SESSION_HANDLE rwsession;
>     CK_RV           crv;
>     CK_MECHANISM_INFO mechanism_info;
>     CK_ATTRIBUTE    keyTemplate[MAX_TEMPL_ATTRS];

pk11_AnyUnwrapKey has the same problem as pk11_DeriveWithTemplate has.
It never adds a CKA_TOKEN attribute to the template.  
Sometimes it calls pk11_HandUnwrap, which understands the isPerm argument,
and other times it calls C_UnwrapKey, which does not understand the isPerm
argument.  To simplify the code, I would make this function add the 
CKA_TOKEN attribute in both paths, and then always pass isPerm=false to
pk11_HandUnwrap.  Also, you probably need to increase the number of 
allocate CK_ATTRIBUTEs above by 1.


>Index: lib/softoken/pkcs11u.c

>@@ -646,9 +646,9 @@
>     case CKA_VERIFY:
>     case CKA_WRAP:
>     case CKA_UNWRAP:
>+    case CKA_MODIFIABLE:
> 	return (PK11Attribute *) &pk11_StaticTrueAttr;
>     case CKA_NEVER_EXTRACTABLE:
>-    case CKA_MODIFIABLE:
> 	return (PK11Attribute *) &pk11_StaticFalseAttr;
>     case CKA_LABEL:
>         label = nsslowkey_FindKeyNicknameByPublicKey(object->obj.slot->keyDB,

This change makes all symKeys modifiable.  Is that what's intended?  
When do we ever need to change the value of a symkey after it is initially
imported/generated/derived?

Comment 4

[Robert Relyea]

Nelson,

Thanks for reviewing the patch: My comments on your comments;) are below. Those
things I didn't comment on I'll incorporate into a new patch. 


> SEC_ERROR_NO_MODULE may be the correct error for type != SKIPJACK, but 
> surely it is the wrong error for the isperm case.  No?

I'll double check the error code, but both cases are the same error. 

> This declaration of a function (in another file) should be moved out of 
> this source file and into a private header file.

It requires the creation of a private header file. I would prefer not doing that
in this patch. This function is pre-existing.

> > 	SECStatus rv = SECSuccess;
> > 
> > 	rwsession = session;
> 
> This line (above) is completely undone by the new code below.
> > +	if (token) {
> > +	    rwsession =  PK11_GetRWSession(slot);
> > +	} else if (rwsession == CK_INVALID_SESSION) {
> > +	    rwsession =  slot->session;
> > +	    PK11_EnterSlotMonitor(slot);
> >	}

Actually it's not. It's critical for the case where token is false and session
!= CK_INVALID_SESSION (still the most common case). This change fixes a critical
bug where we could not create token keys because the passed in session was not
read/write.

> The above 3 lines appear to attempt to correct the error that "operation"
> was not being used.  But there are several problems here.
>
> a) This change makes "operation" and "flags" mutually exclusive.  Instead,
> it should create attribute templates for the union of the two.
> Perhaps it should follow the example of the code at line 2610

This is intentional. I found making the union operation redundant and confusing
to actually use. In the normal case you had to cleverly set operation to match
one of the flags if you wanted to use flags.

> Might a user make a second copy of a symkey in a token for performance?
> Each symkey would have its own session.  
> Two sessions can operate simultaneously.

That's not the intent of this function. I would expect a function to explicitly
say it's going to create another copy of the key before it did (CloneKey or
CopyKey).

> This change makes all symKeys modifiable.  Is that what's intended?  
> When do we ever need to change the value of a symkey after it is initially
> imported/generated/derived?

The *value* field of keys is never modifiable. The modifiable flag controls
whether or not the label and key id are modifiable. A false setting of this flag
made it impossible to set the label on secret keys. Private keys already have
the modifiable flag set to true.

> pk11_AnyUnwrapKey has the same problem as pk11_DeriveWithTemplate has.
> It never adds a CKA_TOKEN attribute to the template. 

That's true... The caller always sets CKA_TOKEN. The isPerm argument is only
needed by these functions to know whether or not to use rwsessions or not.
 
> Sometimes it calls pk11_HandUnwrap, which understands the isPerm argument,
> and other times it calls C_UnwrapKey, which does not understand the isPerm
> argument.  To simplify the code, I would make this function add the 
> CKA_TOKEN attribute in both paths, and then always pass isPerm=false to
> pk11_HandUnwrap.  Also, you probably need to increase the number of 
> allocate CK_ATTRIBUTEs above by 1.

pk11_HandUnwrap likewise does not set CKA_TOKEN. It needs to know is perm for
the same reason given above.

In verifying this, I did find that PK11_UnwrapSymKeyWithFlagsPerm() was not
passing isPerm to pk11_AnyUnwrapKey(). This I'll fix in the new patch.

> This change makes all symKeys modifiable.  Is that what's intended?  
> When do we ever need to change the value of a symkey after it is initially
> imported/generated/derived?

The *value* field of keys is never modifiable. The modifiable flag controls
whether or not the label and key id are modifiable. A false setting of this flag
made it impossible to set the label on secret keys. Private keys already have
the modifiable flag set to true.

Comment 5

[Robert Relyea]

Attachment: Incorporate Nelson's comments that I didn't address in the comment secion

Comment 6

[Robert Relyea]

Comment on attachment 132971 [details] [diff] [review]
Add Move function, Add Perm options to Unwrap and Derive,

New patch supplied

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bob,  the NO_MODULE error is only appropriate in functions that search for
a module.  When the key attributes are wrong for the operation, there are 
error codes that say the key is not OK.

In adding the new isPerm flag to many functions, you made some of the functions
set the CKA_TOKEN attribute, and others do not.  It's inconsistent.   
These functions should all be consistent in this regard.  It's not reasonable
to have some of the functions not do the step because the one initial used of
these functions doesn't need it.  The functions should be designed to be 
general and consistent.  

The same argument applies to the mutual exclusivity of "operation" and "flags".
The existing functions perform the union.  The new functions should be
consistent.  It's not hard to do.  The correct thing is not to change the 
semantics of the old functions, but is to make the new functions consistent.

Comment 8

[Robert Relyea]

1. NO_MODULE error code: That is the default error code for this function. I'd
rather fix that as part of a separate bug where we can spend time examining the
actual implications.

2. setting 'isPerm': These functions take a complete template set by the caller.
They are pk11wrappper functions which even the rest of NSS do not call. They
have always required the callers to set the template up they way they are
supposed to be. I guess I could just remove the isPerm call from them and look
at the template to get their own idea of whether the object creation should
happen on a rw session or not. I created added the isPerm as an optimization for
looking up the value of CKA_TOKEN.

3. My initial thought would be to depricate the old functions. I'm loathed to
continue to propagate the confusing semantic. It doesn't seem useful to support
the union. It seems wrong to require a clever selection of operation with
matches one of the operations you set in the flag if you want to strictly use
the flags. Now that I think about it, though, a better solution might be to
provide for an invalid operation (CKA_NO_OPERATION or CKA_FLAGS_ONLY).
CKA_FLAGS_ONLY could be set to a CKA_ value you know to be invalid for operation
(CKA_CLASS for instance). That way we don't break compatibility. I'll make this
change.

bob

Comment 9

[Robert Relyea]

Attachment: Patch incorporating some bug fixes from CMS team & operation/flags semantics.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Adding self to CC list.  

When asking someone to review your patch, please ensure that person is
on the CC list.  Otherwise, that person will not see your comments.

Comment 11

[Robert Relyea]

Sorry, I actually thought you were on the CC list for this bug nelson.

bob

Comment 12

[Julien Pierre]

Comment on attachment 133442 [details] [diff] [review]
Incorporate Nelson's comments that I didn't address in the comment secion

Bob,

The patch seems syntactically correct, but I'm unable to review whether it is
functionally correct without some more explanations, probably in person.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

This bug appears to be awaiting review on several patches.
But IINM, changes were checked in that do not match the patches shown above.  
(Revs 1.74 and 1.76 of pk11skey.c,  see 
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fsecurity%2Fnss%2Flib%2Fpk11wrap&file=pk11skey.c&rev1=1.73&rev2=1.76&whitespace_mode=ignore&diff_mode=context

SO, I think the best thing to do is to remove the pending review requests from
the patches above, and mark this bug fixed.  Agreed?

Comment 14

[Wan-Teh Chang]

Bob, is this bug fixed?

Comment 15

[Robert Relyea]

yes