producing a ProofOfPossession signature on a EC CRMF fails

RESOLVED FIXED in 3.11.11

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: kaie, Assigned: Robert Relyea)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ECC)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
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)

Updated

11 years ago
Blocks: 326159
(Reporter)

Comment 1

11 years ago
Created attachment 211234 [details] [diff] [review]
Patch v1
Attachment #211234 - Flags: review?(rrelyea)
(Assignee)

Comment 2

11 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

11 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

11 years ago
FYI: The file names and line numbers were produced from the NSS 3.11 branch.
(Assignee)

Comment 5

11 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

11 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
marking P2.  Do you think this is P1?
Who's the right owner for this?
Priority: -- → P2
Whiteboard: ECC
(Reporter)

Comment 8

11 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

11 years ago
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.
Attachment #211234 - Attachment is obsolete: true
(Reporter)

Comment 10

11 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

11 years ago
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
(Reporter)

Comment 12

11 years ago
Bob, thanks for the patch.
I tested your 3.11 version, it fixes the crash for me.
Priority: P2 → P1
(Assignee)

Updated

11 years ago
Attachment #213377 - Flags: review?(wtchang)
(Assignee)

Updated

11 years ago
Attachment #213378 - Flags: superreview?(nelson)
Attachment #213378 - Flags: review?(wtchang)
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

11 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

11 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

11 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

11 years ago
Arg yet another mapping function. We need to consolidate all these functions into one (sigh).

Comment 18

11 years ago
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.
Attachment #213377 - Attachment is obsolete: true
Attachment #213378 - Attachment is obsolete: true
Attachment #213651 - Flags: review?(rrelyea)
(Assignee)

Comment 19

11 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

11 years ago
Created attachment 213660 [details] [diff] [review]
Small more correct patch for the tip (requires new NSS 3.12 function)
Attachment #213660 - Flags: review?(wtchang)

Comment 21

11 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)
(Assignee)

Comment 22

11 years ago
Created attachment 213661 [details] [diff] [review]
Small more correct patch for the tip (requires new NSS 3.12 function)

Hmm try this one
Attachment #213661 - Flags: review?(wtchang)
(Reporter)

Comment 23

11 years ago
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.
(Reporter)

Comment 24

11 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

11 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

11 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
(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

11 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

11 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

11 years ago
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.
Attachment #213969 - Flags: review?(rrelyea)

Comment 31

11 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

11 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

11 years ago
Attachment #213651 - Flags: superreview?(julien.pierre.bugs)

Comment 33

11 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

11 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

11 years ago
Attachment #213661 - Flags: superreview?(julien.pierre.bugs)

Comment 35

11 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

11 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

11 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

11 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

11 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

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED
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

8 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
Thanks, Wan-Teh,  You beat me to it.
Target Milestone: 3.11.1 → 3.11.11
You need to log in before you can comment on or make changes to this bug.