558337 - Wrong S/MIME recipient certificate selected if two certificates have same subject

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[hans-joachim.knobloch]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4

In my Thunderbird certificate store I have two s/MIME recipient certificates with the same subject name but different e-mail addresses in the subjectAltNames certificate extension and different public keys.

Whenever I try to send an encrypted e-mail to any of these two e-mail addresses, only one of the two addresses is selected, which will lead to decryption failures for the other of the two affected recipient addresses.

Apparently Thunderbird does not directly select the certificate with the respective e-mail address in the subjectAltNames extension but the first (last, whatever) certificate in store that hast the same subject distinghiushed name as the one with the intended recipient e-mail address in the subjectAltNames extension.

An attacker might exploit this (albeit not quite easily) by planting his own, ceritficate in the sender's Thunderbird certificate store, provided he knows the subject distinghuished name of the recipient's certificate and can obtain a valid certificate with the same subject DN (but of his own e-mail addresses) for a trusted CA.

Reproducible: Always

Steps to Reproduce:
1. Generate two S/MIME certificates with different e-mail address and key but identical subject distinguished names.
2. Import these certificates in Thunderbird certificate store.
3. Compose S/MIME encrypted message to first e-mail address. Click on S/MIME security information to see the selected recipient certificate.
4. Compose S/MIME encrypted message to second e-mail address. Click on S/MIME security information to see the selected recipient certificate.
Actual Results:  
For both messages the same recipient certificate is selected. Which is obvipously wrong for one of the two messages and will result in a decryption errror for the affected recipient.
(Or even fraudulent decryption of a captured e-mail by an attacker.)

Expected Results:  
For each messaage the certificate with the correct recipient address must be selected.

For reproducing the bug, you might use the two certificates for e-mail addrsses
[e-mail]
and
[e-mail]
that can be retrieved via https://www.trustcenter.de/fcgi-bin/Search.cgi?Language=en (remember to check "E-Mail address" in "Where to search")

Comment 1

[hans-joachim.knobloch]

s/only one of the two addresses is selected/only one of the two certificates is selected/

Comment 2

[Ludovic Hirlimann [:Usul]]

Would this attack lead to code execution ?

Comment 3

[hans-joachim.knobloch]

No. It would lead to a message being encrypted for decryption with the attacker's key instead of the recipient's.

Comment 4

[Ludovic Hirlimann [:Usul]]

(In reply to comment #3)
> No. It would lead to a message being encrypted for decryption with the
> attacker's key instead of the recipient's.

Un-hidding. Bob can you double check that I'm right in un-hidding ?

Comment 5

[Kaspar]

Confirming. The problem is with the following PSM code:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp&rev=1.32&mark=1452,1459-1460,1466,1472#1447

nsNSSCertificateDB::FindCertByEmailAddress() first looks for a certificate with a matching e-mail address (CERT_FindCertByNicknameOrEmailAddr), then looks up all certificates with the same subject DN (CERT_CreateSubjectCertList), filters out those certs which are not suitable for e-mail encryption (CERT_FilterCertListByUsage) and finally picks the first certificate from the resulting list (CERT_LIST_HEAD(certlist)->cert).

Comment 6

[Kai Engert (:KaiE:)]

Kaspar, thanks a lot for your analysis.

> nsNSSCertificateDB::FindCertByEmailAddress() first looks for a certificate with
> a matching e-mail address (CERT_FindCertByNicknameOrEmailAddr), then looks up
> all certificates with the same subject DN (CERT_CreateSubjectCertList),

This is done, because of dual-key or multi-key certificates, where a group of certificates is used, each cert having a separate usage (like separate signing and encryption certificates).


> filters
> out those certs which are not suitable for e-mail encryption
> (CERT_FilterCertListByUsage) 

right


> and finally picks the first certificate from the
> resulting list (CERT_LIST_HEAD(certlist)->cert).

So, the logic doesn't expect there could be certs with identical subject which have a different email address.

As the final step, we should iterate the result list, and skip the certs until we find a cert that is valid for the desired email address, right?

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Kaspar, do you agree with this patch?

Honza, could you please review?

Comment 8

[chokito]

Yesterday I have renewed my certificates and have the same problem. All old certificates has the e-mail address included in subject but all the new hasn't. I wrote an e-mail to "TC TrustCenter" and asked why the e-mail address is omitted in subject!

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 449635 [details] [diff] [review]
Patch v1

>diff --git a/security/manager/ssl/src/nsNSSCertificateDB.cpp b/security/manager/ssl/src/nsNSSCertificateDB.cpp
>+PRBool nsNSSCertificateDB::CertContainsEmailAddress(const char *aEmailAddress, CERTCertificate *aCert)
>+{
>+  if (!aEmailAddress || !aCert)
>+    return PR_FALSE;
>+  
>+  nsCAutoString testAddr(aEmailAddress);
>+  ToLowerCase(testAddr);
>+
>+  for (const char *aAddr = CERT_GetFirstEmailAddress(aCert)
>+       ;
>+       aAddr
>+       ;
>+       aAddr = CERT_GetNextEmailAddress(aCert, aAddr))
>+  {
>+    nsCAutoString certAddr(aAddr);
>+    ToLowerCase(certAddr);
>+
>+    if (certAddr == testAddr)
>+    {
>+      return PR_TRUE;
>+    }
>+  }
>+
>+  return PR_FALSE;
>+}

Please use nsDependentCString testAddr(aEmailAddress), also for aAddr and then compare using:

#ifdef MOZILLA_INTERNAL_API
  if (testAddr.Equals(aAddr, nsCaseInsensitiveCStringComparator() ))
#else
  if (testAddr.Equals(aAddr, CaseInsensitiveCompare))
#endif


or, maybe even better just testAddr.EqualsIgnoreCase(aAddr).


r=honzab with this updated.

Any idea for tests for this?

Comment 10

[Honza Bambas (:mayhemer)]

And a nit: ones you are using style

if () {
}

and on other place

if ()
{
}

Please be consistent, I personally like the letter one more.

Comment 11

[Kaspar]

(In reply to comment #7)
> Created attachment 449635 [details] [diff] [review]
> Patch v1
> 
> Kaspar, do you agree with this patch?

It's a band-aid fix, but alas, as long as NSS only provides CERT_FindCertByNicknameOrEmailAddr(), there's no better way it seems.

I would prefer bug 337433 being addressed first, though. This would allow simplifying the code in nsNSSCertificateDB::FindCertByEmailAddress().

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v2

This is v1 + Honza's requests addressed, in particular:

(In reply to comment #9)
> Please use nsDependentCString testAddr(aEmailAddress), also for aAddr
done

> compare using ... testAddr.EqualsIgnoreCase(aAddr).
done

regarding bracket style, I made it consistent.

Comment 13

[chokito]

(In reply to comment #8)
> Yesterday I have renewed my certificates and have the same problem. All old
> certificates has the e-mail address included in subject but all the new hasn't.
> I wrote an e-mail to "TC TrustCenter" and asked why the e-mail address is
> omitted in subject!

The "TC TrustCenter Team" wrote, that the e-mail address is conform to X.509 in SAN "SubjectAlternativeName" displayed.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to comment #12)
> Created attachment 467467 [details] [diff] [review]
> Patch v2
There is no need for ToLowerCase(testAddr);

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to comment #12)
> Created attachment 467467 [details] [diff] [review]
> Patch v2
As well as for nsDependentCString certAddr(aAddr); while EqualsIgnoreCase works for char*.

Comment 16

[Jürgen Brauckmann]

I'm not a regular here, so my comment might be a bit cheeky.

This patch may correct the one issue that was reported originally, but it will not fix the (in my opinion) wrong approach of nsNSSCertificateDB::FindCertByEmailAddress().

FindCertByEmailAddress() assumes that there is a connection between subject-dn and email-address. Therefore it searches for certificates with same subject-dn after it has found one certificate with matching email address. 

But this connection between subject-dn and email address does only exist in very rare conditions. In normal use-cases where people have several certificates it's all too common to have different subject-dns but same email-addresses.

People renew their certificates all the time with different subject dn (for example change of ou= or something like that) and the same email address.

Even in multi-key scenarios (see comment 6 from Kai) most CAs will set different DNs for the different certificates (otherwise the owner of the certificates would not be able to distinguish them with current UIs).

So I absolutely agree with Kaspars comment 11: the right way would be to adress bug 337433, and then use the new CERT_FindCertByNicknameOrEmailAddrAndUsage() instead of the current calls to CERT_FindCertByNicknameOrEmailAddr(), CERT_CreateSubjectCertList() and CERT_FilterCertListByUsage()

Comment 17

[Jürgen Brauckmann]

To continue my last comment:

Another approach is to get a list of all certificates with matching email address, and then loop through them and filter out all which are expired or have the wrong usage.

In Bug 596215 I'm proposing an NSS function PK11_FindCertsFromEmailAddress(), which simply returns all certificates with matching email address.

In Bug 596221 I'm demonstrating how this could be used for nsNSSCertificateDB::FindCertByEmailAddress().

This way, nsNSSCertificateDB::FindCertByEmailAddress() does not have to rely on any connection between SubjectDN and mail address. 

I think that this approach is much more robust than the present code with its usage of CERT_CreateSubjectCertList(). The present code simply has too many options to fail if people  get renewed certificates with different subject-dns and same email addresses. (or same subject-dns and different email addresses, or ...).

Thanks for reading my rather long comments... .

Comment 18

[Kai Engert (:KaiE:)]

Jürgen, I appreciate your work.

It's getting a little confusing with all the work in parallel, reviewed patches, and waiting for them to land.

We have several already reviewed patches, that got delayed, and we have the new proposals.

Today, I've checked in bug 337433 for NSS 3.12.9

I've prepared bug 337430 which we'll be able to land very soon to Mozilla trunk (mozilla-central), after the next Firefox beta branching is complete.

Also, this bug has another reviewed patch. I'll address Honza's change requests in comment 14 and 15.

The patch in this bug does not conflict with the patch in bug 337430.

I propose we check in patches from this 337430 and this bug, and then base any other patches on top of them, if needed.

Comment 19

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Addressed Honza's review requests.

Comment 20

[Kaspar]

Kai, using the CERT_FindCertByNicknameOrEmailAddr/CERT_CreateSubjectCertList/CERT_FilterCertListByUsage/CertContainsEmailAddress combo still has the problem that PSM will sometimes fail to find a suitable certificate, even if it's available in the DB (in this case, the only "improvement" of this patch is that it won't return a certificate with a non-matching address, but no certificate at all).

Jürgen's work in bug 596215 and 596221 is really a comprehensive solution to all the issues with nsNSSCertificateDB::FindCertByEmailAddress, so I would very much appreciate your help in getting them added for 3.12.9.

Comment 21

[Kai Engert (:KaiE:)]

We believe this has been fixed by bug 596221.