Last Comment Bug 326503 - producing a ProofOfPossession signature on a EC CRMF fails
: producing a ProofOfPossession signature on a EC CRMF fails
Status: RESOLVED FIXED
ECC
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: P1 normal (vote)
: 3.11.11
Assigned To: Robert Relyea
: Jason Reid
Mentors:
Depends on:
Blocks: 326159
  Show dependency treegraph
 
Reported: 2006-02-08 21:39 PST by Kai Engert (:kaie)
Modified: 2009-01-07 11:34 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (666 bytes, patch)
2006-02-08 21:43 PST, Kai Engert (:kaie)
rrelyea: review-
Details | Diff | Review
Patch for nsstip (1.02 KB, patch)
2006-02-27 14:11 PST, Robert Relyea
wtc: review-
Details | Diff | Review
3.11 version (1.12 KB, patch)
2006-02-27 14:23 PST, Robert Relyea
wtc: review-
nelson: review+
Details | Diff | Review
Small patch to fix the crash and to add EC public key support (2.26 KB, patch)
2006-03-01 13:56 PST, Wan-Teh Chang
rrelyea: review+
Details | Diff | Review
Small more correct patch for the tip (requires new NSS 3.12 function) (14.65 KB, patch)
2006-03-01 14:31 PST, Robert Relyea
no flags Details | Diff | Review
Small more correct patch for the tip (requires new NSS 3.12 function) (1.40 KB, patch)
2006-03-01 14:36 PST, Robert Relyea
wtc: review+
kaie: superreview+
Details | Diff | Review
Stack of crash (1.58 KB, text/plain)
2006-03-01 15:10 PST, Kai Engert (:kaie)
no flags Details
Change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default with RSA (1.81 KB, patch)
2006-03-03 22:46 PST, Wan-Teh Chang
rrelyea: review+
nelson: review+
Details | Diff | Review

Description Kai Engert (:kaie) 2006-02-08 21:39:24 PST
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.
Comment 1 Kai Engert (:kaie) 2006-02-08 21:43:11 PST
Created attachment 211234 [details] [diff] [review]
Patch v1
Comment 2 Robert Relyea 2006-02-08 22:03:06 PST
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
Comment 3 Kai Engert (:kaie) 2006-02-08 22:13:28 PST
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);
Comment 4 Kai Engert (:kaie) 2006-02-08 22:18:52 PST
FYI: The file names and line numbers were produced from the NSS 3.11 branch.
Comment 5 Robert Relyea 2006-02-09 07:17:58 PST
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?
Comment 6 Kai Engert (:kaie) 2006-02-09 09:44:24 PST
> 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 Nelson Bolyard (seldom reads bugmail) 2006-02-27 12:17:53 PST
marking P2.  Do you think this is P1?
Who's the right owner for this?
Comment 8 Kai Engert (:kaie) 2006-02-27 12:21:49 PST
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.
Comment 9 Robert Relyea 2006-02-27 14:11:30 PST
Created attachment 213377 [details] [diff] [review]
Patch for nsstip

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.
Comment 10 Kai Engert (:kaie) 2006-02-27 14:13:37 PST
> 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?
Comment 11 Robert Relyea 2006-02-27 14:23:14 PST
Created attachment 213378 [details] [diff] [review]
3.11 version

This patch is for 3.11, as the SEC_GetSignatureAlgorithmOidTag function was only added in 3.12.

bob
Comment 12 Kai Engert (:kaie) 2006-02-27 18:05:40 PST
Bob, thanks for the patch.
I tested your 3.11 version, it fixes the crash for me.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2006-02-28 18:26:42 PST
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;
Comment 14 Robert Relyea 2006-03-01 07:43:52 PST
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 Wan-Teh Chang 2006-03-01 08:10:45 PST
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.
Comment 16 Wan-Teh Chang 2006-03-01 08:15:05 PST
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.
Comment 17 Robert Relyea 2006-03-01 08:29:39 PST
Arg yet another mapping function. We need to consolidate all these functions into one (sigh).
Comment 18 Wan-Teh Chang 2006-03-01 13:56:58 PST
Created attachment 213651 [details] [diff] [review]
Small patch to fix the crash and to add EC public key support

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.
Comment 19 Robert Relyea 2006-03-01 14:25:48 PST
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
Comment 20 Robert Relyea 2006-03-01 14:31:46 PST
Created attachment 213660 [details] [diff] [review]
Small more correct patch for the tip (requires new NSS 3.12 function)
Comment 21 Wan-Teh Chang 2006-03-01 14:33:52 PST
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.
Comment 22 Robert Relyea 2006-03-01 14:36:04 PST
Created attachment 213661 [details] [diff] [review]
Small more correct patch for the tip (requires new NSS 3.12 function)

Hmm try this one
Comment 23 Kai Engert (:kaie) 2006-03-01 15:10:16 PST
Created attachment 213675 [details]
Stack of crash

> 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.
Comment 24 Kai Engert (:kaie) 2006-03-01 15:17:38 PST
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 Wan-Teh Chang 2006-03-01 15:29:23 PST
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.
Comment 26 Robert Relyea 2006-03-03 11:30:19 PST
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 Nelson Bolyard (seldom reads bugmail) 2006-03-03 11:58:17 PST
(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 Wan-Teh Chang 2006-03-03 12:23:04 PST
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.
Comment 29 Robert Relyea 2006-03-03 14:03:36 PST
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 Wan-Teh Chang 2006-03-03 22:46:45 PST
Created attachment 213969 [details] [diff] [review]
Change SEC_GetSignatureAlgorithmOidTag to use SHA-1 by default with RSA

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.
Comment 31 Wan-Teh Chang 2006-03-03 23:09:15 PST
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.
Comment 32 Robert Relyea 2006-03-08 09:55:56 PST
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.
Comment 33 Wan-Teh Chang 2006-03-08 15:35:00 PST
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.
Comment 34 Robert Relyea 2006-03-10 10:04:24 PST
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...
Comment 35 Julien Pierre 2006-03-13 17:38:23 PST
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 .
Comment 36 Robert Relyea 2006-03-15 13:43:13 PST
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
Comment 37 Robert Relyea 2006-03-15 13:47:40 PST
"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
Comment 38 Robert Relyea 2006-03-21 08:54:03 PST
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.
Comment 39 Kai Engert (:kaie) 2006-03-21 09:50:44 PST
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
Comment 40 Robert Relyea 2006-03-21 10:01:11 PST
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
Comment 41 Nelson Bolyard (seldom reads bugmail) 2009-01-06 18:11:35 PST
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.
Comment 42 Wan-Teh Chang 2009-01-07 11:20:18 PST
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
Comment 43 Nelson Bolyard (seldom reads bugmail) 2009-01-07 11:34:00 PST
Thanks, Wan-Teh,  You beat me to it.

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