354273 - S/MIME signed emails include duplicate cert in chain

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[rocketraman]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060913 Fedora/1.5.0.7-1.fc5 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060913 Fedora/1.5.0.7-1.fc5 Firefox/1.5.0.7

I have a Thawte Personal Freemail certificate, which has a certificate chain consisting of three certificates:

Thawte Personal Freemail CA (serial number 0x00)
  Thawte Personal Freemail Issuing CA (serial number 0x0D)
    Thawte Freemail Member (my cert serial 0xFCB65)

When I send a signed or signed/encrypted email, the sent email does not contain the full certificate chain as it should -- instead, it contains two copies of my cert (serial number 0xFCB65) and one copy of the Issuing CA cert (serial number 0x0D).

This has been reproduced with the nightly build 

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2006-09-25-06-trunk/thunderbird-3.0a1.en-US.win32.zip

as well as the release versions of Thunderbird 1.5.0.7 on Linux Fedora Core 5 and 1.5.0.5 on Windows.

Reproducible: Always

Steps to Reproduce:
1. Send yourself a signed email, or a signed/encrypted email.
2. Save email to disk via "Save as".
3. Use OpenSSL to verify the certificate contents:

For a signed-only email:

openssl smime -in email.eml -pk7out | openssl pkcs7 -print_certs -text -noout | grep -A 3 "^Certificate"

For a signed/encrypted email, first decrypt using

openssl smime -decrypt -in email.eml -recip mycert.pem -inkey mykey.pem
Actual Results:  
The cert with serial 0xFCB65 (mine) is included twice, and the root cert with serial 0x0 is not included at all.

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 1035109 (0xfcb65)
--
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 1035109 (0xfcb65)
--
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 13 (0xd)


Expected Results:  
Here are the results of the openssl certificate print using a signed message from a recent version of Outlook, which has the correct behaviour:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 1035109 (0xfcb65)
--
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 0 (0x0)
--
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 13 (0xd)


This causes interop problems with some software, notably Novell Groupwise. Other software may have a problem with this as well.

Comment 1

[rocketraman]

This is still a problem as of the nightly build 

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2007-12-31-04-trunk/thunderbird-3.0a1pre.en-US.linux-i686.tar.bz2

as well as the release version of Thunderbird 2.0.0.9 on Linux Fedora 8.

Comment 2

[Kaspar]

Attachment: Proposed fix to suppress duplicate end-entity certs

(In reply to comment #0)
> When I send a signed or signed/encrypted email, the sent email does not
> contain the full certificate chain as it should

Seems like this was a conscious design decision, back in 2002 - cf. bug 136445 comment 5. The S/MIME RFCs about certificate handling (RFC 2312 - v2, RFC 2632 - v3, RFC 3850 - v3.1), don't disallow this, however:

   Agents MAY send CA certificates, that is, certificates which can be
   considered the "root" of other chains, and which MAY be self-signed.

I wrote a patch for this sometime in 2006, which I might be able to dig up again, if there's an interest in it (it basically adds a new "Include root certificate in signed messages" check box to the account settings).

> instead, it contains two copies of my cert (serial number 0xFCB65)

That's a flaw in nsCMSMessage::CreateSigned, which adds the encryption cert a second time, even if it's identical to the signing certificate. The attached patch fixes this - who would be the right person for a review?

Comment 3

[Kaspar]

Confirming.

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 295619 [details] [diff] [review]
Proposed fix to suppress duplicate end-entity certs

What happens if:
- a signing cert is configured
- the function will not sign
- the function will encrypt only?

In this scenario, will scert==nsnull or will scert==ecert?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Whether the MUA needs to send the root is perhaps debatable.
The assumption has always been that the recipient must already possess 
the root and trust it as a precondition of trusting the signature on an
email.  

But sending the same EE cert twice is silly.  

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: sample signed email exhibiting defect

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: a "pretty print" of the signature, showing the duplicated certs

This is a formatted dump of the p7s signature on the email in the previous 
bug attachment 322644 [details].

Comment 9

[Henk van de Kamer]

In bug 431660 it was suggested that the bug could be due to a Debian specific modification. To show that this is not the case I just sent you another test e-mail from a vanilla Thunderbird working under Windows XP and downloaded from the Mozilla website :-).

As said in my original report, if you disable or use another certificate for the encryption part, there is not a duplicate sent. The attached patch suggest that the code does indeed work like this. So a simple check if both are the same should be enough. You can even argue that sending a certificate for encryption should not be done (if you use a seperate certificate you probably are very security aware and ships that one to your communication partner in another way?).

As you see in the above test e-mail, sending the intermediate certificates is a good idea. However sending the root certificate is not. That one should be already present at the other end or should explicitly imported and trusted.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 295619 [details] [diff] [review]
Proposed fix to suppress duplicate end-entity certs


>-    if (NSS_CMSSignedData_AddCertificate(sigd, ecert) != SECSuccess) {
>+    if (ecert != scert &&

The proper test for equality of those two CERTCertificates would be:
      if (ecert && scert && CERT_CompareCerts(ecert, scert)

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

oops, I reversed the sense of the comparison.
The equivalent test for ecert != scert would be

     if (ecert && !(scert && CERT_CompareCerts(ecert, scert)) //...

or equivalently

     if (ecert && (!scert || !CERT_CompareCerts(ecert, scert)) //...

Comment 12

[Kai Engert [:KaiE:]]

Nelson, function CERT_CompareCerts is not currently being exported in .def files.

FWIW, PSM also uses a simple pointer value equality testing to compare certs for equality. (just stating facts, see nsNSSCertificate::Equals)


If function CERT_CompareCerts were exported, I'd write it as this:
-    if (NSS_CMSSignedData_AddCertificate(sigd, ecert) != SECSuccess) {
+    // If signing and encryption cert are identical, don't add it twice.
+    PRBool addEncryptionCert =
+      (ecert && (!scert || !CERT_CompareCerts(ecert, scert)));
+
+    if (addEncryptionCert &&
+        NSS_CMSSignedData_AddCertificate(sigd, ecert) != SECSuccess) {
       PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSMessage::CreateSigned - can't add own encryption certificate\n"));
       goto loser;
     }

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Please file a bug (RFE) about exporting function CERT_CompareCerts.

Comment 14

[rocketraman]

As per Bug #461085 CERT_CompareCerts is now exported. Kai's fix from Comment #12 could now be implemented?

Comment 15

[rocketraman]

FYI, Thunderbird is still sending duplicate certs as of Thunderbird 3 rc1.

Comment 16

[Bob Lord]

I replicated the issue and see duplicate certs being sent in signed emails.

Comment 17

[Bob Lord]

Changing the title to focus on the duplicate cert issue.

I also opened up Bug 540498 to track the request to add the root in the outgoing signed message.

Comment 18

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Patch exactly as in comment 12.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 450127 [details] [diff] [review]
Patch v2

r=nelson

Comment 20

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/mozilla-central/rev/c4f051d0bccf