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)
JSS Graveyard
Library
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: david.konrad.stutzman, Assigned: cfu)
Details
Attachments
(1 file, 5 obsolete files)
10.91 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: glenbeasley → cfu
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
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+
Assignee | ||
Comment 2•7 years ago
|
||
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
Reporter | ||
Comment 3•7 years ago
|
||
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 → ---
Reporter | ||
Comment 4•7 years ago
|
||
Updated patch for creating/verifying signatures
Attachment #8919888 -
Attachment is obsolete: true
Reporter | ||
Comment 5•6 years ago
|
||
Updated patch against hg tip (sorry!)
Attachment #8929013 -
Attachment is obsolete: true
Reporter | ||
Comment 6•6 years ago
|
||
Another new patch made and tested to import into clean repo.
Attachment #8932175 -
Attachment is obsolete: true
Reporter | ||
Comment 7•6 years ago
|
||
Patch done properly with hg toolset, applies to a clean checkout.
Attachment #8935768 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
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
Reporter | ||
Comment 9•6 years ago
|
||
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.
Assignee | ||
Comment 10•6 years ago
|
||
(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!
Reporter | ||
Comment 11•6 years ago
|
||
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
Assignee | ||
Comment 12•6 years ago
|
||
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 ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•