Closed Bug 1409867 Opened 7 years ago Closed 6 years ago

org.mozilla.jss.pkix.cms.SignerInfo incorrectly producing signatures (especially for EC)

Categories

(JSS Graveyard :: Library, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: david.konrad.stutzman, Assigned: cfu)

Details

Attachments

(1 file, 5 obsolete files)

Attached file SignerInfo-patch.txt (obsolete) —
This class appears to generate signatures with a different algorithm and adds the wrong signatureAlgorithm than the user of the class passes into the constructor.

The constructor takes in a SignatureAlgorithm.  One would expect that same algorithm to appear in the signatureAlgorithm field of the SignerInfo.  That is not presently the case.  The signatureAlgorithm field ends up populated with the "raw" key algorithm and the signature is calculated with that same algorithm.

Currently in JSS, the encoding of the signatureAlgorithm and signature fields of a JSS SignerInfo where I passed in SignatureAlgorithm.ECSignatureWithSHA256Digest results in the following:
212  11: . SEQUENCE {
214   7: . . OBJECT IDENTIFIER ecPublicKey (1 2 840 10045 2 1)
       : . . . (ANSI X9.62 public key type)
223   0: . . NULL
       : . . }
225  64: . OCTET STRING    
       : . . E8 DC 24 7E 51 87 85 87    ..$~Q...
       : . . 96 CA 00 05 E4 FB 58 CC    ......X.
       : . . 8D C4 E4 5F 7D 2B B3 47    ..._}+.G
       : . . 2D ED E5 09 47 F0 F2 55    -...G..U
       : . . C5 5A F6 FA 99 BA D8 66    .Z.....f
       : . . 96 B4 ED F1 0B E2 91 CD    ........
       : . . 6C D8 2C A1 55 50 25 E1    l.,.UP%.
       : . . E4 A9 E5 96 D3 03 D7 19                            
       : . }

whereas it should be the following (this is after applying my patch):
212  12: . SEQUENCE {
214   8: . . OBJECT IDENTIFIER ecdsaWithSHA256 (1 2 840 10045 4 3 2)
       : . . . (ANSI X9.62 ECDSA algorithm with SHA256)
224   0: . . NULL
       : . . }
226  72: . OCTET STRING, encapsulates {
228  70: . . SEQUENCE {
230  33: . . . INTEGER    
       : . . . . 00 F7 7D 41 9A 12 38 99    ..}A..8.
       : . . . . CC 63 81 0A E1 FF A8 ED    .c......
       : . . . . 0B 03 3F AF 38 01 70 59    ..?.8.pY
       : . . . . A7 25 1A 13 C8 50 6F B2    .%...Po.
       : . . . . D6                         .
265  33: . . . INTEGER    
       : . . . . 00 88 83 38 85 A6 6C 54    ...8..lT
       : . . . . 11 B2 BE C4 EB 49 E4 AC    .....I..
       : . . . . A4 A6 51 0D 95 AB BE E5    ..Q.....
       : . . . . 8D 20 2D 25 14 02 5B 33    . -%..[3
       : . . . . 8F                         .
       : . . . }
       : . . }
       : . }

The patch fixes the OID that goes into the signatureAlgorithm field as well as passing the full signature algorithm to the Signature context to generate the signature using the proper algorithm.

According to RFC 5753 (Use of Elliptic Curve Cryptography (ECC) Algorithms in Cryptographic Message Syntax (CMS)):
   -  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).

   -  signature MUST contain the DER encoding (as an octet string) of a
      value of the ASN.1 type ECDSA-Sig-Value (see Section 7.2).

I also see that this same behavior with RSA signature algorithms where regardless of the signature algorithm that the user passed into the constructor, the encoded signatureAlgorithm will be 1.2.840.113549.1.1.1 (rsaEncryption).

With my patch, If I pass SignatureAlgorithm.RSASignatureWithSHA256Digest in the constructor I will now get sha256WithRSAEncryption (1 2 840 113549 1 1 11) in the signatureAlgorithm field.
Assignee: glenbeasley → cfu
Status: NEW → ASSIGNED
Comment on attachment 8919888 [details]
SignerInfo-patch.txt

Patch looks good.  I have verified the issue and patch for both RSA and EC.
This could be rolled in at the next JSS respin.
Attachment #8919888 - Flags: review+
changeset:   2207:b1a3c3cc6b3584948d251d3bfcfe6630d8970db5
tag:         tip
phase:       public
parent:      2206:252c10f448971b7ae087bde259505abd5dc5a03a
parent:      -1:0000000000000000000000000000000000000000
manifest:    2006:7b25ef6eb73049be7b320e796e01bf04b6dfd257
user:        David Stutzman david.konrad.stutzman@us.army.mil
date:        Thu Oct 26 16:59:06 2017 -0700
files:       org/mozilla/jss/pkix/cms/SignerInfo.java
extra:       amend_source=9024dcb9deab76bd7e28adecbe54d65af90751e8
extra:       branch=default
description:
Bugzilla.mozilla 1409867 org.mozilla.jss.pkix.cms.SignerInfo incorrectly producing signatures (especially for EC)

The patch fixes the OID that goes into the signatureAlgorithm field as well as passing the full signature algorithm to the Signature context to generate the signature using the proper algorithm.
With this patch, if one passes SignatureAlgorithm.RSASignatureWithSHA256Digest in the constructor one will now get sha256WithRSAEncryption (1 2 840 113549 1 1 11) in the signatureAlgorithm field.

cfu checking in for dstutzman
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I am re-opening this bug.  After further review and testing, it was determined that signatures were still being created incorrectly.  

This new patch was cross-tested with OpenSSL and Bouncy Castle libraries to ensure compatibility.  After patching JSS on a Dogtag CA and using the same patched SignerInfo code on the client side, I tested creating agent-signed CMC enrollment requests, which the CA was able to verify successfully and my client code was able to verify the CMC response from the CA successfully.  I then tested the same operations creating and verifying the SignerInfo parts of the CMC request/response using the Bouncy Castle library which was also successful.

I also tested a CMS structure created with OpenSSL and was able to verify with both this patched JSS and Bouncy Castle. Bouncy Castle and OpenSSL were able to verify a CMS structure that this patched JSS created.  I tested data signed with both RSA and EC credentials.

I also determined that due to the fact that the JSS SignerInfo constructor is passed in pre-digested data and that JSS can *not* do a raw ECDSA signature (NONEwithECDSA in JCA) that JSS will create an incompatible SignerInfo if the content-type is DATA and no signed attributes are passed to the constructor.  JSS itself will still "verify" this, but any other cryptographic library will be unable to verify.  In this case JSS will create an signature, that is not really an ECDSA signature, that looks like this:

225  64: . OCTET STRING    
       : . . E8 DC 24 7E 51 87 85 87    ..$~Q...
       : . . 96 CA 00 05 E4 FB 58 CC    ......X.
       : . . 8D C4 E4 5F 7D 2B B3 47    ..._}+.G
       : . . 2D ED E5 09 47 F0 F2 55    -...G..U
       : . . C5 5A F6 FA 99 BA D8 66    .Z.....f
       : . . 96 B4 ED F1 0B E2 91 CD    ........
       : . . 6C D8 2C A1 55 50 25 E1    l.,.UP%.
       : . . E4 A9 E5 96 D3 03 D7 19                            
       : . }

For the patch, I removed unused/duplicated imports and commented out constructor/setters in addition to my needed changes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch jss-signerinfo.patch (obsolete) — Splinter Review
Updated patch for creating/verifying signatures
Attachment #8919888 - Attachment is obsolete: true
Attached patch jss-signerinfo.patch (obsolete) — Splinter Review
Updated patch against hg tip (sorry!)
Attachment #8929013 - Attachment is obsolete: true
Another new patch made and tested to import into clean repo.
Attachment #8932175 - Attachment is obsolete: true
Attached patch jss-Signerinfo-dstutzman.patch (obsolete) — Splinter Review
Patch done properly with hg toolset, applies to a clean checkout.
Attachment #8935768 - Attachment is obsolete: true
Comment on attachment 8938950 [details] [diff] [review]
jss-Signerinfo-dstutzman.patch

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

Hi Dave,
Thank you for the patch. The patch applies clealy and seems to work for some operations I run with Dogtag.  Code logic looks fine. Just a few comments:
1. Please leave the low-level constructors alone for now as we have no way of determining whether there are applications that depend on them, and we should not risk breaking them.
2. Please file a separate bug for "...JSS can't do a non-digested ECDSA signature..." so that it won't be forgotten and can be looked into later.
3. for createDigestInfo(),
  a. why is the first parameter, AlgorithmIdentifier digestAlgId, needed if its always the class variable this.digestAlgorithm?
  b. please add check for null on the incoming params (It looks like the existing code is not doing much of that, but since this is a new method, let's just do it right)

thanks,
Christina
1) All of those "low-level" methods I removed were commented out so it was impossible for them to be used.  If you would like me to leave them I can.

2) I do still plan on doing this.  I spent a good amount of time doing more testing. I develop normally on Windows and I am using older NSS 3.12.7 and JSS 4.3.2.  I was able to get the latest NSS/NSPR built for windows but it won't work with the older JSS DLL I have built and I am unable to build a current JSS on windows anymore.  I plan on switching to a Linux dev environment to do further testing before I file another bug.  Just wanted to confirm it was still a problem with a non-ancient version of NSS.  I suppose I should remove the comment in this change about that just in case that ends up being the case.

3)It does seem passing the AlgorithmIdentifier as a param is not necessary and I will remove/change to using class field.  I will add check for the byte[] and throw IllegalArgumentException if null.  The boolean param can't be null since this is a private method, no one can pass in an autoboxed null "Boolean".  Java won't let me write a null-check on a primitive.  Unless you would prefer I make it a Boolean and have the null check.
(In reply to David Stutzman from comment #9)
> 1) All of those "low-level" methods I removed were commented out so it was
> impossible for them to be used.  If you would like me to leave them I can.

I stand corrected.  I  misread that part of the patch.  No, it's fine, it's okay to remove the commended out code.

> 
> 2) I do still plan on doing this.  I spent a good amount of time doing more
> testing. I develop normally on Windows and I am using older NSS 3.12.7 and
> JSS 4.3.2.  I was able to get the latest NSS/NSPR built for windows but it
> won't work with the older JSS DLL I have built and I am unable to build a
> current JSS on windows anymore.  I plan on switching to a Linux dev
> environment to do further testing before I file another bug.  Just wanted to
> confirm it was still a problem with a non-ancient version of NSS.  I suppose
> I should remove the comment in this change about that just in case that ends
> up being the case.

yes, it would be a good idea to remove the comment for this patch.  Due to our schedule, I'd
suggest that you
a. make the minor changes per review on this patch within a week (the sooner the better)
  (please only remove or replace that comment, and make the param change/check as agreed in "3")
b. file a separate bug and work on the remaining issue (sorry I can't help you with Windows)

> 
> 3)It does seem passing the AlgorithmIdentifier as a param is not necessary
> and I will remove/change to using class field.  I will add check for the
> byte[] and throw IllegalArgumentException if null.  The boolean param can't
> be null since this is a private method, no one can pass in an autoboxed null
> "Boolean".  Java won't let me write a null-check on a primitive.  Unless you
> would prefer I make it a Boolean and have the null check.

I was only talking about the non-boolean params.

thanks!
Attached patch SignerInfo.patchSplinter Review
Removed comment, removed AlgorithmIdentifier param from new method in lieu of using class field, and added parameter check for byte[] param.
Attachment #8938950 - Attachment is obsolete: true
Thanks Dave! Patch pushed.

changeset:   2211:9e2db7eee6652330723d935c2b900b9b09b1ab9d
tag:         tip
phase:       public
parent:      2210:ca2c2fcfaf207f87c3c69e493f2b30fd0a088e95
parent:      -1:0000000000000000000000000000000000000000
manifest:    2010:236d06cd894e27b79c5dde2531e6ed127b312c1e
user:        David Stutzman<david.konrad.stutzman@us.army.mil>
date:        Thu Jan 11 14:58:44 2018 -0800
files:       org/mozilla/jss/pkix/cms/SignerInfo.java
extra:       amend_source=1c6f6783037e960ed8a5d2e7ca58329ad907d3bd
extra:       branch=default
description:
Bug 1409867 - additional fix from dstutzman: allow signatures to be created correctly.

cfu for dstutzman
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: