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)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.1
People
(Reporter: rrelyea, Assigned: rrelyea)
References
Details
Attachments
(1 file)
6.40 KB,
patch
|
wtc
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•19 years ago
|
||
Also fixes a bug in softoken so the wrap really works.
Comment 2•19 years ago
|
||
Do we know if this patch also resolves bug 319498 I
filed yesterday? These two bugs might be related.
vipul
Comment 3•19 years ago
|
||
*** Bug 319498 has been marked as a duplicate of this bug. ***
Comment 4•19 years ago
|
||
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
>
Assignee | ||
Comment 5•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Assignee | ||
Comment 8•19 years ago
|
||
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
Assignee | ||
Comment 9•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Target Milestone: 3.12 → 3.11.1
Comment 10•19 years ago
|
||
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+
Comment 11•19 years ago
|
||
(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•19 years ago
|
||
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 13•19 years ago
|
||
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+
Assignee | ||
Comment 14•19 years ago
|
||
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
Updated•19 years ago
|
Attachment #205343 -
Flags: review?(julien.pierre.bugs)
Updated•19 years ago
|
Priority: -- → P2
You need to log in
before you can comment on or make changes to this bug.
Description
•