Last Comment Bug 319619 - "large" ECC private keys cannot be exported through PKCS #11
: "large" ECC private keys cannot be exported through PKCS #11
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.9.7
: All All
: P2 normal (vote)
: 3.11.1
Assigned To: Robert Relyea
: Jason Reid
:
Mentors:
: 319498 (view as bug list)
Depends on:
Blocks: 320187
  Show dependency treegraph
 
Reported: 2005-12-08 15:10 PST by Robert Relyea
Modified: 2006-04-22 22:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use the standard PKCS #11 semantics to get wrapped data size (6.40 KB, patch)
2005-12-08 15:16 PST, Robert Relyea
wtc: review+
nelson: superreview+
Details | Diff | Splinter Review

Description Robert Relyea 2005-12-08 15:10:48 PST
NSS cannot export ECC keys larger than about 200 bits through the pkcs #11 interface.

The problem is the pk11wrappers which try to export private keys calls a function which tries to guess the size of the bundle needed based on the size of the private key components. Unfortuately ECC does not supply any components that are proportional to the size of the key. The current function that returns the size returns the size of of the ECC parameters times a fudge factor (larger than the fudge factor used in RSA and DSA. Unfortunately the current ECC params are only an oid (no points), so the fudge factor is to small for 256 bit keys.

The possible solutions are:
1) increase the fudge factor to cover the larger ECC ciphers.
2) just return a fixed size large enough to cover the largest ECC key.
3) use the curve to determine the size of the ECC key and use an appropriate factor.
4) change the pk11wrappers to use the standard PKCS #11 semantics to determine the key size (calling with a NULL parameter in the output data and looking at the returned length).

I have a patch that implements #4
Comment 1 Robert Relyea 2005-12-08 15:16:31 PST
Created attachment 205343 [details] [diff] [review]
Use the standard PKCS #11 semantics to get wrapped data size

Also fixes a bug in softoken so the wrap really works.
Comment 2 Vipul Gupta 2005-12-08 15:27:01 PST
Do we know if this patch also resolves bug 319498 I
filed yesterday? These two bugs might be related.

vipul

Comment 3 Vipul Gupta 2005-12-08 16:01:16 PST
*** Bug 319498 has been marked as a duplicate of this bug. ***
Comment 4 Vipul Gupta 2005-12-08 16:05:34 PST
I've verified that this patch also resolves the issue
reported in bug 319498 and marked that bug
as a duplicate.

vipul

(In reply to comment #2)
> Do we know if this patch also resolves bug 319498 I
> filed yesterday? These two bugs might be related.
> 
> vipul
> 

Comment 5 Robert Relyea 2005-12-08 17:00:00 PST
Yes, I wouldn't have opened this bug if I has seen that one, but my search was for pkcs12 and ECC (not "Eliptic Curve").


bob
Comment 6 Vipul Gupta 2005-12-08 17:47:27 PST
I guess when I added the EC part of the code in 
pk11_private_key_encrypt_buffer_length(SECKEYPrivateKey *key),
I didn't fully understand how the private key is encrypted.

All of the NSS supported Elliptic Curves have atleast
a 5-byte OID and the biggest private key (for the 571-bit
curves) still fits within 72 bytes so I assumed that
a factor of 15 (15*5 = 75 > 72) would cover us.

Looks like there's a lot more to key wrapping than what I
assumed. 

Perhaps we ought to expand the ECC regression
tests to cover a larger set of curves (at the very least,
the smallest and the largest in each field) to catch bugs like this 
one and also bug 319292. If the pk12util tests
in nss/tests/ectools.sh had used larger curves (instead 
of 160-bit curves), we could have identified this bug much
earlier.

vipul
Comment 7 Vipul Gupta 2005-12-08 17:49:43 PST
(In reply to comment #6)
> one and also bug 319292. If the pk12util tests

  I meant bug 319252.
Comment 8 Robert Relyea 2005-12-09 10:59:16 PST
What is wrapped is a pkcs8 blob.

72 bytes is enough for 1 component, but current Template for the private key also includes the parameters (sizeof oid) and public key (72 *2 bytes)... plus there's the algid (which includes and oid and the parameters again!) There's also DER overhead.

The patch will remove this kind of error systemantically. (we move the knowledge about how big the blob is down into softoken).

bob
Comment 9 Robert Relyea 2005-12-16 16:23:18 PST
Comment on attachment 205343 [details] [diff] [review]
Use the standard PKCS #11 semantics to get wrapped data size

We should tarrget this for NSS 3.11
Comment 10 Wan-Teh Chang 2005-12-19 11:05:53 PST
Comment on attachment 205343 [details] [diff] [review]
Use the standard PKCS #11 semantics to get wrapped data size

r=wtc.  I have some questions about this patch.
Also, another NSS developer would be a better person
than Vipul to review this patch because none of your
changes is specific to ECC.

In pk11wrap/pk11akey.c, function
PK11_ExportEncryptedPrivKeyInfo:

>-    SECItem                        encryptedKey = {siBuffer, NULL, 0};

The original code uses PORT_ZAlloc to allocate
the encryptedKey.data buffer but doesn't free it.
So your patch fixes a memory leak.  It also avoids
an unnecessary SECITEM_CopyItem call.  Good.

>+    if(!epki->encryptedData.len) {
>+	rv = SECFailure;
> 	goto loser;
>     }

This test comes from the original code.  I don't understand
why it's necessary.  If it is necessary, then we should
also have the same test after the first C_WrapKey call
(which gets the buffer length).

In softoken/pkcs11c.c, we have:

>@@ -3937,6 +3937,17 @@
> 
> 	    crv = NSC_Encrypt(hSession, (CK_BYTE_PTR)pText.data, 
> 		              pText.len, pWrappedKey, pulWrappedKeyLen);
>+	    /* always force a finalize, both on errors and when
>+	     * we are just getting the size */
>+	    if (crv != CKR_OK || pWrappedKey == NULL) {
>+	    	CK_RV lcrv ;
>+		lcrv = sftk_GetContext(hSession,&context,
>+				       SFTK_ENCRYPT,PR_FALSE,NULL);
>+		sftk_SetContextByType(session, SFTK_ENCRYPT, NULL);
>+	    	if (lcrv == CKR_OK && context) {
>+		    sftk_FreeContext(context);
>+		}
>+    	    }

Since we already called sftk_GetContext earlier in this
case (CKO_SECRET_KEY) with the same arguments, can we avoid
the sftk_GetContext call here?

Also, based on my reading of the PKCS #11 documenentation
of C_Encrypt, I believe this if test and the if test you
added to the CKO_PRIVATE_KEY case should be written as:

    if ((crv == CKR_BUFFER_TOO_SMALL) ||
        (crv == CKR_OK && pWrappedKey == NULL)) {

> 	case CKO_PRIVATE_KEY:
> 	    {
> 		SECItem *bpki = sftk_PackagePrivateKey(key, &crv);
>+		SFTKSessionContext *context = NULL;

You can move the declaration of context into the if block
you added below.
Comment 11 Vipul Gupta 2005-12-19 11:13:48 PST
(In reply to comment #10)

> Also, another NSS developer would be a better person
> than Vipul to review this patch because none of your
> changes is specific to ECC.

   I agree.  Someone with greater familiarity with the
   NSS PKCS11 layer would do a better job than I.

   vipul
Comment 12 Wan-Teh Chang 2006-01-31 13:43:44 PST
Comment on attachment 205343 [details] [diff] [review]
Use the standard PKCS #11 semantics to get wrapped data size

Nelson, Julien, could one of you review this patch?
Thanks.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2006-01-31 17:28:44 PST
Comment on attachment 205343 [details] [diff] [review]
Use the standard PKCS #11 semantics to get wrapped data size

This patch makes the correctness of PK11_ExportEncryptedPrivKeyInfo 
depend on the correctness of the length value returned by NSC_WrapKey 
when it is invoked with a null output buffer pointer.   

Bug 320187 shows that NSC_WrapKey returns wrong (too small) values
when invoked with a NULL output buffer pointer AND the wrapping 
mechanism is one where the block size depends on the keys size,
such as is the case for RSA.

So, IFF PK11_ExportEncryptedPrivKeyInfo never uses RSA as a wrapping
mechanism, and never uses any other wrapping mechanism in which the
block size depends on the wrapping key size, then this patch is OK.
Otherwise, this bug depends on the fix for bug 320187.

I'm giving this patch r+, because it appears to be right, GIVEN 
that NSC_WrapKey always returns the proper output when called from
PK11_ExportEncryptedPrivKeyInfo.  But this bug cannot be marked
resolved/fixed untill either (a) this bug is shown to never use 
RSA as a wrapping mechanism, or (b) bug 320187 is fixed.
Comment 14 Robert Relyea 2006-02-01 09:03:14 PST
NSS 3.12 checkin

Checking in pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.11; previous revision: 1.10
done
Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.71; previous revision: 1.70
done

NSS 3.11 checkin

Checking in pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.9.2.1; previous revision: 1.9
done
Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.1; previous revision: 1.68
done



Note You need to log in before you can comment on or make changes to this bug.