497223 - NSS failures with ECC hardware tokens

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

In trying to get NSS to work we ECC hardware we ran into several issues.

1) NSS was using the wrong encoding for CKA_EC_POINT (fixed in bug 480280)
2) Some tokens can only create single purpose keys (derive or signing). Most of this issue was fixed in XXXXX. The remaining fix for this is up to applications to use the new generate key call. Besides fixes to applications, NSS uses a generate key call when doing SSL ECDHE. That call needs to generate an ECC Derive key.
3) The PKCS #11 spec does not require tokens to 'know' how long the derived key is, so NSS needs to specify it (softoken would figure it out automatically).
4) The fix for CKA_EC_POINT above, also 'fixed' the encoding for derive. The cryptoki mailing list has since defined the correct encoding as the previous way NSS was encoding the value (not DER encoded). This code tried unecoded value first than falls back to the encoded value for maximum compatibility.

Comment 1

[Robert Relyea]

Attachment: hardware ECC fixex.

1) Set the CKA_VALUE_LEN entry on the template for ECC (this is accomplished by setting key_length to a non-zero value if it's zero. That value should be the 'natural' length of the key (either the length of the hash from the kdf function, or the curve length (which is calculated from the provided public key).
2) remove the environmental check for using unencoded keys, and use the unencoded key followed by the encoded key if the unencoded key fails.
3) if SECKEY_CreateECPrivateKey fails, try to generate a derive only key. This function is used by SSL go get it's temporary key exchange keys.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Bob, in comment 0 you wrote:
> Most of this issue was fixed in XXXXX. 

Which XXXXX was that?  Is this a reference to 
Bug 376417: PK11_GenerateKeyPair needs to get the key usage from the caller 
?

Comment 3

[Robert Relyea]

yes, I forgot to go look up the bug and replace the XXXX.

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 382401 [details] [diff] [review]
hardware ECC fixex.

Some preliminary review comments.

1. This CAN'T be right.

>-	    if (key_size == 0) templateCount--;
>+	    if (key_size == 0) {
>+		/* sigh, some tokens can't figure this out and require
>+		 * CKA_VALUE_LEN to be set */
>+		key_size = SHA1_LENGTH;
>+	    }

The size of 'Z', the output of the derive operation, must match the curve 
strength.  The three "Suite B" key sizes are all greater than 160 bits. 
I think the code you put into pk11_PubDeriveECKeyWithKDF is closer to what
is needed here.

2. According to 
http://mxr.mozilla.org/security/ident?i=SECKEY_CreateECPrivateKey&tree=security
all the calls to SECKEY_CreateECPrivateKey come from lib/ssl.  lib/ssl always
wants a derive-only key (it's only used for ecdhe).  This function gets called
at least once for every connection that does a full handshake with ECDHE, so 
we want it to be efficient.  having to try (and FAIL) twice before getting the
right answer is terribly inefficient.  So, I recommend that you just change
SECKEY_CreateECPrivateKey to always try to create a derive-only key.  

We should discuss this by phone Thursday.

Comment 5

[Robert Relyea]

re: point 1

No, it's correct. The key_size when using the KDF is the output of the KDF function, which is the length of the SHA1 Hash. Look at softoken to confirm that this is what we generate today for this case. SSL's needs is irrelevant because SSL calls the WithKDF variant which allows it to choose the appropriate kdf function, which is why there is a switch statement to choose the keysize, one of which is SHA1_LENGTH.

point 2.

mxr only gives our NSS internal usage, not application usage. SECKEY_CreateECPrivateKey is a public function (actually all functions libssl uses in NSS are, by definition, public -- at least mechanically). I coded it so a case that used to not work at all would now work (albeit not as fast as it could theoretically go). I can probably be convinced to blow off other applications that may have used the function to grab a signing key, but without any data about if there are such applications floating around (New apps seem to gravitate to the SECKEY_CreateXXXPrivateKey even though I personally discourage such usages as locking apps into a particular key type).

A better method might be to create a new function which passes the usage down (what a novel idea;) and have ssl call that new function. None of this touches FIPs code, so it doesn't have to be resolved on our FIPS timetable.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Re: point 1.  You're right.  It is correct.  Sorry.
Re: point 2.  SECKEY_CreateECPrivateKey creates a temporary "session" key.
I can't think of any reason why one would ever create a temporary signing key.  
But I wouldn't object to creating a new function such as SECKEY_CreateECPrivateKeyWithOpFlags, and changing libSSL to use that instead.
Would you rather do that?

Comment 7

[Robert Relyea]

Attachment: Always create a derive only key..

Comment 8

[Robert Relyea]

Comment on attachment 383031 [details] [diff] [review]
Always create a derive only key..

Here's a patch that addresses issue 2.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 383031 [details] [diff] [review]
Always create a derive only key..

r=nelson

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 382401 [details] [diff] [review]
hardware ECC fixex.

I gave r+ to the other patch for this bug.

Comment 11

[Robert Relyea]

Checking in lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.49; previous revision: 1.48
done
Checking in lib/pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.115; previous revision: 1.114
done

Comment 12

[Robert Relyea]

this should close.

Comment 13

[Robert Relyea]

Attachment: Don't set the LENGTH variable if a fixed length key type is specified.

This patch is on top of the old patch.

I'll back out my change to pk11skey.c so the tree will go green tonight. I'll add both back once this patch is reviewed. (and I've verified it doesn't create any more problems.

bob

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 384036 [details] [diff] [review]
Don't set the LENGTH variable if a fixed length key type is specified.

I'll bet Bob intended to ask me to review this patch.

Comment 15

[Robert Relyea]

>I'll bet Bob intended to ask me to review this patch.

You'd win that bet:).

bob

Comment 16

[Robert Relyea]

I've verified this passes all tests on Linux using full ECC.

bob

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 384036 [details] [diff] [review]
Don't set the LENGTH variable if a fixed length key type is specified.

I see two problems with this patch, both in pk11_PubDeriveECKeyWithKDF

>@@ -1759,23 +1775,27 @@ pk11_PubDeriveECKeyWithKDF(
> 
>     keyType = PK11_GetKeyType(target,keySize);
>     key_size = keySize;
>-    symKey->size = keySize;
>     if (key_size == 0) {
>+	if (pk11_GetPredefinedKeyLength(keyType)) {
>+	    templateCount --;
>+	} else {
>+	    /* sigh, some tokens can't figure this out and require
>+	     * CKA_VALUE_LEN to be set */
>+	    switch (kdf) {
>+	    case CKD_NULL:
>+		key_size = (pubKey->u.ec.publicValue.len-1)/2;
>+		break;
>+	    case CKD_SHA1_KDF:
>+		key_size = SHA1_LENGTH;
>+		break;
>+	    default:
>+		PORT_Assert("Invalid CKD");

1. That statement is a no-op.  It never asserts because the expression
is always true.  I will suggest a couple alternatives below.

>+		PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
>+		return NULL;
>+	    }
> 	}
>     }
>+    symKey->size = keySize;

2. I'm pretty sure you want that to be 
      symKey->size = key_size; 
because that's what your patch to PK11_PubDerive does.  Your patch changes the
assignment to symKey->size in both functions, moving it down to after the 
call to pk11_GetPredefinedKeyLength.  The patch also changes the assignment to 
assign the value from key_size in PK11_PubDerive, but not in this function.
I suspect that difference is unintentional.

Suggested alternative assertions:
1)    PORT_Assert(!"Valid CKD");

2) #define INVALID_CKD 0
      PORT_Assert(INVALID_CKD);

Comment 18

[Robert Relyea]

Attachment: Address nelson's comments.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 384178 [details] [diff] [review]
Address nelson's comments.

OK, close enough.  r=nelson

>+		PORT_Assert(!"Invalid CKD");

It's not invalid?

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Bob, this bug has patches with r+.  Why haven't you committed them?

Comment 21

[Robert Relyea]

They have, pk11skey version 1.115 and 1.117.