Closed
Bug 655850
Opened 14 years ago
Closed 14 years ago
lib crmf uses a hardcoded maximum size of 2048 for wrapped private keys (MAX_WRAPPED_KEY_LEN)
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.11
People
(Reporter: KaiE, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
|
1.65 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
The crmf library uses a hardcoded maximum size of 2048 for wrapped private keys (MAX_WRAPPED_KEY_LEN)
We ran into a failure when trying to use generateCRMFRequest to produce a 4096 byte RSA key with a 1024 bit escrow cert.
Required output array is 2368.
#0 DES_Encrypt (cx=0xa4bbdd30, out=
0xa4a0f800 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., outLen=0xbfffb59c, maxOutLen=2048, in=0xa85f5000 "0\202\tB\002\001", inLen=2368) at desblapi.c:280
#1 0x04bd542b in DES_Encrypt (cx=0xa4bbdd30, output=
0xa4a0f800 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., outputLen=0xbfffb59c, maxOutputLen=2048, input=0xa85f5000 "0\202\tB\002\001", inputLen=2368) at loader.c:490
#2 0x04bb5230 in NSC_EncryptUpdate (hSession=16777217, pPart=0xa85f5000 "0\202\tB\002\001", ulPartLen=2368, pEncryptedPart=
0xa4a0f800 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., pulEncryptedPartLen=0xbfffb704) at pkcs11c.c:926
#3 0x04bb55cb in NSC_Encrypt (hSession=16777217, pData=0xa85f5000 "0\202\tB\002\001", ulDataLen=2374, pEncryptedData=
0xa4a0f800 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., pulEncryptedDataLen=0xbfffb704) at pkcs11c.c:1019
#4 0x04bbd107 in NSC_WrapKey (hSession=16777217, pMechanism=0xbfffb6f8, hWrappingKey=3, hKey=4120494076, pWrappedKey=
0xa4a0f800 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., pulWrappedKeyLen=0xbfffb704) at pkcs11c.c:4449
#5 0x03e316a5 in PK11_WrapPrivKey (slot=0xa6504800, wrappingKey=0xa4a423d0, privKey=0xa4a73810, wrapType=310, param=0xa4a4e050, wrappedKey=0xbfffb784, wincx=0x0) at pk11obj.c:1116
#6 0x02a2bcf4 in crmf_create_encrypted_value_wrapped_privkey (inPrivKey=0xa4a73810, inCAKey=0xa4a74810, destValue=0xa4a17100) at crmfcont.c:823
#7 0x02a2bf95 in CRMF_CreateEncryptedKeyWithEncryptedValue (inPrivKey=0xa4a73810, inCACert=0xa4bb0810) at crmfcont.c:908
#8 0x01fd9494 in nsSetEscrowAuthority (certReq=0xa4a74010, keyInfo=0xa4a160a0, wrappingCert=0xa4be9bc0) at /plaindata/moz/20/mozilla/security/manager/ssl/src/nsCrypto.cpp:982
#9 0x01fd9d9a in nsCreateSingleCertReq (keyInfo=0xa4a160a0, reqDN=0xa4b4e5a0 "CN=Kai Engert", regToken=0xb7d252bc "", authenticator=0xb7d252c2 "", wrappingCert=0xa4be9bc0) at /plaindata/moz/20/mozilla/security/manager/ssl/src/nsCrypto.cpp:1367
#10 0x01fda707 in nsCreateReqFromKeyPairs (keyids=0xa4a160a0, numRequests=1, reqDN=0xa4b4e5a0 "CN=Kai Engert", regToken=0xb7d252bc "", authenticator=0xb7d252c2 "", wrappingCert=0xa4be9bc0) at /plaindata/moz/20/mozilla/security/manager/ssl/src/nsCrypto.cpp:1764
#11 0x01fdbaf8 in nsCrypto::GenerateCRMFRequest (this=0xa4b4e130, aReturn=0xbfffbb68) at /plaindata/moz/20/mozilla/security/manager/ssl/src/nsCrypto.cpp:1978
|
Reporter | |
Comment 1•14 years ago
|
||
Now the question is, should we
(a)
simply increase the limit
or
(b)
inside lib/crmf, could we query the desired output buffer size?
We're here:
CRMFEncryptedValue *
crmf_create_encrypted_value_wrapped_privkey(SECKEYPrivateKey *inPrivKey,
SECKEYPublicKey *inCAKey,
CRMFEncryptedValue *destValue)
|
||
Comment 2•14 years ago
|
||
Wether we hard code ir or not, Could we have it defined in only one place?
http://mxr.mozilla.org/security/ident?i=MAX_WRAPPED_KEY_LEN
shows the constant difined in two places
* security/jss/org/mozilla/jss/pkcs11/PK11KeyWrapper.c (....)
o line 60 -- #define MAX_WRAPPED_KEY_LEN 4096
* security/nss/lib/crmf/crmfi.h (View CVS log or CVS annotations)
o line 51 -- #define MAX_WRAPPED_KEY_LEN 2048
|
|
||
Comment 3•14 years ago
|
||
Andrew, this change under discussion won't allow you to do 8092 keys as you aked. Perhaps #define MAX_WRAPPED_KEY_LEN 16384 would be a better value.
|
||
Comment 4•14 years ago
|
||
(sigh).
There's another define in blapi.h for the size of an RSA key in bits.
JSS should calculate MAX_WRAPPED_KEY_LEN (in bytes) from that (see below).
For now crmfi should also get max_wrapped key length from there, but really crmf should be written to be size independent. The rest of NSS handles any size RSA keys. (Only softoken limits the size of a key -- and that is just to prevent DOS attacks). crmf should malloc any data it needs to store and move the wrapped key.
NOTE: The definitions above hide the nature of and meaning of MAX_WRAPPED_KEY_LEN. I believe it is the maxium size of a fully wrapped RSA key in bytes, which is not necessarily the MAX_RSA_KEY_SIZE (in bits). Here is the calculation to needed to store a wrapped RSA key.
RSA Modulus (n): MAX_RSA_KEY_LENGTH_IN_BYTES + 3
RSA Public Exponent (e): 3 + 2
RSA Private Exponent (d): MAX_RSA_KEY_LENGTH_IN_BYTES +3
RSA Prime 1 (p): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
RSA Prime 2 (q): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
RSA Exponent 1 (d mod p-1): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
RSA Exponent 2 (d mod p-2): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
RSA Coefficent (q**-1 mod p): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
Padding: up to 16 bytes.
RSA wrapped encryption key: MAX_RSA_KEY_LENGTH_IN_BYTES +3
Other DER overhead: order of 20 bytes
Total: MAX_RSA_KEY_LENGTH_IN_BYTES*5.5 + 65 (call it +100).
Now MAX_RSA_KEY_LENGTH_IN_BITS = MAX_RSA_KEY_LENGTH_IN_BYTES *8,
So if we take MAX_RSA_WRAPPED_KEY_LENGTH (in bytes) = MAX_RSA_KEY_LENGTH (in bits), then we should be OK, but we should outline that is what we are doing with a nice comment.
|
|
Reporter | |
Comment 5•14 years ago
|
||
> There's another define in blapi.h for the size of an RSA key in bits.
Do you refer to file blapit.h and
#define RSA_MAX_MODULUS_BITS 8192
?
> JSS should calculate MAX_WRAPPED_KEY_LEN (in bytes) from that (see below).
Maybe JSS should do that too, but this bug is about NSS.
(Elio simply made the additional proposal that our fix here might be applied to JSS, too.)
> For now crmfi should also get max_wrapped key length from there, but really
> crmf should be written to be size independent. The rest of NSS handles any
> size RSA keys. (Only softoken limits the size of a key -- and that is just
> to prevent DOS attacks). crmf should malloc any data it needs to store and
> move the wrapped key.
>
> NOTE: The definitions above hide the nature of and meaning of
> MAX_WRAPPED_KEY_LEN. I believe it is the maxium size of a fully wrapped RSA
> key in bytes,
Yes, but not just RSA, right?
I believe that other types of keys can be wrapped, too.
(I don't know if RSA will always be the largest one.)
> which is not necessarily the MAX_RSA_KEY_SIZE (in bits). Here
> is the calculation to needed to store a wrapped RSA key.
>
> RSA Modulus (n): MAX_RSA_KEY_LENGTH_IN_BYTES + 3
> RSA Public Exponent (e): 3 + 2
> RSA Private Exponent (d): MAX_RSA_KEY_LENGTH_IN_BYTES +3
> RSA Prime 1 (p): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
> RSA Prime 2 (q): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
> RSA Exponent 1 (d mod p-1): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
> RSA Exponent 2 (d mod p-2): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
> RSA Coefficent (q**-1 mod p): MAX_RSA_KEY_LENGTH_IN_BYTES/2 +3
> Padding: up to 16 bytes.
> RSA wrapped encryption key: MAX_RSA_KEY_LENGTH_IN_BYTES +3
> Other DER overhead: order of 20 bytes
>
> Total: MAX_RSA_KEY_LENGTH_IN_BYTES*5.5 + 65 (call it +100).
> Now MAX_RSA_KEY_LENGTH_IN_BITS = MAX_RSA_KEY_LENGTH_IN_BYTES *8,
>
> So if we take MAX_RSA_WRAPPED_KEY_LENGTH (in bytes) = MAX_RSA_KEY_LENGTH (in
> bits), then we should be OK, but we should outline that is what we are doing
> with a nice comment.
Thanks a lot for this explanation,
but instead of
MAX_RSA_WRAPPED_KEY_LENGTH
don't we rather need
MAX_WRAPPED_KEY_LENGTH
?
I agree with two statements you made:
- we should make the implementation size independent
- CRMF should allocate the storage space on its own
I would like to propose, at the same time to make it algorithm independent.
If I understand correctly,
lib CRMF doesn't know the size needed for the wrapped key.
If we wanted to teach it to know it, I'm worried we might require to duplicate code from elsewhere into CRMF. I'd say it's better to avoid code duplication.
An approach I general like, is a count approach.
When calling PK11_WrapPrivKey
could we define a set of input parameters,
that will return the count of required bytes,
instead of filling a buffer?
Alternatively, could we implement a new API PK11_WrapPrivKeyGetSize
?
However, Bob, if you're sure that RSA is the largest to worry about,
then we could simply use
#include "blapit.h"
/*
* most of Bob's comment 4
*/
#define MAX_WRAPPED_KEY_LENGTH RSA_MAX_MODULUS_BITS
|
|
Reporter | |
Comment 6•14 years ago
|
||
If we decided that more discussion/patchwork is necessary before we can fix this bug correctly,
should we use an intermediate patch that uses
#define MAX_WRAPPED_KEY_LENGTH 8192
that solves Andrew's immediate problem?
|
|
||
Comment 7•14 years ago
|
||
> Do you refer to file blapit.h and
> #define RSA_MAX_MODULUS_BITS 8192
> don't we rather need
> MAX_WRAPPED_KEY_LENGTH
yes, though I would be surprised if there weren't other RSA specific things in this part of crmf. There shouldn't be but...
> I believe that other types of keys can be wrapped, too.
> (I don't know if RSA will always be the largest one.)
It is.
DH keys:
Prime (p) MAX_DH_KEY_LENGTH_IN_BYTES + 3
Base (g) MAX_DH_KEY_LENGTH_IN_BYTES + 3
Private Key (x) MAX_DH_KEY_LENGTH_IN_BYTES + 3
Public key (X) MAX_DH_KEY_LENGTH_IN BYTES + 3 - ?
Padding: up to 16 bytes.
RSA wrapped encryption key: MAX_RSA_KEY_LENGTH_IN_BYTES +3
Other DER overhead: order of 20 bytes
MAX_DH_KEY_LENGTH_IN_BYTES is going to be the same or less than MAX_RSA_KEY_LENGTH_IN_BYTES
Total MAX_DH_KEY_SIZE_IN_BYTES*5 + 51
DSA keys:
Prime (p) MAX_DSA_KEY_LENGTH_IN_BYTES +3
Base (g) MAX_DSA_KEY_LENGTH_IN_BYTES +3
SubPrime (q) MAX_DSA_Q_LENGTH_IN_BTES +2
Private Key (x) MAX_DSA_Q_LENGTH_IN BYTES +2
Padding: up to 16 bytes.
RSA wrapped encryption key: MAX_RSA_KEY_LENGTH_IN_BYTES +3
Other DER overhead: order of 20 byte
Total MAX_DSA_KEY_LENGTH_IN_BYTES*3 + MAX_RSA_KEY_LENGTH_IN_BYTES + MAX_DSA_Q_LENGTH_IN_BYTES*2 + 49 bytes
Our current code uses has MAX_DSA_KEY_LENGTH_IN_BYTES = 128 and MAX_DSA_Q_LENGTH_IN_BYTES = 20 . In theory we could support a new version of DSA which has MAX_DSA_KEY_LENGTH_IN_BYTES = MAX_RSA_KEY_LENGTH_IN_BYTES. For the sizes we are talking about MAX_DSA_Q_LENGTH_IN_BYTES is << than MAX_DSA_KEY_LENGTH_IN_BYTES/2, so DSA keys are guarrenteed to be less than:
MAX_RSA_KEY_LENGTH_IN_BYTES *5 +49
ECC keys:
Curve Oid 20 bytes
Private Key MAX_ECC_KEY_LENGTH * 2.
Padding: up to 16 bytes.
RSA wrapped encryption key: MAX_RSA_KEY_LENGTH_IN_BYTES +3
Other DER overhead: order of 20 byte
Total: MAX_ECC_KEY_LENGTH_IN_BYTES + MAX_RSA_KEY_LENGTH_IN_BYTES + 61
MAX_ECC_KEY_LENGTH_IN_BYTES is the same as MAX_DH_Q_LENGTH in the 'general' DSA case (currently 64-65 bytes). Clearly this key won't be larger than our RSA key sizes.
NOTE: the RSA wrapped encryption key is the same for all of these. We currently only support RSA for escrowing keys. Using DH or ECDH would require protocol changes. In that case the keys would be slightly larger, but still all be less than RSA_KEY_SIZE_IN_BYTES * 8.
> If I understand correctly, lib CRMF doesn't know the size needed for
> the wrapped key.
It should be able to query that. How do we do it in PKCS #12? It's really the same code. My guess is crmf is using a static buffer rather than a dynamic one.
> #include "blapit.h"
> /*
> * most of Bob's comment 4
> */
> #define MAX_WRAPPED_KEY_LENGTH RSA_MAX_MODULUS_BITS
That would be my recommendation until we can match crmf so it doesn't need MAX_WRAPPED_KEY_LENGTH.
|
|
Reporter | |
Comment 8•14 years ago
|
||
> > #include "blapit.h"
> > /*
> > * most of Bob's comment 4
> > */
> > #define MAX_WRAPPED_KEY_LENGTH RSA_MAX_MODULUS_BITS
>
> That would be my recommendation until we can match crmf so it doesn't need
> MAX_WRAPPED_KEY_LENGTH.
OK.
It built without the #include
Is my summary sufficient? I left out the details of the calculation, and instead pointed to this bug.
Let's get NSS fixed first.
Attachment #531716 -
Flags: review?(rrelyea)
|
|
Reporter | |
Comment 9•14 years ago
|
||
(In reply to comment #8)
>
> OK.
> It built without the #include
Actually, no, it doesn't build.
The build dependency system for crmf is nonworking.
|
|
Reporter | |
Comment 10•14 years ago
|
||
patch that builds
Attachment #531716 -
Attachment is obsolete: true
Attachment #531716 -
Flags: review?(rrelyea)
Attachment #531721 -
Flags: review?(rrelyea)
|
|
Reporter | |
Comment 11•14 years ago
|
||
I verified this patch works with Andrew's testcase.
|
|
Reporter | |
Comment 12•14 years ago
|
||
I tried both 4K and 8K RSA keys. Both work fine with this patch. Andrew, I'll send you that 8K request by email.
|
|
||
Comment 13•14 years ago
|
||
Comment on attachment 531721 [details] [diff] [review]
Patch v2
r+ rrelyea
Attachment #531721 -
Flags: review?(rrelyea) → review+
|
|
Reporter | |
Comment 14•14 years ago
|
||
trunk for 3.13:
Checking in crmfi.h;
/cvsroot/mozilla/security/nss/lib/crmf/crmfi.h,v <-- crmfi.h
new revision: 1.5; previous revision: 1.4
branch for 3.12.11:
Checking in crmfi.h;
/cvsroot/mozilla/security/nss/lib/crmf/crmfi.h,v <-- crmfi.h
new revision: 1.3.192.1; previous revision: 1.3
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
|
|
Reporter | |
Updated•14 years ago
|
Target Milestone: --- → 3.12.11
You need to log in
before you can comment on or make changes to this bug.
Description
•