657379 - NSS uses the wrong OID for signatureAlgorithm field of signerInfo in CMS for DSA and ECDSA

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[David Cooper]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Moziila/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110516 Lanikai/3.1.10

When Thunderbird is used to sign an email using DSA or ECDSA, the signatureAlgorithm field of signerInfo is either dsa (1.2.840.10040.4.1) or ecPublicKey (1.2.840.10045.2.1).  That is the OID identifies the type of key used to sign the message rather than the signature algorithm.  This is appropriate when the message is signed using RSA PKCS #1 v1.5 since RFC 3370 says:

   The rsaEncryption algorithm identifier is used to identify RSA (PKCS
   #1 v1.5) signature values regardless of the message digest algorithm
   employed.  CMS implementations that include the RSA (PKCS #1 v1.5)
   signature algorithm MUST support the rsaEncryption signature value
   algorithm identifier, and CMS implementations MAY support RSA (PKCS
   #1 v1.5) signature value algorithm identifiers that specify both the
   RSA (PKCS #1 v1.5) signature algorithm and the message digest
   algorithm.

However, for DSA and ECDSA the signatureAlgorithm field is supposed to use an OID that specifies both the signature algorithm and the message digest algorithm.  This is specified in RFC 3370 for DSA with SHA-1, RFC 5754 for DSA with SHA-224 and DSA with SHA-256, and RFC 5753 for ECDSA.  Section 2.1.1 of RFC 5753 says:

   When using ECDSA with SignedData, the fields of SignerInfo are as in
   [RFC 5662], but with the following restrictions:

   -  signatureAlgorithm contains the signature algorithm identifier
      (see Section 7.1.3): ecdsa-with-SHA1, ecdsa-with-SHA224, ecdsa-
      with-SHA256, ecdsa-with-SHA384, or ecdsa-with-SHA512.  The hash
      algorithm identified in the name of the signature algorithm MUST
      be the same as the digestAlgorithm (e.g., digestAlgorithm is id-
      sha256 therefore signatureAlgorithm is ecdsa-with-SHA256).

When verifying signatures on emails signed using DSA or ECDSA, Thunderbird only checks for the dsa (1.2.840.10040.4.1) or ecPublicKey (1.2.840.10045.2.1) OID and so cannot verify signatures that specify the correct OID.

Reproducible: Always

Steps to Reproduce:
1. Use OpenSSL to sign an email using a DSA key
2. Open email in Thunderbird
3. Check whether signature verification was successful

Comment 1

[Ludovic Hirlimann [:Usul]]

Can you attach a testcase ?

Comment 2

[David Cooper]

Attachment: Email signed with OpenSSL version 1.0.0d using DSA with SHA-1

In this email, signatureAlgorithm is dsaWithSha1 (1.2.840.10040.4.3).

Comment 3

[David Cooper]

Attachment: Email signed with Thunderbird 3.1.10 using DSA with SHA-1

In this email, signatureAlgorithm is dsa (1.2.840.10040.4.1).

Comment 4

[David Cooper]

Attachment: Patch to allow NSS to verify DSA and ECDSA CMS signatures identified using correct OIDs.

With this patch NSS (and Thunderbird) can verify DSA and ECDSA signatures on CMS messages created by other applications that identify the signatureAlgorithm using the correct OID (e.g., dsaWithSha1, ecdsaWithSHA256).  This patch does not change the OID that NSS (and Thunderbird) places in the signatureAlgorithm field when signing CMS messages using DSA or ECDSA.

Comment 5

[David Cooper]

Attachment: Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

I created two separate patches since changing the OID that NSS uses when signing CMS messages will not be backwards compatible with older versions of NSS.  If only the first patch is applied then NSS will be able to verify messages signed by other applications as well as by older versions of NSS.  However, if the second patch is not applied then some applications that don't use NSS may not accept CMS messages signed by NSS using DSA or ECDSA.

Comment 6

[David Cooper]

I forgot to mention that both patches were created against NSS version 3.12.10.

Comment 7

[Ludovic Hirlimann [:Usul]]

Comment on attachment 533021 [details] [diff] [review]
Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

Splitting review between nelson and kaie

Comment 8

[Ludovic Hirlimann [:Usul]]

David you might want to produce your patches against cvs trunk and re-request reviews.

Comment 9

[David Cooper]

Attachment: Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

This is the same as attachment 533019 [details] [diff] [review], but this patch will work against cvs HEAD.

Comment 10

[David Cooper]

Attachment: Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messages using DSA or ECDSA.

This is the same as attachment 533021 [details] [diff] [review], but this patch will work against cvs HEAD.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 533019 [details] [diff] [review]
Patch to allow NSS to verify DSA and ECDSA CMS signatures identified using correct OIDs.

This patch appears to be identical to the newer one with the same name, so I'm marking this one as obsolete.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 533021 [details] [diff] [review]
Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

This patch appears to be identical to the newer one with the same name, so I'm marking this one as obsolete.

Comment 13

[Robert Relyea]

Comment on attachment 533406 [details] [diff] [review]
Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

r+ rrelyea

Comment 14

[Robert Relyea]

Comment on attachment 533407 [details] [diff] [review]
Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messages using DSA or ECDSA.

First, thank you for running this down and submitting the patches.

r-

The code is right, but we want to collect all these mapping knowledge into a much smaller area.

I suggest adding a helper function to  secsign.c which takes the input tag and the digest tag and automatically produces the right signature out. This can replace the obsolete PK11_FortezzaMapSign() function. and can use the existing SEC_GetSignatureAlgorithmOidTag.

Doing it this way would let us add support for new asymmetric algorithms more easily.

bob

Comment 15

[David Cooper]

Attachment: Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

This version of the patch should work exactly the same as the previous version, but I simplified the code in secvfy.c.

Comment 16

[David Cooper]

Attachment: Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

I created a new function in secsign.c, SEC_GetSignerInfoSignatureAlgorithmOidTag, that takes a certificate and a hash algorithm and returns the correct OID for the signatureAlgorithm field of SignerInfo.  The call in p7encode.c to this new function replaces the call to PK11_FortezzaMapSig and also addresses the comment that there should be a cert-level interface so that the caller does not need to know the internal details of the certificate data structure.

Comment 17

[Wan-Teh Chang]

Comment on attachment 546599 [details] [diff] [review]
Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

David, thank you for the patch.

In mozilla/security/nss/lib/cryptohi/seckey.c

>       case SEC_OID_ANSIX962_EC_PUBLIC_KEY:
>+      case SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE:
>+      case SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE:
>+      case SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE:
>+      case SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE:
>+      case SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE:
>+      case SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST:
>+      case SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST:
> 	keyType = ecKey;
> 	break;

Please list the SHA-2 algorithms in this order: SHA1, SHA224, SHA256, SHA385, SHA512.

Comment 18

[Robert Relyea]

Comment on attachment 546599 [details] [diff] [review]
Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

r+ rrelyea

Comment 19

[Robert Relyea]

Comment on attachment 546606 [details] [diff] [review]
Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

r+, though I would like you to try something.

Instead of your patch for cmssigninfo. try replacing the PK11_FortezzaMapSig() with SEC_GetSignerInfoSignatureAlgorithmOidTag(). 

bob

Comment 20

[Ludovic Hirlimann [:Usul]]

Robert , Wan-teh does david need to provide new patches to address comment 17 and comment 19 ?

Comment 21

[David Cooper]

Attachment: Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

(In reply to comment #17)
> Comment on attachment 546599 [details] [diff] [review] [review]
> Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.
> 
> David, thank you for the patch.
> 
> In mozilla/security/nss/lib/cryptohi/seckey.c
> 
> >       case SEC_OID_ANSIX962_EC_PUBLIC_KEY:
> >+      case SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE:
> >+      case SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE:
> >+      case SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE:
> >+      case SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE:
> >+      case SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE:
> >+      case SEC_OID_ANSIX962_ECDSA_SIGNATURE_RECOMMENDED_DIGEST:
> >+      case SEC_OID_ANSIX962_ECDSA_SIGNATURE_SPECIFIED_DIGEST:
> > 	keyType = ecKey;
> > 	break;
> 
> Please list the SHA-2 algorithms in this order: SHA1, SHA224, SHA256,
> SHA385, SHA512.

Done.

Comment 22

[David Cooper]

Attachment: Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

(In reply to comment #19)
> Comment on attachment 546606 [details] [diff] [review] [review]
> Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS
> messasges using DSA or ECDSA
> 
> r+, though I would like you to try something.
> 
> Instead of your patch for cmssigninfo. try replacing the
> PK11_FortezzaMapSig() with SEC_GetSignerInfoSignatureAlgorithmOidTag(). 
> 
> bob

Done.

Comment 23

[Robert Relyea]

Comment on attachment 546794 [details] [diff] [review]
Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

r+ rrelyea

Thanks for the patch and the changes.

Comment 24

[Robert Relyea]

Comment on attachment 546792 [details] [diff] [review]
Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

r+ rrelyea

Comment 25

[Wan-Teh Chang]

Comment on attachment 546792 [details] [diff] [review]
Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.

David, I have one concern about the second file in this patch.

In mozilla/security/nss/lib/cryptohi/secvfy.c, function vfy_CreateContext:

> 	case dsaKey:
> 	case ecKey:
>+	    if (type == dsaKey)
>+		cx->encAlg = encAlg = SEC_OID_ANSIX9_DSA_SIGNATURE;
>+	    else
>+		cx->encAlg = encAlg = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
>+
> 	    sigLen = SECKEY_SignatureLen(key);
> 	    if (sigLen == 0) {
> 		/* error set by SECKEY_SignatureLen */
> 		rv = SECFailure;	
> 		break;
> 	    }
> 	    rv = decodeECorDSASignature(encAlg, sig, cx->u.buffer, sigLen);
>  	    break;

Please add a comment to explain why for dsaKey and ecKey we need to
completely ignore the passed-in value of the encAlg argument and use
SEC_OID_ANSIX9_DSA_SIGNATURE and SEC_OID_ANSIX962_EC_PUBLIC_KEY instead.

It seems better to fix the callers of vfy_CreateContext to pass in the
appropriate value of encAlg.  Do you agree?

I traced the NSS code.  It seems that the encAlg argument comes from
these two functions: sec_DecodeSigAlg and NSS_CMSSignerInfo_Verify.
Perhaps we can make those two functions do the right thing.

Also note this code in NSS_CMSSignerInfo_Verify:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/smime/cmssiginfo.c&rev=1.34&mark=382-383,389-394#382

Comment 26

[Wan-Teh Chang]

Comment on attachment 546794 [details] [diff] [review]
Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS messasges using DSA or ECDSA

In mozilla/security/nss/lib/cryptohi/secsign.c:

>+SECOidTag
>+SEC_GetSignerInfoSignatureAlgorithmOidTag(SECAlgorithmID *algID,
>+						SECOidTag hashAlgTag)

The function parameters don't look aligned.

>+    switch (algTag) {
>+    case SEC_OID_ANSIX9_DSA_SIGNATURE:
>+    case SEC_OID_MISSI_KEA_DSS:
>+    case SEC_OID_MISSI_DSS:
>+    case SEC_OID_MISSI_DSS_OLD:
>+    case SEC_OID_MISSI_KEA_DSS_OLD:
>+    case SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST:
>+	return SEC_GetSignatureAlgorithmOidTag(dsaKey, hashAlgTag);
>+    case SEC_OID_ANSIX962_EC_PUBLIC_KEY:
>+	return SEC_GetSignatureAlgorithmOidTag(ecKey, hashAlgTag);
>+    default:
>+	return algTag;
>+	break;
>+    }

Please remove the SEC_OID_MISSI_* cases.

I wonder if the inverse of SEC_GetSignatureAlgorithmOidTag (the
inverse of the mapping above) could help obtain the right encAlg
value for NSS_CMSSignerInfo_Verify or vfy_CreateContext.

Comment 27

[Kai Engert (:KaiE:)]

If this patch is intended for 3.12.11, then the patch must be modified.

Files nss.def must be changed to add the new function in version 3.12.11, not 3.13

I can do that prior to checkin.

Comment 28

[Kai Engert (:KaiE:)]

Wan-Teh, I had intended to checkin the patches, because they already have r+.

Please advice if you want us to (a) checkin and follow-up, or (b) delay until your requests are addressed.

Comment 29

[Kai Engert (:KaiE:)]

Attachment: Patch v6

FWIW, this patch combines the ones that have r+, plus the fix for nss.def

(Why does nss.def list 3.13 prior to 3.12.10 ? Should we reorder that ? )

Comment 30

[David Cooper]

(In reply to comment #25)
> Comment on attachment 546792 [details] [diff] [review] [review]
> Patch to allow NSS to verify DSA and ECDSA CMS signatures using correct OIDs.
> 
> David, I have one concern about the second file in this patch.
> 
> In mozilla/security/nss/lib/cryptohi/secvfy.c, function vfy_CreateContext:
> 
> > 	case dsaKey:
> > 	case ecKey:
> >+	    if (type == dsaKey)
> >+		cx->encAlg = encAlg = SEC_OID_ANSIX9_DSA_SIGNATURE;
> >+	    else
> >+		cx->encAlg = encAlg = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
> >+
> > 	    sigLen = SECKEY_SignatureLen(key);
> > 	    if (sigLen == 0) {
> > 		/* error set by SECKEY_SignatureLen */
> > 		rv = SECFailure;	
> > 		break;
> > 	    }
> > 	    rv = decodeECorDSASignature(encAlg, sig, cx->u.buffer, sigLen);
> >  	    break;
> 
> Please add a comment to explain why for dsaKey and ecKey we need to
> completely ignore the passed-in value of the encAlg argument and use
> SEC_OID_ANSIX9_DSA_SIGNATURE and SEC_OID_ANSIX962_EC_PUBLIC_KEY instead.
> 
> It seems better to fix the callers of vfy_CreateContext to pass in the
> appropriate value of encAlg.  Do you agree?
> 
> I traced the NSS code.  It seems that the encAlg argument comes from
> these two functions: sec_DecodeSigAlg and NSS_CMSSignerInfo_Verify.
> Perhaps we can make those two functions do the right thing.
> 
> Also note this code in NSS_CMSSignerInfo_Verify:
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/smime/
> cmssiginfo.c&rev=1.34&mark=382-383,389-394#382

The passed in value of the encAlg argument isn't being completely ignored.  vfy_CreateContext calls "type = seckey_GetKeyType(encAlg)" and then the code that I added is resetting the value of encAlg based on the value of type.  I only do this if type is dsaKey or ecKey since encAlg is used in decodeECorDSASignature but not DecryptSigBlock. [However, cx->encAlg is set to encAlg and vfy_CreateContext returns cx, and I did not do any tests to see what would happen if one tried to verify an RSA signature on a CMS message if the signatureAlgorithm OID were incorrectly set to something like SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION rather than SEC_OID_PKCS1_RSA_ENCRYPTION.]

It is true that an alternative solution would be to ensure that the value of encAlg passed to vfy_CreateContext were always the public key OID and not the signature algorithm OID.  At the very least this would involve changing NSS_CMSSignerInfo_Verify and sec_pkcs7_verify_signature.  I couldn't find any other places that would need to be changed, but then I couldn't find any calls to the function VFY_CreateContextDirect.

To me it seems easier to address the issue within vfy_CreateContext since vfy_CreateContext already calls seckey_GetKeyType and it is easy to reset encAlg to SEC_OID_ANSIX9_DSA_SIGNATURE or SEC_OID_ANSIX962_EC_PUBLIC_KEY (or SEC_OID_PKCS1_RSA_ENCRYPTION) based on the value of type.  However, I just identified the problem and presented one possible solution.  It is not up to me to decide how NSS implements this.

Comment 31

[David Cooper]

(In reply to comment #26)
> Comment on attachment 546794 [details] [diff] [review] [review]
> Patch to have NSS use correct signatureAlgorithm OIDs when signing CMS
> messasges using DSA or ECDSA
> 
> In mozilla/security/nss/lib/cryptohi/secsign.c:
> 
> >+SECOidTag
> >+SEC_GetSignerInfoSignatureAlgorithmOidTag(SECAlgorithmID *algID,
> >+						SECOidTag hashAlgTag)
> 
> The function parameters don't look aligned.
> 
> >+    switch (algTag) {
> >+    case SEC_OID_ANSIX9_DSA_SIGNATURE:
> >+    case SEC_OID_MISSI_KEA_DSS:
> >+    case SEC_OID_MISSI_DSS:
> >+    case SEC_OID_MISSI_DSS_OLD:
> >+    case SEC_OID_MISSI_KEA_DSS_OLD:
> >+    case SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST:
> >+	return SEC_GetSignatureAlgorithmOidTag(dsaKey, hashAlgTag);
> >+    case SEC_OID_ANSIX962_EC_PUBLIC_KEY:
> >+	return SEC_GetSignatureAlgorithmOidTag(ecKey, hashAlgTag);
> >+    default:
> >+	return algTag;
> >+	break;
> >+    }
> 
> Please remove the SEC_OID_MISSI_* cases.

Simply removing the SEC_OID_MISSI_* cases would be incorrect.  The calls to SEC_GetSignerInfoSignatureAlgorithmOidTag are replacing calls to PK11_FortezzaMapSig (below).  So, currently the SEC_OID_MISSI_* OIDs are being mapped to SEC_OID_ANSIX9_DSA_SIGNATURE.  My code just maps them to SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST instead. What do you believe the correct behavior should be?  Should SEC_GetSignerInfoSignatureAlgorithmOidTag map the SEC_OID_MISSI_* OIDs to SEC_OID_ANSIX9_DSA_SIGNATURE (as is currently done by the calls to PK11_FortezzaMapSig or should they be mapped to SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST?

    SECOidTag
    PK11_FortezzaMapSig(SECOidTag algTag)
    {
        switch (algTag) {
        case SEC_OID_MISSI_KEA_DSS:
        case SEC_OID_MISSI_DSS:
        case SEC_OID_MISSI_DSS_OLD:
        case SEC_OID_MISSI_KEA_DSS_OLD:
        case SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST:
            return SEC_OID_ANSIX9_DSA_SIGNATURE;
        default:
            break;
        }
        return algTag;
    }

Comment 32

[Wan-Teh Chang]

David: thank you for the replies.  I fully understand this bug now.

We are removing the obsolete Fortezza code, which is why I said
it's not necessary to carry the SEC_OID_MISSI_* cases to the new
function.

I'd like to spend more time to study the code and see if I can
come up with a better fix.  There are two things I find not ideal
in the current patch:

1. The new helper function SEC_GetSignerInfoSignatureAlgorithmOidTag
handles a CMS quirk but is added to the base NSS shared library
libnss3.so.  It should ideally be in libsmime3.so.

2. The function seckey_GetKeyType has this comment:

      /* accommodate applications that hand us a signature type when they
        * should be handing us a cipher type */

which seems to imply that the input SECOidTag tag is supposed to be
a public key algorithm as opposed to a signature algorithm.  So, adding
a bunch of signature algorithm OID tags to seckey_GetKeyType may
cause the function to be overly permissive in some cases.  A possible
solution is to create a new version of seckey_GetKeyType for mapping
signature algorithms to key types.

(The use of the 'encAlg' variable name, which comes from PKCS #7, is
a little confusing when working on CMS, which has replaced
digestEncryptionAlgorithm with signatureAlgorithm.  We should fix that
at some point.)

If I cannot get around to working on this bug in a few weeks, you guys
can go ahead and check in David's patch.

Comment 33

[Wan-Teh Chang]

Attachment: Patch v7

David, please test this patch.  Thanks.

Bob, please review this patch.

After studying the sign and verify functions in lib/cryptohi
carefully, I found that the VFY_VerifyXXXWithAlgorithmID
functions meet the requirement perfectly.  That allows me to
avoid changing seckey_GetKeyType function.

On the signing side, I added a function in lib/smime/cmssiginfo.c
to get the CMS SignerInfo signatureAlgorithm based on the key
type.  This avoids adding a CMS-specific helper function to the
base NSS library (libnss3.so).  The drawback is that lib/pkcs7
would need to duplicate this helper function.  But I think it is
fine to let the older lib/pkcs7 code support RSA only and not bother
changing it to support ECDSA.

Comment 34

[David Cooper]

(In reply to comment #33)
> Created attachment 549612 [details] [diff] [review] [diff] [details] [review]
> Patch v7
> 
> David, please test this patch.  Thanks.

> The drawback is that lib/pkcs7
> would need to duplicate this helper function.  But I think it is
> fine to let the older lib/pkcs7 code support RSA only and not bother
> changing it to support ECDSA.

I tested this patch in Thunderbird using RSA, DSA, and ECDSA and it seems to work correctly.  As you noted, though, it leaves programs that use the lib/pkcs7 code, such as the p7verify command, unable to unable to verify the signatures created using DSA or ECDSA.

Comment 35

[Wan-Teh Chang]

Removing [SMIME-ECC] from the status whiteboard.  This bug is
not specific to ECDSA.  This bug also applies to DSA and even
RSA in some CMS implementations.

Comment 36

[Robert Relyea]

Comment on attachment 549612 [details] [diff] [review]
Patch v7

I'm going to r+ this patch but highlight the following semantic changes this patch makes (in addition to working making ECC work correctly).

---1) The most important, after this patch we will no longer validate old ECC and DSA signatures that we incorrectly encoded.

 The difference between Verify_Direct and Verify_AlgorithmID is that Verify_Direct takes both a encoding algorthim and a hash, where Verify_AlgorithmID takes an oid that encodes both, which I'll call a signature oid (or a oid plus data that indicates which hash function is used). In general, we usually have signature oids, but the initial spec for CMS has separate encoding algorithm and hashing algorithm --- at least for RSA. When other signing algorithms were added, this difference from CMS/PKCS 7 and other signing operations were removed. That is essentially the purpose of this bug. The code Wan-Teh presents, handles all the specified cases. If the RSA 'encoding' algorithm is used, the Hash is set to SEC_OID_UNKNOWN internally. This is because RSA signatures are self-describing, and can figure out what the true hash is from the signature itself. wtc correctly verifies that the underlying hash was the same as the hash we were expecting (which is required for RSA). For non-RSA, a correctly encoded blob will include the and actual signature oid (well actually algorithm id), which itself describes the hashing function. The issue is if we try to decode some file that was signed by either ECC or DSA with the encoding alg rather than a signature alg. This code will now fail in the sec_DecodeSigAlg() function.

What we can do about this:
1) don't worry about these failures, presuming that DSA and ECC signed data from NSS is exceedingly rare.
2) go back to a code fragment like the one we had initially.
3) change VFY_VerifyDataAlgorithmWithAlgorithmID() to accept both a hash hint and the ECC and DSA encoding algorithms (simple change to sec_DecodeSigAlg())
4) on signature failure detect if we are in the ECC or DSA case and fall back to the VerifyDirect code.

My preference would be 1 or 3 first followed by 4 then 2.

Comment 37

[Ludovic Hirlimann [:Usul]]

Can we set the chekin-needed keyword or shall we wait for Wan-Teh's comments ?

Comment 38

[David Cooper]

Robert,

Thanks for catching the backward compatibility problem.  I think NSS does need to be able to verify DSA and ECDSA signatures that use the incorrect OIDs.  I think there is another option for fixing this though. NSS_CMSSignerInfo_Verify could compare the signatureAlgorithm OID to pubAlgTag.  If they are the same, then it would verify the signature using VFY_VerifyDataDirect and if they are different if would verify the signature using VFY_VerifyDataWithAlgorithmID (i.e., use the current code if signatureAlgorithm contains a public key OID, and use Wan-Teh's new code if signatureAlgorithm contains a signature algorithm OID).

(In reply to Robert Relyea from comment #36)
> Comment on attachment 549612 [details] [diff] [review]
> Patch v7
> 
> I'm going to r+ this patch but highlight the following semantic changes this
> patch makes (in addition to working making ECC work correctly).
> 
> ---1) The most important, after this patch we will no longer validate old
> ECC and DSA signatures that we incorrectly encoded.
> 
> 
> What we can do about this:
> 1) don't worry about these failures, presuming that DSA and ECC signed data
> from NSS is exceedingly rare.
> 2) go back to a code fragment like the one we had initially.
> 3) change VFY_VerifyDataAlgorithmWithAlgorithmID() to accept both a hash
> hint and the ECC and DSA encoding algorithms (simple change to
> sec_DecodeSigAlg())
> 4) on signature failure detect if we are in the ECC or DSA case and fall
> back to the VerifyDirect code.
> 
> My preference would be 1 or 3 first followed by 4 then 2.

Comment 39

[Robert Relyea]

That sounds like a semantically preferable version of option 4, which raises it in my mind to one of the more preferable solutions.

bob

Comment 40

[David Cooper]

Comment on attachment 549612 [details] [diff] [review]
Patch v7

Review of attachment 549612 [details] [diff] [review]:
-----------------------------------------------------------------

Based on Robert's feedback, I'm going to propose a slight modification to the patch so that verification of signatures that specify the public key OID for DSA or ECDSA will verify.

Comment 41

[David Cooper]

Attachment: Patch v8

As described in Comment 38, this patch uses the current code if the signatureAlgorithm OID is the same as the OID in the subjectPublicKeyInfo field of the certificate and uses the code from Patch v7 if the OIDs are different.

Wan-Teh, are you okay with this change or would you prefer a different approach to address Comment 36?

Comment 42

[Robert Relyea]

Comment on attachment 623316 [details] [diff] [review]
Patch v8

r+ rrelyea I'm fine with this change.

wtc if you are OK with it, feel free to check it in.

bob

Comment 43

[David Cooper]

Attachment: Patch v8 updated

This is the same as Patch v8, just updated to work with the current NSS code.

Comment 44

[David Cooper]

Attachment: Patch v8 updated

This is a patch that was developed several years ago, but was never checked in. I didn't make any changes to the patch, just updated it so that it can be applied to the current NSS code.

Comment 45

[Kai Engert (:KaiE:)]

Comment on attachment 9090720 [details] [diff] [review]
Patch v8 updated

David, thanks for following up. It's a pity this wasn't checked in 7 years ago. 

I compared the patch reviewed by Bob with your latest patch, and the only differences are in context.

I don't think this needs another review. I'll check this in now.

Comment 46

[Kai Engert (:KaiE:)]

try build running: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=563417e81754774e51e305eaef7bdcbc47c02874

Comment 47

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/7a83b248de3038b2cf0031cb89733fbb90f59142