Closed
Bug 326503
Opened 18 years ago
Closed 18 years ago
producing a ProofOfPossession signature on a EC CRMF fails
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.11
People
(Reporter: KaiE, Assigned: rrelyea)
References
Details
(Whiteboard: ECC)
Attachments
(4 files, 4 obsolete files)
2.26 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
wtc
:
review+
KaiE
:
superreview+
|
Details | Diff | Splinter Review |
1.58 KB,
text/plain
|
Details | |
1.81 KB,
patch
|
rrelyea
:
review+
nelson
:
review+
|
Details | Diff | Splinter Review |
I worked on a patch to make crypto.generateCRMFRequest work with EC keys. First I crashed, then I disabled some cleanup functions, then I saw a failure, because of an invalid algorithm mapping. I tracked down the failure to seckey.c, SECKEY_CreateSubjectPublicKeyInfo() If input public key is an ecKey, this function calls SECOID_SetAlgorithmID(SEC_OID_ANSIX962_EC_PUBLIC_KEY) Using SEC_OID_ANSIX962_EC_PUBLIC_KEY looks wrong, a public key is not an algo, is it? I naively searched the NSS code for a EC related algo and found SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE. Using that seems to fix the bug for me. All NSS and application code succeeds with this change. Also, the crash I mentioned in the first place no longer appears.
Reporter | ||
Comment 1•18 years ago
|
||
Attachment #211234 -
Flags: review?(rrelyea)
Assignee | ||
Comment 2•18 years ago
|
||
Comment on attachment 211234 [details] [diff] [review] Patch v1 r- we must be crashing for another reason. SEC_OID_ANSIX962_EC_PUBLIC_KEY is definately the correct oid here. bob
Attachment #211234 -
Flags: review?(rrelyea) → review-
Reporter | ||
Comment 3•18 years ago
|
||
I stepped with gdb to this position: #0 SEC_SignData (res=0x7fffffcb0120, buf=0x120fd90 "0\201\002\004.\r\232�\201\211\200\001\002\0270\0251\0230\021\006\003U\004\003\023\nKai EngertY0\023\006\a*\206H�\002\001\006\b*\206H�\003\001\a\003B", len=189, pk=0x1272ce0, algid=SEC_OID_ANSIX962_EC_PUBLIC_KEY) at secsign.c:315 #1 0x00002aaab307dd58 in crmf_sign_certreq (poolp=0xeeb0e0, crmfSignKey=0x1271968, certReq=0x12714e0, inKey=0x1272ce0, inAlgId=0x1271930) at crmfpop.c:217 #2 0x00002aaab307de81 in crmf_create_poposignkey (poolp=0xeeb0e0, inCertReqMsg=0x12714a0, signKeyInput=0x0, inPrivKey=0x1272ce0, inAlgID=0x1271930, signKey=0x1271968) at crmfpop.c:263 #3 0x00002aaab307e020 in CRMF_CertReqMsgSetSignaturePOP (inCertReqMsg=0x12714a0, inPrivKey=0x1272ce0, inPubKey=0x1270c70, inCertForInput=0x0, fn=0, arg=0x0) at crmfpop.c:317 #4 0x00002aaab304e74c in nsSetProofOfPossession (certReqMsg=0x12714a0, keyInfo=0x116b820) at /home/kaie/moz/18-ecc/mozilla/security/manager/ssl/src/nsCrypto.cpp:1403 #5 0x00002aaab304ea42 in nsCreateReqFromKeyPairs (keyids=0x116b820, numRequests=1, reqDN=0x116b6c0 "CN=Kai Engert", regToken=0x116b710 "bla1", authenticator=0x116b760 "bla2", wrappingCert=0x0) at /home/kaie/moz/18-ecc/mozilla/security/manager/ssl/src/nsCrypto.cpp:1502 #6 0x00002aaab3051300 in nsCrypto::GenerateCRMFRequest (this=0x116a950, aReturn=0x7fffffcb0730) at /home/kaie/moz/18-ecc/mozilla/security/manager/ssl/src/nsCrypto.cpp:1685 The code being executed is: sgn = SGN_NewContext(algid, pk); algid = SEC_OID_ANSIX962_EC_PUBLIC_KEY In function SGN_NewContext there is a switch(alg) but NO case SEC_OID_ANSIX962_EC_PUBLIC_KEY. We end up at PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
Reporter | ||
Comment 4•18 years ago
|
||
FYI: The file names and line numbers were produced from the NSS 3.11 branch.
Assignee | ||
Comment 5•18 years ago
|
||
The problem is in crmf. SubjectPublicKeys have key oids, while signing operations take signature oids. In some places these have been confused. Are you seeing this crash on the up to data 3.11 branch?
Reporter | ||
Comment 6•18 years ago
|
||
> Are you seeing this crash on the up to data 3.11 branch?
Yes, my tree is up to date. I just updated, and the only new change I got was a checkin from yesterday to cmd/platlibs.mk
Comment 7•18 years ago
|
||
marking P2. Do you think this is P1? Who's the right owner for this?
Priority: -- → P2
Whiteboard: ECC
Reporter | ||
Comment 8•18 years ago
|
||
I think Bob probably knows best what's going wrong. Bob, please let me know if you need me to provide more details. Regarding priority, this bug blocks my work on testing CRMF functionality and complete the work of in-browser key generation / cert requesting.
Assignee: wtchang → rrelyea
Assignee | ||
Comment 9•18 years ago
|
||
This patch fixes the base problem. It should work for the tip, but no for the NSS 3.11 branch. Kai could you check to see if this solves your problem. I'll attach a version for the NSS 3.11 branch shortly.
Attachment #211234 -
Attachment is obsolete: true
Reporter | ||
Comment 10•18 years ago
|
||
> Kai could you check to see if this solves your problem.
I have not yet attempted to build a Mozilla client with the tip of NSS.
Should I try, or should I wait for your branch patch?
Assignee | ||
Comment 11•18 years ago
|
||
This patch is for 3.11, as the SEC_GetSignatureAlgorithmOidTag function was only added in 3.12. bob
Reporter | ||
Comment 12•18 years ago
|
||
Bob, thanks for the patch. I tested your 3.11 version, it fixes the crash for me.
Updated•18 years ago
|
Priority: P2 → P1
Assignee | ||
Updated•18 years ago
|
Attachment #213377 -
Flags: review?(wtchang)
Assignee | ||
Updated•18 years ago
|
Attachment #213378 -
Flags: superreview?(nelson)
Attachment #213378 -
Flags: review?(wtchang)
Comment 13•18 years ago
|
||
Comment on attachment 213378 [details] [diff] [review] 3.11 version r=nelson One question, should we be using SHA1 instead of MD5 here: >+ switch (inKey->keyType) { >+ case rsaKey: >+ sigAlgTag = SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION; >+ break;
Attachment #213378 -
Flags: superreview?(nelson) → review+
Assignee | ||
Comment 14•18 years ago
|
||
RE: SHA1 versus MD5 We may want to default to different hashes in the future, but I chose to maintain the values what we are currently generating for the 3.11 patch. bob
Comment 15•18 years ago
|
||
Comment on attachment 213378 [details] [diff] [review] 3.11 version My code tracing showed that for RSA keys we are currently using SHA1 (see crmf_map_keytag_to_signtag), so this patch will create a discrepancy for RSA keys because we will pass RSA-with-MD5 to SEC_SignData but continue to set pop->popChoice.signature.algorithmIdentifier to RSA-with-SHA1 in CRMF_CertReqMsgSetSignaturePOP. I found this problem because I noticed that this patch makes inAlgID an unused parameter to crmf_sign_certreq and its sole caller crmf_create_poposignkey. Then I looked at how CRMF_CertReqMsgSetSignaturePOP creates the algID and passes it to crmf_create_poposignkey. All the pertinent functions have only one caller, so it's easy to trace all the relevant code. The simplest way to fix this bug is to enhance crmf_map_keytag_to_signtag to handle EC public keys.
Attachment #213378 -
Flags: review?(wtchang) → review-
Comment 16•18 years ago
|
||
Comment on attachment 213377 [details] [diff] [review] Patch for nsstip Same problem. SEC_GetSignatureAlgorithmOidTag and crmf_map_keytag_to_signtag pick different hash algorithms for RSA keys. The best way to avoid such a discrepancy is to use the same function to do the key-to-signature-algorithm mapping.
Attachment #213377 -
Flags: review?(wtchang) → review-
Assignee | ||
Comment 17•18 years ago
|
||
Arg yet another mapping function. We need to consolidate all these functions into one (sigh).
Comment 18•18 years ago
|
||
Kai, in comment 3 you posted the stack trace of a function failure that lead to the crash. Could you post the stack trace of the crash itself? By code inspection, I believe we are crashing in the PORT_Free(certReqSig.data) call in crmf_sign_certreq because certReqSig.data is not initialized. I wonder why compilers don't warn about this. This patch initializes the 'data' fields of those two SECItem's to NULL, so we won't crash. Then, it adds a case for EC public keys to the switch statement in crmf_map_keytag_to_signtag. This will fix the failure that leads to the crash. The crmf_map_keytag_to_signtag function simply returns the input SECOidTag if the input SECOidTag is not one of the public key algorithms in the switch statement. I believe this default return value is what led Bob on the wrong path. Perhaps the default case in that switch statement should return SEC_OID_UNKNOWN instead. This patch also makes that change. I agree with Bob that we should consolidate all the mapping functions into one. The purpose of this patch is to provide a small fix that may be more appropriate for a patch release, and to note that we should fix the crash itself as well as the failure that leads to the crash.
Attachment #213377 -
Attachment is obsolete: true
Attachment #213378 -
Attachment is obsolete: true
Attachment #213651 -
Flags: review?(rrelyea)
Assignee | ||
Comment 19•18 years ago
|
||
Comment on attachment 213651 [details] [diff] [review] Small patch to fix the crash and to add EC public key support r+ relyea for the 3.11 branch. For the 3.12 branch, we should colapse crmf_map_keytag_to_signtag with the function that crmf_get_key_sign_tag, and have the latter call SEC_GetSignatureAlgorithmOidTag for all keys except RSA (to keep backward compatibility). bob
Attachment #213651 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 20•18 years ago
|
||
Attachment #213660 -
Flags: review?(wtchang)
Comment 21•18 years ago
|
||
Comment on attachment 213660 [details] [diff] [review] Small more correct patch for the tip (requires new NSS 3.12 function) Bob, you attached the wrong patch.
Attachment #213660 -
Attachment is obsolete: true
Attachment #213660 -
Flags: review?(wtchang)
Reporter | ||
Comment 23•18 years ago
|
||
> Could you post the stack trace of the crash itself?
> I believe we are crashing in the PORT_Free(certReqSig.data)
> call in crmf_sign_certreq
I'm attaching the stack.
You are right.
Reporter | ||
Comment 24•18 years ago
|
||
Comment on attachment 213651 [details] [diff] [review] Small patch to fix the crash and to add EC public key support I confirm this patch fixes the crash when applied to NSS_3_11_BRANCH.
Comment 25•18 years ago
|
||
Comment on attachment 213661 [details] [diff] [review] Small more correct patch for the tip (requires new NSS 3.12 function) Bob, instead of this special case for RSA keys >+ /* maintain backward compatibility with older >+ * implementations */ >+ if (inPubKey->keyType == rsaKey) { > return SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION; > } why don't we just pass SEC_OID_SHA1 to SEC_GetSignatureAlgorithmOidTag? I also recommend my changes to crmf_sign_certreq and CRMF_CertReqMsgSetSignaturePOP to fix the crash if SEC_GetSignatureAlgorithmOidTag returns SEC_OID_UNKNOWN.
Attachment #213661 -
Attachment description: patch 2006-03-01 13:56 PST 2.26 KB rrelyea: review+
Edit | Diff
Small more correct patch for the tip (requires new NSS 3.12 function) → Small more correct patch for the tip (requires new NSS 3.12 function)
Assignee | ||
Comment 26•18 years ago
|
||
Because I want to allow better defaults in the future. I really don't want SHA1 hardcoded were we really don't need it. bob
Comment 27•18 years ago
|
||
(In reply to comment #26) > Because I want to allow better defaults in the future. > I really don't want SHA1 hardcoded were we really don't need it. I think Wan-Teh was suggesting something like this: SECOidTag +crmf_get_key_sign_tag(SECKEYPublicKey *inPubKey) { + /* maintain backward compatibility with older + * implementations */ + return SEC_GetSignatureAlgorithmOidTag(inPubKey->keyType, + (inPubKey->keyType == rsaKey ? SEC_OID_SHA1 : SEC_OID_UNKNOWN)); } Concerning "hard coding" of SHA1, it seems to me that the line > return SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION; hard codes the use of SHA1 just as much as Wan-Teh's proposal. no?
Comment 28•18 years ago
|
||
Nelson, what I suggested is: SECOidTag +crmf_get_key_sign_tag(SECKEYPublicKey *inPubKey) { + return SEC_GetSignatureAlgorithmOidTag(inPubKey->keyType, SEC_OID_SHA1); } Bob's patch also hardcodes SHA-1, except that it's only for RSA keys. Perhaps we should change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default for all types of keys.
Assignee | ||
Comment 29•18 years ago
|
||
I prefer nelson's code. I don't think we want to hardcode SHA1 for all keys... especially since there is likely to be a move away from SHA1 in the future. bob
Comment 30•18 years ago
|
||
SEC_GetSignatureAlgorithmOidTag was added in NSS 3.10 (bug 285233) and then used to fix bug 265003. I don't know why it picks MD5 as the default hash algorithm for RSA signatures. This patch changes SEC_GetSignatureAlgorithmOidTag to use SHA1 as the default hash algorithm for RSA signatures. I also tried to improvecat the comment for this function, but I'm not sure how to best describe under what condition the function returns SEC_OID_UNKNOWN.
Attachment #213969 -
Flags: review?(rrelyea)
Comment 31•18 years ago
|
||
I found that SEC_GetSignatureAlgorithmOidTag was originally the static function getSignatureOidTag in cmd/certutil/certutil.c. getSignatureOidTag used MD5 as the default hash algorithm for RSA signatures. The default hash algorithm is used if certutil's -Z hash type option is not specified when signing a cert or cert request. I think it's fine to change the default hash algorithm for RSA signatures to SHA-1.
Assignee | ||
Comment 32•18 years ago
|
||
Comment on attachment 213969 [details] [diff] [review] Change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default with RSA r+ relyea. We probably shouldn't be defaulting to md5 anywhere anymore.
Attachment #213969 -
Flags: review?(rrelyea) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #213651 -
Flags: superreview?(julien.pierre.bugs)
Comment 33•18 years ago
|
||
Comment on attachment 213661 [details] [diff] [review] Small more correct patch for the tip (requires new NSS 3.12 function) Bob, I believe the new function you referred to is SEC_GetSignatureAlgorithmOidTag. This function was added in NSS 3.10, not 3.12. So this patch is also suitable for the NSS_3_11_BRANCH.
Attachment #213661 -
Flags: review?(wtchang) → review+
Assignee | ||
Comment 34•18 years ago
|
||
Ah, you're right. The NSS 3.12 functions I was thinking of were the new VFY_Verify functions. Let's get the second review on the small correction then...
Assignee | ||
Updated•18 years ago
|
Attachment #213661 -
Flags: superreview?(julien.pierre.bugs)
Comment 35•18 years ago
|
||
Comment on attachment 213651 [details] [diff] [review] Small patch to fix the crash and to add EC public key support Cancelling review request since this patch isn't needed .
Attachment #213651 -
Flags: superreview?(julien.pierre.bugs)
Assignee | ||
Comment 36•18 years ago
|
||
wtc's "Change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default with RSA" patch checked into tip: Checking in cryptohi.h; /cvsroot/mozilla/security/nss/lib/cryptohi/cryptohi.h,v <-- cryptohi.h new revision: 1.11; previous revision: 1.10 done Checking in secsign.c; /cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v <-- secsign.c new revision: 1.17; previous revision: 1.16 done
Assignee | ||
Comment 37•18 years ago
|
||
"Small more correct patch for the tip (requires new NSS 3.12 function)" checked into tip.. cvs commit crmfpop.c Checking in crmfpop.c; /cvsroot/mozilla/security/nss/lib/crmf/crmfpop.c,v <-- crmfpop.c new revision: 1.4; previous revision: 1.3 done ... needs 2nd review for 3.11
Assignee | ||
Comment 38•18 years ago
|
||
Comment on attachment 213661 [details] [diff] [review] Small more correct patch for the tip (requires new NSS 3.12 function) kaie, could you review this patch so we can get it into 3.11? thanks.
Attachment #213661 -
Flags: superreview?(julien.pierre.bugs) → superreview?(kengert)
Reporter | ||
Comment 39•18 years ago
|
||
Comment on attachment 213661 [details] [diff] [review] Small more correct patch for the tip (requires new NSS 3.12 function) So with this patch, the new code looks like: SECOidTag +crmf_get_key_sign_tag(SECKEYPublicKey *inPubKey) { + /* maintain backward compatibility with older + * implementations */ + if (inPubKey->keyType == rsaKey) { return SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION; } + return SEC_GetSignatureAlgorithmOidTag(inPubKey->keyType, SEC_OID_UNKNOWN); } On the trunk of NSS you have changed the default to SHA-1, that means the if-clause-special-case is not needed, because SEC_GetSignatureAlgorithmOidTag will produce the same return value. I assume you have a good reason to not change the default on the 3.11 branch. Therefore, the if-clause seems reasonable on the 3.11 branch. r=kengert
Attachment #213661 -
Flags: superreview?(kengert) → superreview+
Assignee | ||
Comment 40•18 years ago
|
||
Checking in crmfpop.c; /cvsroot/mozilla/security/nss/lib/crmf/crmfpop.c,v <-- crmfpop.c new revision: 1.3.28.1; previous revision: 1.3 done Checked into 3.11
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 41•15 years ago
|
||
Comment on attachment 213969 [details] [diff] [review] Change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default with RSA r=nelson for 3.11 branch In comments 30-34, we discussed applying this patch to the 3.11 branch also, but never did. In light of recent news about MD5, I believe we need to do so now.
Attachment #213969 -
Flags: review+
Comment 42•15 years ago
|
||
Comment on attachment 213969 [details] [diff] [review] Change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default with RSA I checked in this patch on the NSS_3_11_BRANCH (NSS 3.11.11). Checking in cryptohi.h; /cvsroot/mozilla/security/nss/lib/cryptohi/cryptohi.h,v <-- cryptohi.h new revision: 1.9.28.2; previous revision: 1.9.28.1 done Checking in secsign.c; /cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v <-- secsign.c new revision: 1.14.2.4; previous revision: 1.14.2.3 done
You need to log in
before you can comment on or make changes to this bug.
Description
•