240554 - Add ECDSA support in S/MIME

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Vipul Gupta]

NSS 3.9 with ECC support does not enable the use of ECDSA signatures with S/MIME.

Comment 1

[Vipul Gupta]

Attachment: This patch enables ECDSA support in S/MIME

Adds the ability to sign and verify S/MIME messages using ECDSA (work in
progress)

Comment 2

[Vipul Gupta]

Attachment: Same patch as above backported to Mozilla-1.6

Same patch as attachment 146144 [details] [diff] [review] but back ported for Mozilla 1.6

Comment 3

[Vipul Gupta]

Attachment: Updated patch for Mozilla-1.6

Adds new test script nss/tests/smime/ecsmime.sh and updates
nss/tests/fixtests.sh to include ecdsa in smime tests.

Comment 4

[Vipul Gupta]

Attachment: This patch enabled ECDSA support in S/MIME for the mozilla trunk

Updates attachment 146144 [details] [diff] [review] by adding a new test script in
nss/tests/smime/ecsmime.sh

Comment 5

[Julien Pierre]

Vipul,

Are those patches ready to be reviewed or are you still working on them ?

Comment 6

[Vipul Gupta]

(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

Comment 7

[Vipul Gupta]

Attachment: Fixes tests/cert/eccert.sh for new S/MIME tests that use ECDSA

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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Vipul Gupta]

(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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 11

[Vipul Gupta]

Attachment: Enables ECDSA support in S/MIME for Mozilla trunk (as of Jun 11, 2004)

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 12

[Vipul Gupta]

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 13

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 14

[Vipul Gupta]

(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

Comment 15

[Vipul Gupta]

(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

Comment 16

[Vipul Gupta]

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

Comment 17

[Vipul Gupta]

Attachment: Enables ECDSA support in S/MIME for Mozilla trunk (as of Jul 15, 2004)

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

Comment 18

[Vipul Gupta]

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

Comment 19

[Julien Pierre]

Unsetting target due to the uncertainties around ECC .

Comment 20

[Wan-Teh Chang]

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.

Comment 21

[Wan-Teh Chang]

Attachment: Enables ECDSA support in S/MIME for NSS trunk (as of Aug 2, 2005)

Comment 22

[Wan-Teh Chang]

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?

Comment 23

[Vipul Gupta]

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?

Comment 24

[Wan-Teh Chang]

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.

Comment 25

[Wan-Teh Chang]

Attachment: Enables ECDSA support in S/MIME for NSS trunk, v2.0

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.

Comment 26

[Wan-Teh Chang]

Attachment: Enables ECDSA support in S/MIME for NSS trunk, v2.1

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 27

[Wan-Teh Chang]

Attachment: Wan-Teh's additional changes

Comment 28

[Wan-Teh Chang]

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.

Comment 29

[Robert Relyea]

Comment on attachment 192442 [details] [diff] [review]
Wan-Teh's additional changes

r=relyea

yes, the VFY expects signature algs, not 'mechanism' algs.

Comment 30

[Wan-Teh Chang]

All patches have been checked in on the NSS trunk (NSS 3.11).

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

(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?

Comment 32

[Wan-Teh Chang]

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.