Closed Bug 240554 Opened 16 years ago Closed 15 years ago
Add ECDSA support in S/MIME
NSS 3.9 with ECC support does not enable the use of ECDSA signatures with S/MIME.
Adds the ability to sign and verify S/MIME messages using ECDSA (work in progress)
Adds new test script nss/tests/smime/ecsmime.sh and updates nss/tests/fixtests.sh to include ecdsa in smime tests.
Attachment #146145 - Attachment is obsolete: true
Updates attachment 146144 [details] [diff] [review] by adding a new test script in nss/tests/smime/ecsmime.sh
Attachment #146144 - Attachment is obsolete: true
Vipul, Are those patches ready to be reviewed or are you still working on them ?
(In reply to comment #5) > Vipul, > > Are those patches ready to be reviewed or are you still working on them ? > Hi Julien, There were some changes made to the S/MIME code between NSS 3.8 and NSS 3.9. While this patch used to pass all the ecsmime.sh tests earlier, it now fails the "Decode Alice's Signature (ECDSA SHA1)" test. I haven't looked at the bug recently but here are my notes from my last attempt in late March. cmd/smimeutils/cmsutil.c prints error in decode(): line 349 signer = 0 status = SigningCertNotTrusted and in doBatchDecode: line 1055 cmsutil: problem decoding: Peer's Certificate issuer is not recognized I subsequently got busy with other stuff and wasn't able to resolve this issue. Do you know what changed between 3.8 and 3.9 as far as the S/MIME code is concerned? Interestingly enough, when I use this patch to create an ECC enabled version of Mozilla mail, it is able to sign and verify messages with ECDSA successfully (but, of course, we have no way to test interoperability with another implementation of ECDSA in S/MIME). vipul
Between NSS 3.8 and NSS 3.9, the S/MIME code was cleaned up. Due to these changes, one of the newly added tests in ecsmime.sh started failing (the test that tries to decode Alice's ECDSA signature). It turns out that the reason for the failure was that the script eccert.sh in tests/cert did not add Alice's EC certificate and the ECC Test CA's certificate in Bob's certificate database. This patch takes care of that issue. I've tested this patch on NSS 3.9 and all S/MIME tests (including all new ECC based tests) now pass.
It shouldn't be necessary for Alice's cert to be in Bob's DB, but the root for Alice's cert(s) should be there in Bob's DB and be trusted.
(In reply to comment #8) > It shouldn't be necessary for Alice's cert to be in Bob's DB, but the root > for Alice's cert(s) should be there in Bob's DB and be trusted. This makes sense. I'll retry the tests without adding Alice's cert to Bob's database and let you know. I'm in the process of testing these patches on the mozilla trunk. I noticed that tests/cert/cert.sh puts Alice's RSA cert into Bob's DB. And when I modified eccert.sh, I was simply mirroring that code for EC certs. Does this mean that cert.sh also needs to be changed? vipul
When setting up the DB, prior to reading a signed email, only the sender's root CA needs to be in the DB and trusted. When the mozilla browser reads a signed email, if it finds the sender's ENCRYPTION cert, it saves a copy of that cert (and the chain for it, IIRC) in the DB, so that we can later find that cert to send an encyrpted reply. If the signed email contains separate signing and encryption certs, mozilla only saves the encryption cert. If the signed email contains only a signing cert (no encryption cert) then we do not save any cert. So, in the alice-bob test, if alice's signed email contains an encryption cert, that cert will be saved as a side effect of reading it in mozilla. That's how it's intended to work. If it works differently, we should fix that. The cmsutil program has a command line option to enable the saving of decoded encryption certs, just as mozilla does. It's the -k (keep) option. If the test script is using it, then cmsutil will also save encryption certs. Hope that helps.
This patch enables ECDSA support in S/MIME for the mozilla trunk (as of Jun 11, 2004). It combines the functionality of attachments 146148 and 150561. I have also verified that it is not necessary for Alice's EC cert to be imported into Bob's certdb (see Nelson's comment above). I think this patch is ready for review and check-in. vipul NOTE: In order to enabled ECDSA support in S/MIME for NSS 3.9 (used in Mozilla 1.6), one would still need to apply both 146147 and 150561.
Comment on attachment 150581 [details] [diff] [review] Enables ECDSA support in S/MIME for Mozilla trunk (as of Jun 11, 2004) Nelson, Please let me know who else should be reviewing this patch. thanks, vipul
Comment on attachment 150581 [details] [diff] [review] Enables ECDSA support in S/MIME for Mozilla trunk (as of Jun 11, 2004) Please explain why you're changing from the use of - signalg = SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST; to + signalg = SEC_OID_ANSIX962_EC_PUBLIC_KEY; That change certainly seems inconsistent with ALL the other signature algids. Is there some standard that demands this? Or is this for compatibility with some third party's software?
(In reply to comment #13) > (From update of attachment 150581 [details] [diff] [review]) > Please explain why you're changing from the use of > - signalg = SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST; > to > + signalg = SEC_OID_ANSIX962_EC_PUBLIC_KEY; > > That change certainly seems inconsistent with ALL the other signature algids. > Is there some standard that demands this? > Or is this for compatibility with some third party's software? NSS expects the value set in signalg to match the algorithm in the public key info field of the cert used for verification. (Off hand, I don't remember which part of the code this is but I can dig up the details in next week -- it might be later since I will be in San Francisco for Java One from Mon-Thu). According to RFC 3279 (section 2.3.5), this field is set to SEC_OID_ANSIX962_EC_PUBLIC_KEY for all ECC certificates. Before I made this change, signature verification used to fail because the algorithm field in the cert did not match signalg. Given your familiarity with NSS, you might already know the code I'm talking about. vipul
(In reply to comment #14) > NSS expects the value set in signalg to match the algorithm > in the public key info field of the cert used for verification. .... > > According to RFC 3279 (section 2.3.5), this field is set to > SEC_OID_ANSIX962_EC_PUBLIC_KEY for all ECC certificates. > Before I made this change, signature verification used to fail > because the algorithm field in the cert did not match signalg. After I wrote this I realized that perhaps the right thing to do is to make changes to code that does the match. Unfortunately, all of this was done a long time ago and I can't remember the details. Can you help by pointing me to files that are likely to contain the code that matches the public key info algorithm to signalg? If not, I'll try to refresh my memory by tracing through the existing code. vipul
Hi Nelson, Sorry it took me a while to investigate this further. I was at JavaOne the last week of June and Sun was closed the following week. Here's what I found during my investigation today. In the patch submitted as attachment 150581 [details] [diff] [review], the changes in secsign.c are not necessary. I can't recall what prompted them in the first place but removing them does address the concern you expressed in comment 13. I also discovered that the new S/MIME tests (see ecsmime.sh) pass without any changes to p7encode.c and p7decode.c. However, it seems to me that those changes should be included. Is there a test script for testing PKCS7 in NSS? One final question: In the cases I have traced through, the algid value passed to decodeECorDSASignature in secvfy.c seems to come from the algorithm in the subject public key info field of the cert used to verify the signature. So I wonder if there will ever be a situation when algid will be SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST. If you agree that it will always be SEC_OID_ANSIX962_EC_PUBLIC_KEY, then this can be reflected in the patch to secvfy.c. I look forward to your comments. In the mean time, I will upload a new patch (the same as attachment Id 150581 but without any changes to secsign.c). vipul
This is the patch described in comment #15. It is essentially the same as attachment 150581 [details] [diff] [review] except that it does not change secsign.c. This patch has been tested by running the new ECDSA in S/MIME tests in ecsmime.sh. To run ECC tests: cd mozilla/security/nss/tests ./fixtests enable_ecc ./all.sh
Attachment #150581 - Attachment is obsolete: true
It has been a while since the last update regarding this bug. Is anyone from the NSS team available to review the latest patch? thanks, vipul
Unsetting target due to the uncertainties around ECC .
Target Milestone: 3.10 → ---
QA Contact: bishakhabanerjee → jason.m.reid
Comment on attachment 153328 [details] [diff] [review] Enables ECDSA support in S/MIME for Mozilla trunk (as of Jul 15, 2004) r=wtc. This patch only has a minor problem: In mozilla/security/nss/lib/pkcs7/p7decode.c >+#ifdef NSS_ENABLE_ECC >+ (algiddata->offset != SEC_OID_ANSIX962_EC_PUBLIC_KEY) && >+#endif NSS_ENABLE_ECC The "NSS_ENABLE_ECC" after "#endif" needs to be in a comment. I will attach a new version of this patch against the current NSS tip. The new file ecsmime.sh will use the new MPL/GPL/LGPL triple license.
Attachment #153328 - Flags: review?(nelson) → review+
For DSA we have two OIDs: SEC_OID_ANSIX9_DSA_SIGNATURE = 124, SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST = 125, For ECDSA we only have one OID: SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST = 201, Why don't we have the OID SEC_OID_ANSIX962_ECDSA_SIGNATURE?
Assignee: julien.pierre.bugs → wtchang
Hi Wan-Teh, The ANSI X9.62 definition of ECDSA ties it to the use of the SHA1 hash algorithm. That specification only defines an OID for ecdsa-with-SHA1 (pg 133). I'll forward you a copy of the spec separately so we can make sure we haven't overlooked anything. thanks, vipul (In reply to comment #22) > For DSA we have two OIDs: > > SEC_OID_ANSIX9_DSA_SIGNATURE = 124, > SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST = 125, > > For ECDSA we only have one OID: > > SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST = 201, > > Why don't we have the OID SEC_OID_ANSIX962_ECDSA_SIGNATURE?
Hi Vipul, I found the answer to my question in RFC 3279. SEC_OID_ANSIX962_EC_PUBLIC_KEY is the OID for the EC public key algorithm and is EC's counterpart to SEC_OID_PKCS1_RSA_ENCRYPTION and SEC_OID_ANSIX9_DSA_SIGNATURE.
OK, after actually building and testing Vipul's patch, I understood our S/MIME code better now and figured out the right way to do signature verification. So no changes to lib/cryptohi is necessary. Unfortunately this requires my own changes to lib/smime/cmssiginfo.c, so I'll need Bob Relyea to review them. I didn't make the corresponding changes to lib/pkcs7 because I don't know how to test our pkcs7 code. I only added "XXX" comments to note where changes should be made. I kept Vipul's changes to lib/pkcs7 though because I am sure they are correct. Bob, you only need to review my changes to lib/crypto/secsign.c and lib/smime/cmssiginfo.c. In lib/crypto/secsign.c, I use a more informative error code. In lib/smime/cmssiginfo.c, we should pass the signature algorithm ID (rather than public key algorithm ID) to VFY_VerifyData and VFY_VerifyDigest. That algid is eventually passed to VFY_CreateContext.
I read all the comments and patches in this bug and here is hopefully the final patch that addresses all the issues Vipul identified. Bob, you only need to review the changes to the following files: secsign.c cmssiginfo.c cert.sh In cert.sh, I verified that our S/MIME test does not require importing Alice's cert into Bob's db.
Comment on attachment 192442 [details] [diff] [review] Wan-Teh's additional changes I've checked in all of Vipul's patch except the change for lib/cryptohi/secvfy.c because I came up with a better fix (see the change to lib/smime/cmssiginfo.c in this patch). I created this patch for the remaining changes, which are all mine. Hopefully this smaller patch is easier to review. Here is a short description of the changes in this patch. 1. lib/cryptohi/secsign.c: - Set a more informative error code. - Set the error code and remove an unreachable break statement. 2. lib/smime/cmssiginfo.c: in NSS_CMSSignerInfo_Verify, we are passing the public key's algorithm to VFY_VerifyData and VFY_VerifyDigest. I think that's wrong. Based on the comments for VFY_VerifyData and VFY_VerifyDigest, I believe their algid parameter should be the signature algorithm (a composite of the public key's algorithm and the message digest algorithm). 3. lib/smime/cmsutil.c: a fix for signed/unsigned comparison compiler warning. 4. lib/pkcs7/p7decode.c: I don't want to make the same fix (public key's algorithm vs. the composite signature algorithm) to our PKCS7 code, so I just added XXX comments. 5. tests/cert/cert.sh and tests/cert/eccert.sh: Alice's cert doesn't need to be added to Bob's db.
Attachment #192442 - Attachment description: Wan-Teh → Wan-Teh's additional changes
Comment on attachment 192442 [details] [diff] [review] Wan-Teh's additional changes r=relyea yes, the VFY expects signature algs, not 'mechanism' algs.
Attachment #192442 - Flags: review?(rrelyea) → review+
All patches have been checked in on the NSS trunk (NSS 3.11).
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11
(In reply to comment #28) > 4. lib/pkcs7/p7decode.c: I don't want to make the same fix > (public key's algorithm vs. the composite signature algorithm) > to our PKCS7 code, so I just added XXX comments. Why not fix the PKCS7 code also?
Nelson: I don't want to fix the PKCS7 code without a way to test it. This is the same reason Vipul gave in comment 16.
You need to log in before you can comment on or make changes to this bug.