320583 - NSS ECDSA can only sign SHA-1

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Robert Relyea]

NSS can only sign and verify signature with SHA-1 hashes.
   For NSS 3.12 I'd like to fix the general case of ECDSA, which involves adding new VFY_ api calls.
   For NSS 3.11 I want to add support to handle ECDSA signature in certificates (internal NSS), and the ability to use the ECDSA_WITH_SHAXXX oids (which do not require an interface change).

Comment 1

[Robert Relyea]

Attachment: Proposed VFY interface changes....

here's the proposed interface changes to VFY. It essentially changes the code from a call that takes a 'signature Algtag' (one that has both signature and hash functions) to one that takes separate 'encryption' and hash algtags, or replaces the signature Algtag with a full signature AlgorithmID. Several of the functions also now return the hash value used back to the application if the application requests it.

Comment 2

[Robert Relyea]

Attachment: NSS 3.12 changes for the new interface.

These patch makes the following changes:
   1) implements the new proposed VFY_ interfaces.
   2) changes internal NSS code to use the new interfaces.
   3) reorganizes some of the existing code in secvfy.c and secsign.c to limit knowledge of algorithms as much as possible.
   4) Adjust some of the error codes in secvfy and secsign to make S/MIME code happier.
   4) remove the knowledge of signing and verification algorithms from S/MIME and PKCS #7.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206137 [details] [diff] [review]
Proposed VFY interface changes....

>@@ -246,28 +283,105 @@
> ** Verify the signature on a block of data for which we already have
> ** the digest. The signature data is an RSA private key encrypted
> ** block of data formatted according to PKCS#1.
>+** >> This function is depricated. Use VFY_VerifyDigestDirect or 

deprecated is misspelled, here and below.

> /*
> ** Verify the signature on a block of data. The signature data is an RSA
> ** private key encrypted block of data formatted according to PKCS#1.
>+** >> This function is depricated. Use VFY_VerifyDataDirect or

Comment 4

[Julien Pierre]

Comment on attachment 206137 [details] [diff] [review]
Proposed VFY interface changes....

Please change the comments in VFY_CreateContext .  

s/This version is deprecated//
s/>> //

s/depricated/deprecated/

For VFY_CreateContextDirect :

- What are the cases where the returned hash algorithm is different from the one passed in ? The API suggests they are always the same, but you wouldn't have added this argument if that was true.

- It would be nice to add the use of const for the arguments in this API since this is a new function. This would make it easier to spot the input and output arguments . But this may create const propagation issues.

For VFY_CreateContextWithAlgorithmID :
- in the comments, change  the end of the description for hash to be "If this value is NULL,, no hash OID is returned".

For VFY_VerifyDigest :
Again remove the ">> " and change depricated to deprecated.

For VFY_VerifyData :
Again remove ">> " and replace depricated with deprecated.

VFY_VerifyDataWithAlgorithmID :
Fix the hash description according to my earlier comments.


I notice that VFY_VerifyDataDirect and VFY_VerifyDataWithAlgorithmID are taking a separate "unsigned char * buf" and "int len". Wouldn't a SECItem* do ?
I know we can't change VFY_VerifyData which has this signature, but there is no need to perpetuate this - let's reduce the number of arguments by making this change.

Comment 5

[Robert Relyea]

> (From update of attachment 206137 [details] [diff] [review] [edit])
> Please change the comments in VFY_CreateContext .  

> s/depricated/deprecated/

Oops, I meant to do that before I created the attachment.

> For VFY_CreateContextDirect :

> - What are the cases where the returned hash algorithm is different from the
> one passed in ? The API suggests they are always the same, but you wouldn't
> have added this argument if that was true.

I debated adding that argument. there is one case where you are doing an RSA signature, and you pass the RSA signature in, you can pass SEC_OID_UNKNOWN and the code will use the signature decoded from the RSA signature.

We could just as easily restrict this and never allow the SEC_OID_UNKNOWN case. (you have to know you are dealing with RSA in order to use it).

> - It would be nice to add the use of const for the arguments in this API since
> this is a new function. This would make it easier to spot the input and output
> arguments . But this may create const propagation issues.

I did add some const (SECAlgorithmID). I guess we could add const to sig as well.

> I notice that VFY_VerifyDataDirect and VFY_VerifyDataWithAlgorithmID are taking
> a separate "unsigned char * buf" and "int len". Wouldn't a SECItem* do ?
> I know we can't change VFY_VerifyData which has this signature, but there is no
> need to perpetuate this - let's reduce the number of arguments by making this
change.

It's always a question if you should combined data into a single secitem or pass it in as separate buf and len. Through most of NSS, raw security objects tend to be secitems, while user data tends to be buffer and length (You'll see that in the PK11 interfaces as well... The idea is that when passing data around the application typically doesn't parse, it's more convientient to pass them around in secitems, when passing data in that the application gets from a stream, that data is already in buffer, len, so it's sort of wasteful to require the application to package that up into a secitem.

Comment 6

[Robert Relyea]

Comment on attachment 206138 [details] [diff] [review]
NSS 3.12 changes for the new interface.

I'm cancelling this review request because this patch will have some minor changes based on the API review. I don't expect them to be extensive, so if you want to get a jump on the review, it's OK.

bob

Comment 7

[Robert Relyea]

Attachment: New VFY API changes.

Here is the updated API. I added const and fixed up the comments. I did not remove the pointer to the returned hash in VFY_CreateContextDirect and VFY_VerifyDataDirect, though I am willing to do so. It's a pretty weird edge case where you need it.

bob

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Changed bug summary.  Old summary suggested that only ECDSA could sign SHA1.

Comment 9

[Robert Relyea]

ping: Julien, could you comment on the new API. I'm particularly interested if we should remove the returned hashoid from the Direct functions.

bob

Comment 10

[Julien Pierre]

Comment on attachment 206315 [details] [diff] [review]
New VFY API changes.

Bob,

The new patch looks better.

My only requests are that

1) you document the case for SECOidTag* hash in VFY_CreateContextDirect in the function comments (ie. if hashAlg = SEC_OID_UNKNOWN).

2) fix this comment that appears twice :

s/If this value is NULL no, hash oid is return./
If this value is NULL, no hash oid is returned./

Comment 11

[Robert Relyea]

Attachment: NSS 3.11 version of this patch

Unlike the Tip patch, this patch does not add any new external APIs. This means that non-NSS programs will still not be able to handle ECC_SPECIFY type oids.

Particularly gross things about this patch reviewers should take not of: the new private header which will only exist in this release. (not in the trunk). I felt this was better than hand copying the private function prototype throughout NSS.

The Certificate parsing has been verified against other vendor's certs.

bob

Comment 12

[Vipul Gupta]

(In reply to comment #11)

> The Certificate parsing has been verified against other vendor's certs.

   Bob,

    I assume that you've successfully tested "certutil -V" (and not just
    "certutil -A" and "certutil -L") on ECDSA-signed certs using hashes
   other than SHA1. I bring this up because parsing (with certutil -L and 
   certutil -A) used to work for those certs even for unpatched NSS 
   but signature verifiation (with certutil -V) used to fail.

   My report on using certutil with such certs is at the SSL interop site.
   (login at http://dev.experimentalstuff.com:8082/sslinterop,
   click on "Uploaded samples" followed by "NSSCertutilResults-20051212.txt").
   I can resend you the login info privately if needed. The info
   was originally sent out by Douglas to the interop mailing list.

   vipul

Comment 13

[Robert Relyea]

Yes 'certutil -V -u V -e -n cert -d db'

I'll attach my shell scripts to the wikki.

bob

Comment 14

[Robert Relyea]

Attachment: NSS 3.12 version of this patch

This patch not only adds the new APIs to Verify, but also colapses several functions in NSS that have knowledge about the mappings of various signing OID values to their component oids, related key types, and mechanisms. After this patch, the amount of work to add a new signing algorithm should be greatly reduced.

bob

Comment 15

[Robert Relyea]

Just a ping on reviews for the NSS 3.11 and 3.12 patches for this bug.
thanks.

Comment 16

[Wan-Teh Chang]

Comment on attachment 206556 [details] [diff] [review]
NSS 3.11 version of this patch

I have some questions and suggested changes for
this patch.  Note that there is a bug in lib/cryptohi/secvfy.c.

1. lib/cryptohi/seckey.c

>+    rv = vfy_VerifyDataPrivate(sd.data.data, sd.data.len, pubKey, &sig,
>+     			SECOID_GetAlgorithmTag(&(sd.signatureAlgorithm)), 
>+			&sd.signatureAlgorithm.parameters, wincx);

I suggest removing the parentheses around sd.signatureAlgorithm
inside SECOID_GetAlgorithmTag.

2. lib/cryptohi/secvfy.c

>     switch (algid) {
>-    case SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST:
>-    case SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST:
>-    case SEC_OID_ANSIX9_DSA_SIGNATURE:
>     case SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA256_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA384_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA512_DIGEST:
>         if (algid == SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST) {
> 	    if (len > MAX_ECKEY_LEN * 2) {
> 	        PORT_SetError(SEC_ERROR_BAD_DER);
> 		return SECFailure;
> 	    }
>-	    dsasig = DSAU_DecodeDerSigToLen(sig, len);
>-	} else {
>-	    dsasig = DSAU_DecodeDerSig(sig);
> 	}

I believe that the outer "if (algid == ...) {" and the
matching "}" should be removed.
 
>+const SEC_ASN1Template hashParameterTemplate[] =

Add "static".

> static SECStatus
>-decodeSigAlg(SECOidTag alg, SECOidTag *hashalg)
>+decodeSigAlg(SECOidTag alg, SECItem *params, SECKEYPublicKey *key, 
>+	     SECOidTag *hashalg)
> {

Update the comment to document new parameters "params"
and "key".

>+    int len;

Add "unsigned".

>+	/* This is an EC algorithm. Recommended means the largest
>+	 * hash algorithm that is not reduced by the keysize of 

Change "reduced" to "truncated"?

>+	len = SECKEY_PublicKeyStrength(key);
>+	if (len < 32) { /* 32 bytes == 256 bits */
>+	    *hashalg = SEC_OID_SHA1;
>+	} else if (len < 48) { /* 48 bytes == 384 bits */
>+	    *hashalg = SEC_OID_SHA256;
>+	} else if (len < 64) { /* 48 bytes == 512 bits */
>+	    *hashalg = SEC_OID_SHA384;
>+	} else {
>+	    /* use the largest in this case */
>+	    *hashalg = SEC_OID_SHA512;
>+	}

You can use SECKEY_PublicKeyStrengthInBits instead.
If you make this change, please update the comments.

>@@ -288,7 +345,10 @@
> 	        cx->type = VFY_DSA;
> 		sigLen = DSA_SIGNATURE_LEN;
> 	    }
>-  	    cx->alg = SEC_OID_SHA1;
>+	    rv = decodeSigAlg(algid, params, key, &cx->alg);
>+	    if (rv != SECSuccess) {
>+		break;
>+	    }

Change "break" to "goto loser".  This is a bug.

>+vfy_VerifyDataPrivate(unsigned char *buf, int len, SECKEYPublicKey *key,
>+	       SECItem *sig, SECOidTag algid, SECItem *param, void *wincx)
> {
...
>-    cx = VFY_CreateContext(key, sig, algid, wincx);
>+    cx = vfy_CreateContextPrivate(key, sig, algid, param, wincx);

For consistency, change "param" to "params".

3. lib/cryptohi/vfypriv.h

>+SECStatus vfy_VerifyDataPrivate(unsigned char *buf, int len, 
>+	SECKEYPublicKey *key, SECItem *sig, SECOidTag algid, 
>+	SECItem *param, void *wincx);

For consistency, change "param" to "params".

Observation, not suggested change: "SECOidTag algid"
and "SECItem *param" are closely related.  It seems
that they should ideally be combined into a
"SECAlgorithmID *alg" parameter.

4. lib/freebl/ec.c

In ECDSA_SignDigestWithSeed, there are two instances
of "SHA1(M)" that need to be updated.

In ECDSA_VerifyDigest, there are two instances of "SHA1(M')"
that need to be updated.

5. lib/util/secoid.c

>     OD( cmsESDH, SEC_OID_CMS_EPHEMERAL_STATIC_DIFFIE_HELLMAN,
>-        "Ephemeral-Static Diffie-Hellman", CKM_INVALID_MECHANISM /* XXX */,
>+        "Ephemeral-Static Diffie-Hellman", CKM_X9_42_DH_DERIVE,
>         INVALID_CERT_EXTENSION ),

This change is unrelated to this bug.

>+    /* more ECC Signature Oids */
>+    OD( ansix962SignatureRecommended,
>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST,
>+	"X9.62 ECDSA signature with recommended digest", CKM_INVALID_MECHANISM,
>+	INVALID_CERT_EXTENSION ),
>+    OD( ansix962SignatureSpecified,
>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST,
>+	"X9.62 ECDSA signature with specified digest", CKM_ECDSA,
>+	INVALID_CERT_EXTENSION ),
>+    OD( ansix962SignaturewithSHA224Digest,
>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA224_DIGEST,
>+	"X9.62 ECDSA signature with SHA224", CKM_INVALID_MECHANISM,
>+	INVALID_CERT_EXTENSION ),
>+    OD( ansix962SignaturewithSHA256Digest,
>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA256_DIGEST,
>+	"X9.62 ECDSA signature with SHA256", CKM_INVALID_MECHANISM,
>+	INVALID_CERT_EXTENSION ),
>+    OD( ansix962SignaturewithSHA384Digest,
>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA384_DIGEST,
>+	"X9.62 ECDSA signature with SHA384", CKM_INVALID_MECHANISM,
>+	INVALID_CERT_EXTENSION ),
>+    OD( ansix962SignaturewithSHA512Digest,
>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA512_DIGEST,
>+	"X9.62 ECDSA signature with SHA512", CKM_INVALID_MECHANISM,
>+	INVALID_CERT_EXTENSION ),
> };

Why don't you use CKM_ECDSA for all of these?

Comment 17

[Wan-Teh Chang]

Comment on attachment 206556 [details] [diff] [review]
NSS 3.11 version of this patch

Bob, my suggested change of "break" to "goto loser"
in lib/cryptohi/secvfy.c is wrong.  Your code is correct.

Comment 18

[Robert Relyea]

>>+    /* more ECC Signature Oids */
>>+    OD( ansix962SignatureRecommended,
>>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST,
>>+	"X9.62 ECDSA signature with recommended digest", CKM_INVALID_MECHANISM,
>>+	INVALID_CERT_EXTENSION ),
>>+    OD( ansix962SignatureSpecified,
>>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST,
>>+	"X9.62 ECDSA signature with specified digest", CKM_ECDSA,
>>+	INVALID_CERT_EXTENSION ),
>>+    OD( ansix962SignaturewithSHA224Digest,
>>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA224_DIGEST,
>>+	"X9.62 ECDSA signature with SHA224", CKM_INVALID_MECHANISM,
>>+	INVALID_CERT_EXTENSION ),
>>+    OD( ansix962SignaturewithSHA256Digest,
>>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA256_DIGEST,
>>+	"X9.62 ECDSA signature with SHA256", CKM_INVALID_MECHANISM,
>>+	INVALID_CERT_EXTENSION ),
>>+    OD( ansix962SignaturewithSHA384Digest,
>>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA384_DIGEST,
>>+	"X9.62 ECDSA signature with SHA384", CKM_INVALID_MECHANISM,
>>+	INVALID_CERT_EXTENSION ),
>>+    OD( ansix962SignaturewithSHA512Digest,
>>+	SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA512_DIGEST,
>>+	"X9.62 ECDSA signature with SHA512", CKM_INVALID_MECHANISM,
>>+	INVALID_CERT_EXTENSION ),
>> };
>
> Why don't you use CKM_ECDSA for all of these?
>
I originally did, but CKM_ECDSA isn't really correct. Any code that tries to get the mechanism for these oids will expect to get the combined hashing/signing mechanism, which are not yet defined in the PKCS #11 spec.

Also I've noticed that there really needs to be a one-for-one mapping between the oid and the PKCS #11 mechanism, since there are cases where the oid is generated from the mechanism value as well as the mechanism from the oid.

bob

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: review comments on above patches

This attachment contains review comments for both 3.11 and 3.12 patches
above.  These are my complete comments for the 3.11 patch.  I may also
have some more comments for the 3.12 patch soon.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206556 [details] [diff] [review]
NSS 3.11 version of this patch

See review comments in attachment.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206655 [details] [diff] [review]
NSS 3.12 version of this patch

Additional review comments for the 3.12 patch:

>Index: lib/cryptohi/secvfy.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v


>+      case SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST:
[...]
>+	rv = SEC_QuickDERDecodeItem(arena, &oid, hashParameterTemplate, param);
>+	if (rv != SECSuccess) {
>+	    PORT_FreeArena(arena, PR_FALSE);
>+	    return rv;
>+	}
>+	*hashalg = SECOID_FindOIDTag(&oid);
>+	break;

This break will cause this function to return SECSuccess, 
even if *hashalg is SEC_OID_UNKNOWN.  This function should always return
SECFailure if *hashalg is not a valid hash algorithm tag.  That includes
not only SEC_OID_UNKNOWN, but also other OID tags that are not hash alg tags.

Since this same problem seems to occur in many places, I suggest a new 
function, SECOID_FindHashAlgTag(&oid), that returns only a valid hash 
alg tag or SEC_OID_UNKNOWN.  That simplifies the testing needed here.


>Index: lib/pkcs7/p7decode.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v
>retrieving revision 1.20
>diff -u -r1.20 p7decode.c
>--- lib/pkcs7/p7decode.c	3 Oct 2005 22:01:56 -0000	1.20
>+++ lib/pkcs7/p7decode.c	22 Dec 2005 22:38:04 -0000

>@@ -1615,10 +1615,10 @@
>     /*
>      * Find and confirm digest algorithm.
>      */
>-    algiddata = SECOID_FindOID (&(signerinfo->digestAlg.algorithm));
>+    digestTag = SECOID_FindOIDTag(&(signerinfo->digestAlg.algorithm));
> 
>     if (detached_digest != NULL) {
>-	HASH_HashType found_type = HASH_GetHashTypeByOidTag(algiddata->offset);
>+	HASH_HashType found_type = HASH_GetHashTypeByOidTag(digestTag);
> 	unsigned int hashLen     = HASH_ResultLen(digest_type);
> 
> 	if (digest_type != found_type || 
>@@ -1638,12 +1638,12 @@
> 	/*
> 	 * pick digest matching signerinfo->digestAlg from digests
> 	 */
>-	if (algiddata == NULL) {
>+	if (digestTag == SEC_OID_UNKNOWN) {
> 	    PORT_SetError (SEC_ERROR_PKCS7_BAD_SIGNATURE);
> 	    goto done;
> 	}

I think this code is not in the right order.  This test for digestTag
should happen immediately after the call to SECOID_FindOIDTag above,
before passing it as an argument to HASH_GetHashTypeByOidTag, rather 
than afterwords, I think.  

>-    algiddata = SECOID_FindOID (&(signerinfo->digestEncAlg.algorithm));
>-    if (algiddata == NULL ||
>-	((algiddata->offset != SEC_OID_PKCS1_RSA_ENCRYPTION) &&
>-	 (algiddata->offset != SEC_OID_ANSIX962_EC_PUBLIC_KEY) &&
>-	 (algiddata->offset != SEC_OID_ANSIX9_DSA_SIGNATURE))) {
>+    encTag = SECOID_FindOIDTag(&(signerinfo->digestEncAlg.algorithm));
>+    if (encTag == SEC_OID_UNKNOWN) {
> 	PORT_SetError (SEC_ERROR_PKCS7_BAD_SIGNATURE);
> 	goto done;
>     }

The old code checked the OIDtag for being one of the valid signature 
alg tags.  The new code only checks that it is a known OID.  
Shouldn't the new code continue to check that the OID tag is a valid
tag in this context?  
Should we be calling SEC_GetSignatureAlgorithmOidTag here, instead of
merely SECOID_FindOIDTag?

>Index: lib/pkcs7/p7encode.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pkcs7/p7encode.c,v
>retrieving revision 1.11
>diff -u -r1.11 p7encode.c
>--- lib/pkcs7/p7encode.c	2 Sep 2005 01:24:56 -0000	1.11
>+++ lib/pkcs7/p7encode.c	22 Dec 2005 22:38:04 -0000

>@@ -1050,11 +1001,12 @@
> 		    return SECFailure;
> 		}
> 
>+	        algid = SEC_GetSignatureAlgorithmOidTag(privkey->keyType,
>+ 							digestalgtag);

Should we check that algid is not SEC_OID_UNKNOWN before calling SEC_SignData?

> 		rv = SEC_SignData (&signature,
> 				   encoded_attrs.data, encoded_attrs.len,
> 				   privkey,
>-				   sec_pkcs7_pick_sign_alg (digestalgtag,
>-							    signalgtag));
>+				   algid);
> 		SECITEM_FreeItem (&encoded_attrs, PR_FALSE);
> 	    } else {


>Index: lib/smime/cmssiginfo.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/smime/cmssiginfo.c,v
>retrieving revision 1.29
>diff -u -r1.29 cmssiginfo.c
>--- lib/smime/cmssiginfo.c	2 Sep 2005 01:24:56 -0000	1.29
>+++ lib/smime/cmssiginfo.c	22 Dec 2005 22:38:05 -0000
>@@ -262,7 +262,8 @@
> 	                &encoded_attrs) == NULL)
> 	    goto loser;
> 
>-	signAlgTag = NSS_CMSUtil_MakeSignatureAlgorithm(digestalgtag, pubkAlgTag);
>+	signAlgTag = SEC_GetSignatureAlgorithmOidTag(privkey->keyType, 
>+                                                     digestalgtag);

Same issue, check for SEC_OID_UNKNOWN, and don't call SignData if so.

> 	rv = SEC_SignData(&signature, encoded_attrs.data, encoded_attrs.len, 
> 	                  privkey, signAlgTag);
> 	PORT_FreeArena(tmppoolp, PR_FALSE); /* awkward memory management :-( */

Comment 22

[Wan-Teh Chang]

Comment on attachment 208458 [details]
review comments on above patches

I suggest shortening ANSIX962_ECDSA_SIGNATURE
to ECDSA.

Comment 23

[Robert Relyea]

Attachment: Comments on review patches (3.11 only)

Replies to nelson's comments.

Comment 24

[Robert Relyea]

ec.c portions checked in...

Checking in ec.c;
/cvsroot/mozilla/security/nss/lib/freebl/ec.c,v  <--  ec.c
new revision: 1.9.2.2; previous revision: 1.9.2.1
done
Checking in ec.c;
/cvsroot/mozilla/security/nss/lib/freebl/ec.c,v  <--  ec.c
new revision: 1.11; previous revision: 1.10
done

Comment 25

[Robert Relyea]

Attachment: Incorporate WTC and Nelson's comments. 3.11 version

Some notes: I did not pick up the 3.12 version of decodeECorDSASignature() because the meaning of the paramters have changed between those 2 versions. The algid is the 'encAlgid' in 3.12 while its the combined enc/hash algid in 3.11.

Adding the 3.12 version of VFY_VerifyDataWithAlgorithmID created the following issues:
1) the 3.12 version optionally returns the hash oid to the caller. 3.11 does not have all the 3.12 changes, so returning the oid is not possible. Since in 3.11 this function is not exported, I simply made it an error to request the hash oid.
2) there are more consts propogated in this 3.11 patch than the previous patch.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 208799 [details] [diff] [review]
Incorporate WTC and Nelson's comments. 3.11 version

Thanks, Bob.  This is much clearer to me than the previous patch.  
I see there are several places where you had to "cast away const" in this patch.  
I'd prefer that you propagate the const declarations down to the called functions, since they too obviously must be using their args as constants.  But I understand you're trying to minimize change for 3.11.  
r=nelson

Comment 27

[Wan-Teh Chang]

Comment on attachment 208799 [details] [diff] [review]
Incorporate WTC and Nelson's comments. 3.11 version

r=wtc.  I didn't have time to read Nelson's review
comments on the old patch and Bob's response to them.
So some of my comments below may have already been
discussed.

I did notice one bug in our handling of
ecdsa-with-Recommended regarding SHA-224, one bug
with adding "ECC" to NSS_VERSION, and some
errors in comments.  I also want to suggest some
minor changes and express my opinion on the use
of const in this patch, which you don't need to
address.  I will mark my comments with what kind of
comment they are.  Please at least fix the bugs and
comment errors when you check this patch in.

1. lib/cryptohi/cryptohi.h

>+** Verify the signature on a block of data. The signature data is an RSA
>+** private key encrypted block of data formatted according to PKCS#1.

Comment error: The second sentence above is wrong.
I suggest simply removing that sentence.

>+**      "sig" the encrypted signature data

Comment error: this is related to the comment error above.
I suggest removing the word "encrypted".

2. lib/cryptohi/secsign.c

>+	case SEC_OID_UNKNOWN:	/* default for ECDSA if not specified */
>+	case SEC_OID_SHA1:

Comment error: change "ECDSA" to "SHA1" or "ECDSA with SHA1".

3. lib/cryptohi/secvfy.c

>+    case SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE:
>+    case SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE:
>+    case SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST:
>+    case SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE:

Nit: it would look nicer to list
SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE first.

> /*
>  * Pulls the hash algorithm, signing algorithm, and key type out of a
>  * composite algorithm.
>  *
>  * alg: the composite algorithm to dissect.
>  * hashalg: address of a SECOidTag which will be set with the hash algorithm.
>- * signalg: address of a SECOidTag which will be set with the signing alg.
>- *          (not implemented)
>- * keyType: address of a KeyType which will be set with the key type.
>- *          (not implemented)
>- * Returns: SECSuccess if the algorithm was acceptable, SECFailure if the
>+ * params:  specific signature parameter (from the signature AlgorithmID).
>+ * key:     public key to verify against.
>+ * Returns: SECSuccess if the alg algorithm was acceptable, SECFailure if the
>  *	algorithm was not found or was not a signing algorithm.
>  */
> static SECStatus
>-decodeSigAlg(SECOidTag alg, SECOidTag *hashalg)
>+decodeSigAlg(SECOidTag alg, const SECItem *params, const SECKEYPublicKey *key, 
>+	     SECOidTag *hashalg)

Borderline comment error/nit: the comment should
describe the function parameters in the same order
as they are listed in the function's prototype
(alg, params, key, hashalg).

>+      case SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST:
...
>+	len = SECKEY_PublicKeyStrength((SECKEYPublicKey *)key);
>+	if (len < 32) { /* 32 bytes == 256 bits */
>+	    *hashalg = SEC_OID_SHA1;
>+	} else if (len < 48) { /* 48 bytes == 384 bits */
>+	    *hashalg = SEC_OID_SHA256;
>+	} else if (len < 64) { /* 48 bytes == 512 bits */
>+	    *hashalg = SEC_OID_SHA384;
>+	} else {
>+	    /* use the largest in this case */
>+	    *hashalg = SEC_OID_SHA512;
>+	}
>+	break;

Bug: for ecdsa-with-Recommended, SHA224 should be used
when it's the longest hash not truncated by the EC key
size.  So, we should say:

        if (len < 28) { /* 28 bytes == 224 bits */
            *hashalg = SEC_OID_SHA1;
        } else if (len < 32) { /* 32 bytes == 256 bits */
            /* we don't implement SHA-224 hashes */
            return SECFailure;
        } else if (len < 48) { /* 48 bytes == 384 bits */
            *hashalg = SEC_OID_SHA256;
        ...

>+ * this function is private to nss3.dll in NSS 3.11

Nit: remove "to nss3.dll" or change it to "to the nss3
shared library".

>+SECStatus
>+VFY_VerifyDataWithAlgorithmID(const unsigned char *buf, int len,
>+                              const SECKEYPublicKey *key,
>+                              const SECItem *sig,
>+                              const SECAlgorithmID *sigAlgorithm,
>+                              SECOidTag *hash, void *wincx)
>+{
>+    /* the hash parameter is only provided to match the NSS 3.12 signature */
>+    PORT_Assert(hash == NULL);
>+    if (hash) {

Nit: change the four instances of "hash" to "reserved" to
match the parameter's name in the function declaration in
lib/cryptohi/cryptohi.h.

4. lib/nss/nss.h

>+#ifdef NSS_ENABLE_ECC
>+#define NSS_VERSION  "3.11.1 ECC Beta"
>+#else
> #define NSS_VERSION  "3.11.1 Beta"
>+#endif

Bug: Right now we add -DNSS_ENABLE_ECC in the directories
that need it, rather than in coreconf.  So this change
requires adding the following to the manifest.mn files
in cmd/signtool, lib/nss, and lib/smime:

ifdef NSS_ENABLE_ECC
DEFINES += -DNSS_ENABLE_ECC
endif

Alternatively, we can do this in coreconf/config.mk
and remove the above code from the manifest.mn files.  I
think this is the better long-term fix even though it may
touch more files.

6. lib/util/secoidt.h

>-    SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST = 201,
>+    SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE = 201,
>+
>+#define SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST \
>+	SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE

Nit: I suggest that you handle this as follows:

      SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE = 201,
      SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST = 201,  /* deprecated */
 
>+    /* new EC Signature oids */
>+    SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST = 275,
>+    SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST = 276,
>+    SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE = 277,
>+    SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE = 278,
>+    SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE = 279,
>+    SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE = 280,

Nit: seems that
SEC_OID_ANSIX962_ECDSA_RECOMMENDED_SIGNATURE and
SEC_OID_ANSIX962_ECDSA_SPECIFIED_SIGNATURE would match
the SEC_OID_ANSIX962_ECDSA_XXX_SIGNATURE pattern better.

Commentary: The new functions use const on function
parameters, which forced us to cast away the const in
several places.  Although it is a good idea to use
const on function parameters in general, these casts
make the good idea seem gratuitous in this patch.

Comment 28

[Robert Relyea]

NSS 3.11 checked in with wtc's comments.

Checking in coreconf/config.mk;
/cvsroot/mozilla/security/coreconf/config.mk,v  <--  config.mk
new revision: 1.17.28.1; previous revision: 1.17
done
Checking in nss/lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.44.10.1; previous revision: 1.44
done
Checking in nss/lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.21.2.1; previous revision: 1.21
done
Checking in nss/lib/cryptohi/cryptohi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/cryptohi.h,v  <--  cryptohi.h
new revision: 1.9.28.1; previous revision: 1.9
done
Checking in nss/lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.36.2.1; previous revision: 1.36
done
Checking in nss/lib/cryptohi/secsign.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v  <--  secsign.c
new revision: 1.14.2.1; previous revision: 1.14
done
Checking in nss/lib/cryptohi/secvfy.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v  <--  secvfy.c
new revision: 1.16.2.1; previous revision: 1.16
done
Checking in nss/lib/nss/nss.h;
/cvsroot/mozilla/security/nss/lib/nss/nss.h,v  <--  nss.h
new revision: 1.46.2.2; previous revision: 1.46.2.1
done
Checking in nss/lib/smime/cmsutil.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsutil.c,v  <--  cmsutil.c
new revision: 1.13.2.1; previous revision: 1.13
done
Checking in nss/lib/util/secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.31.28.1; previous revision: 1.31
done
Checking in nss/lib/util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.19.28.1; previous revision: 1.19
done

Comment 29

[Robert Relyea]

Attachment: Incorporate Nelson's comments to 3.12 version

Additional answers:

> 1. Bug in patch for trunk for cmd/checkcert/checkcert.c 
>>-	oiddata = SECOID_FindOIDByTag(cx->alg);
>>+	oiddata = SECOID_FindOIDByTag(hashAlgTag);
>
> With this patch, hashAlgTag is uninitialized here.

Initialzied in the following line:
+    cx = VFY_CreateContextWithAlgorithmID(key, sig, sigAlgorithm, &hashAlgTag, 
+                                          NULL);

> Since this same problem seems to occur in many places, I suggest a new 
> function, SECOID_FindHashAlgTag(&oid), that returns only a valid hash 
> alg tag or SEC_OID_UNKNOWN.  That simplifies the testing needed here.

There were only a couple of tests, and not all of them wanted to find the hashAlgTag (some had the hashAlgTag. some wanted the HASH_HashType), So I used HASH_GetHashTypeByOidTag to determine if the Oid was a valid hash oid. 
the existing function returns HASH_NULLAlg and sets and error code if the hash oid wasn't valid. This also restricts the knowledge of which oids are hash oids to sechash.c where it belongs.

>The old code checked the OIDtag for being one of the valid signature 
>alg tags.  The new code only checks that it is a known OID.  
>Shouldn't the new code continue to check that the OID tag is a valid
>tag in this context?  

No, we've moved that knowledge into the Verify and sign functions (to simplify adding new signing algorithms in the future. The checks were there because the old versions of the hash and sign functions did not properly validate the oids.

Comment 30

[Robert Relyea]

Attachment: Full version of the NSS 3.12 patch

Comment 31

[Robert Relyea]

Attachment: NSS 3.12 patch

Previous patch was missing lib/smime/cmssiginfo.c

Comment 32

[Robert Relyea]

Preserving the diff line used to generate NSS 3.12 patch to assist a more accurate checkin.

cvs diff -uN cmd/certcgi cmd/checkcert cmd/sigtool lib/certhigh lib/cryptohi lib/nss lib/pkcs7 lib/smime/cms.h lib/smime/cmsutil.c lib/smime/cmssigninfo.c 

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210490 [details] [diff] [review]
NSS 3.12 patch

Bob, Your comment about the most recent patch says that it includes
the changes to file nss/lib/smime/cmssiginfo.c, but it actually does not.
The new command, added in commment 32, tries to pick up 
lib/smime/cmssigninfo.c which doesn't exist.
                ^ extra letter

Your latest attachment (attachment 210490 [details] [diff] [review]) is still is not as complete 
as the patch I previously reviewed (attachment 206655 [details] [diff] [review]).  Attachment 
206655 patched several files that are not patched by 210490, including:

nss/lib/freebl/ec.c             (this was checked in already, so OK)
nss/lib/smime/cmssiginfo.c      
nss/lib/util/secoid.c           
nss/lib/util/secoidt.h          
nss/cmd/crmf-cgi/crmfcgi.c      
nss/cmd/signtool/certgen.c      

The patches for those last 5 files listed above seem vital, and so 
seem to be missing from the latest patch.

The new attachment 210490 [details] [diff] [review] patches two files not patched by 206655:
nss/lib/cryptohi/dsautil.c       
nss/lib/cryptohi/keyhi.h         
These are both additional const propagation, and seem fine.  

So, I conclude that attachment 210490 [details] [diff] [review] is still missing changes to 5 
files.  Please attach a complete patch.

Comment 34

[Robert Relyea]

Attachment: Sigh, This one should have everything.

the previous patch missed because my change to cmssiginfo really changed signtool to sigtool. It was missing crmf-cgi and lib/util (the latter was critical).

Current cvs command to help final checkin:

cvs diff -uN cmd/certcgi cmd/checkcert cmd/crmf-cgi cmd/signtool lib/certhigh lib/cryptohi lib/nss lib/pkcs7 lib/util lib/smime/cms.h lib/smime/cmsutil.c lib/smime/cmssiginfo.c

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210901 [details] [diff] [review]
Sigh, This one should have everything.

Bob,
One of your patches modifies a function that leaks memory in many of its
error paths.  Your patch adds yet another error path that leaks.  
Since that leak was pre-existing, I'm going to give this patch r=nelson, 
with the request that you fix the leaks in another separate patch after 
checking this one in.  (Please don't make me review this huge patch again :)

The leak is in NSS_CMSSignerInfo_Sign() in  lib/smime/cmssiginfo.c at line 
http://lxr.mozilla.org/mozilla/source/security/nss/lib/smime/cmssiginfo.c#239

The code allocates a PL_ArenaPool named tmppoolp, to hold the output of the
subsequent call to NSS_CMSAttributeArray_Encode().  That allocation is followed
by several error checks, each of which does "goto loser" on failure.  The leak
occurs because none of these goto loser paths frees the tmppoolp arenapool. 
Your patch adds yet another error path that goes to loser without freeing the
arena.  

>@@ -262,7 +262,13 @@
> 	                &encoded_attrs) == NULL)
> 	    goto loser;

Here's a leak (the second one):

>-	signAlgTag = NSS_CMSUtil_MakeSignatureAlgorithm(digestalgtag, pubkAlgTag);
>+	signAlgTag = SEC_GetSignatureAlgorithmOidTag(privkey->keyType, 
>+                                                     digestalgtag);
>+	if (signAlgTag == SEC_OID_UNKNOWN) {
>+	    PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
>+	    goto loser;
>+	}

here's the new third leak.

> 	rv = SEC_SignData(&signature, encoded_attrs.data, encoded_attrs.len, 
> 	                  privkey, signAlgTag);
> 	PORT_FreeArena(tmppoolp, PR_FALSE); /* awkward memory management :-( */

Here's the only place where tmppoolp is freed.

i'd suggest adding a new label (e.g. "poolLoser") just before lable loser, 
that frees tmppoolp and then falls through into loser.  Then change the 3
leaking goto's to goto poolLoser.

Comment 36

[Robert Relyea]

Attachment: fix tmppoolp memory leak

Thanks for the review, and the catch.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 211202 [details] [diff] [review]
fix tmppoolp memory leak

r=nelson.bolyard

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Bug 320581 seems to describe this same problem, but only for two specific
functions, ECDSA_SignDigestWithSeed and ECDSA_VerifyDigest .
It seems to fall under the description "NSS ECDSA can only sign SHA-1".

So, my questions are:
a) is bug 320581 a duplicate of this bug?
b) Does it block this bug?
c) Is this bug intended to cover a specific set of NSS functions that 
   does not include functions ECDSA_SignDigestWithSeed and ECDSA_VerifyDigest ?

Comment 39

[Wan-Teh Chang]

*** Bug 320581 has been marked as a duplicate of this bug. ***

Comment 40

[Robert Relyea]

checked in.