Closed Bug 319619 Opened 19 years ago Closed 19 years ago

"large" ECC private keys cannot be exported through PKCS #11

Categories

(NSS :: Libraries, defect, P2)

3.9.7
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.1

People

(Reporter: rrelyea, Assigned: rrelyea)

References

Details

Attachments

(1 file)

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
Also fixes a bug in softoken so the wrap really works.
Assignee: wtchang → rrelyea
Status: NEW → ASSIGNED
Attachment #205343 - Flags: review?(wtchang)
Do we know if this patch also resolves bug 319498 I filed yesterday? These two bugs might be related. vipul
*** Bug 319498 has been marked as a duplicate of this bug. ***
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 >
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
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
(In reply to comment #6) > one and also bug 319292. If the pk12util tests I meant bug 319252.
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
Blocks: 320187
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
Attachment #205343 - Flags: superreview?(vipul.gupta)
Target Milestone: 3.12 → 3.11.1
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.
Attachment #205343 - Flags: review?(wtchang) → review+
(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 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.
Attachment #205343 - Flags: superreview?(vipul.gupta)
Attachment #205343 - Flags: superreview?(nelson)
Attachment #205343 - Flags: review?(julien.pierre.bugs)
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.
Attachment #205343 - Flags: superreview?(nelson) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #205343 - Flags: review?(julien.pierre.bugs)
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: