Last Comment Bug 596221 - PSM should use new function to find encryption certificates of email recipients
: PSM should use new function to find encryption certificates of email recipients
Status: RESOLVED FIXED
[duptome]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
: -- enhancement with 2 votes (vote)
: mozilla7
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on: 596215
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-14 07:24 PDT by Jürgen Brauckmann
Modified: 2012-01-18 04:36 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
nsNSSCertificateDB.cpp.patch (2.11 KB, patch)
2010-09-16 09:27 PDT, Jürgen Brauckmann
kaie: review-
Details | Diff | Splinter Review
Patch v2 (3.17 KB, patch)
2011-05-30 13:19 PDT, Kai Engert (:kaie)
rrelyea: review+
Details | Diff | Splinter Review
Patch v2 - fixed [checked in] (3.20 KB, patch)
2011-06-28 05:23 PDT, Kai Engert (:kaie)
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jürgen Brauckmann 2010-09-14 07:24:41 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.2.8) Gecko/20100723 SUSE/3.6.8-1.1.1 Firefox/3.6.8
Build Identifier: 

Currently, PSM uses a three step process of finding an encryption certificate for email recipients:

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).), 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).

This behaviour can lead to all sort of problems, see e.g. bug 558337 or bug 337430.


It would be far better if PSM uses just one function to retrieve all valid certificates for an email address, and would then select the best certificate itself, without any indirection through subject-dn-lookups and the like.

bug 596215 suggests the implementation of such a function.



Reproducible: Always
Comment 1 Jürgen Brauckmann 2010-09-14 07:34:29 PDT
Sorry, I posted a garbled description.

The middle paragraph was supposed to be:

====
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 2 Jürgen Brauckmann 2010-09-16 09:27:17 PDT
Created attachment 475875 [details] [diff] [review]
nsNSSCertificateDB.cpp.patch

Using the modifications to the NSS API proposed in bug 596215, this patch provides a cleaner way for PSM to search for certificates for email receipients.

With this patch, PSM does not get confused by certificates with identical subject-dn but different email address (see bug 558337, bug 337430 or bug 594297#c14)
Comment 3 Kai Engert (:kaie) 2010-09-24 05:51:10 PDT
Jürgen, thanks for starting to work on this bug and bug 596215.

However, the patches in bug 337430 and bug 337433 have already been reviewed and I've planned to add them shortly.

If I understand correctly, they are different approaches to the same problem.
Comment 4 Kaspar Brand 2010-09-26 22:13:55 PDT
Confirming as an enhancement (depends on a new NSS function proposed in bug 596215, as mentioned above).
Comment 5 Jürgen Brauckmann 2010-10-08 06:39:50 PDT
(In reply to comment #3)
> However, the patches in bug 337430 and bug 337433 have already been reviewed
> and I've planned to add them shortly.
> 
> If I understand correctly, they are different approaches to the same problem.

Kai: If I understand it correctly, bug 337430 deals with problems finding the encryption certificate of the sender of an encrypted email, so Thunderbird can encrypt a copy of the mail readable for the sender?

In bug 337430, you created a patch for nsNSSCertificateDB::FindEmailEncryptionC ert, which I think is only called for finding the encryption certificate of the sender of an encrypted email. Please correct me if I'm wrong... .

This bug would enhance finding certificates for the recipients minus the sender of an encrypted email. For this task nsNSSCertificateDB::FindCertByEmailAddress is used, called by several functions in mailnews/extensions/smime/src, e.g. nsSMimeJSHelper::GetRecipientCertsInfo or nsMsgComposeSecure::MimeCryptoHackCerts.
Comment 6 Konstantin Andreev 2011-03-23 07:01:59 PDT
Too bad, but PSM shall *NOT* choose encryption certificates - it unable to do this correctly. The selection should be done at the CMS/SMIME (NSS) level.

The trouble is - the different certificates support different content encryption algorithms, but a message can have only one one.

Let me illustrate by example.

  Recipient X: CertX1(a,b,c) CertX2(b,c,d)
  Recipient Y: CertY(d,e) 
  Sender     : CertS(a,d) 

The a,b,c,d,e - a content encryption algorithms, supported by particular certificate.

You can easily see that only CertX2 satisfy "common content encryption algorithm" criteria, that's 'd' algorithm.

Certainly, PSM do know nothing about content encryption, and should not know. The selection of the right certificate is pertinent to the CMS layer.

Worth mentioning that certificate encryption capabilities may be restricted by "SMIME capabilities" on per recipient and per recipient's MUA basis.
Comment 7 Jürgen Brauckmann 2011-03-23 10:15:03 PDT
PSM chooses encryption certificates today. PSM currently searches and selects certificates for encryption by a mixture of subject dn and email address. Sometimes it fails; bug 558337 is a nice example what can go wrong.

This enhancement request aims at making that existing stuff better to help solve actual bugs. It does not introduce new responsibilites into PSM, and does not change the way PSM or NSS care for the correct algorithm/certificate set.

Your comment aims at making PSM better at choosing the right algorithm for all receipients. Thats fine, but should be subject to broader architectural considerations outside this bug, I think.
Comment 8 Kai Engert (:kaie) 2011-05-30 13:17:46 PDT
Comment on attachment 475875 [details] [diff] [review]
nsNSSCertificateDB.cpp.patch

Review of attachment 475875 [details] [diff] [review]:
-----------------------------------------------------------------

::: comm-1.9.2/mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1453,4 @@
>  nsNSSCertificateDB::FindCertByEmailAddress(nsISupports *aToken, const char *aEmailAddress, nsIX509Cert **_retval)
>  {
>    nsNSSShutDownPreventionLock locker;
> +  CERTCertificate *any_cert;

any_cert is no longer necessary, let's remove it

@@ +1453,5 @@
>  nsNSSCertificateDB::FindCertByEmailAddress(nsISupports *aToken, const char *aEmailAddress, nsIX509Cert **_retval)
>  {
>    nsNSSShutDownPreventionLock locker;
> +  CERTCertificate *any_cert;
> +  CERTCertListNode *node;

let's move "node" down to where it's used.

@@ -1459,5 @@
>  
>    CERTCertificateCleaner certCleaner(any_cert);
>      
> -  // any_cert now contains a cert with the right subject, but it might not have the correct usage
> -  CERTCertList *certlist = PK11_FindCertForEmailAddress(aEmailAddress, nsnull);

your patch confused me. I don't understand which revision your patch is against, because I couldn't find an old revision that uses PK11_FindCertForEmailAddress. But let's ignore that.

@@ +1471,2 @@
>    if (SECSuccess != CERT_FilterCertListByUsage(certlist, certUsageEmailRecipient, PR_FALSE))
>      return NS_ERROR_FAILURE;

this becomes unnecessary, I'll explain below.

@@ +1478,5 @@
> +  for (node = CERT_LIST_HEAD(certlist); !CERT_LIST_END(node,certlist);
> +  			node = CERT_LIST_NEXT(node)) {
> +	if (CERT_CheckCertValidTimes(node->cert,  PR_Now(), PR_FALSE) == secCertTimeValid)
> +		// found a valid certificate
> +		break;

I would like to propose: let's go one step further.

The intention of this function is to return one email certificate. It's not clearly said so in the API, but we should return the very best certificate we can find.

Your proposal to check for expired certs is good, but there may be other reasons why a cert is still not valid. For example, someone might own a cert from a CA that has meanwhile been marked as not trusted. Maybe that's unlikely, but it's possible.

Therefore I propose, let's simply check for complete certificate validity, and skip any certs that aren't valid.

I will attach a cert that does it.
Comment 9 Kai Engert (:kaie) 2011-05-30 13:19:02 PDT
Created attachment 536155 [details] [diff] [review]
Patch v2
Comment 10 Kai Engert (:kaie) 2011-05-30 13:42:46 PDT
Comment on attachment 536155 [details] [diff] [review]
Patch v2

Some comments to make reviewing easier:

>+  
>+  nsCOMPtr<nsINSSComponent> inss;
>+  nsRefPtr<nsCERTValInParamWrapper> survivingParams;
>+  
>+  if (nsNSSComponent::globalConstFlagUsePKIXVerification) {
>+    inss = do_GetService(kNSSComponentCID, &nsrv);
>+    if (!inss)
>+      return nsrv;
>+    nsrv = inss->GetDefaultCERTValInParam(survivingParams);
>+    if (NS_FAILED(nsrv))
>+      return nsrv;
>+  }


This block is copied from elsewhere. It simply prepares the parameters that we reuse when calling CERT_PKIXVeryCert.


>+  CERTCertList *certlist = PK11_FindCertsFromEmailAddress(aEmailAddress, nsnull);

Call the recently contributed function.

>-  if (SECSuccess != CERT_FilterCertListByUsage(certlist, certUsageEmailRecipient, PR_FALSE))

No longer filter by usage - that part is now done by the verify function.


>+  CERTCertListNode *node;
>+  // search for a valid certificate
>+  for (node = CERT_LIST_HEAD(certlist);
>+       !CERT_LIST_END(node, certlist);
>+       node = CERT_LIST_NEXT(node)) {


loop the list of certs that match the email address


Now, based on our global pref, use either classic or PKIX verify, and break as soon as we find something good. If found, return it


>+
>+    if (!nsNSSComponent::globalConstFlagUsePKIXVerification) {
>+      if (CERT_VerifyCert(certdb, node->cert,
>+          PR_TRUE, certUsageEmailRecipient, PR_Now(), nsnull, nsnull) == SECSuccess) {
>+        // found a valid certificate
>+        break;
>+      }
>+    }
>+    else {
>+      CERTValOutParam cvout[1];
>+      cvout[0].type = cert_po_end;
>+      if (CERT_PKIXVerifyCert(node->cert, certificateUsageEmailRecipient,
>+                              survivingParams->GetRawPointerForNSS(),
>+                              cvout, nsnull)
>+          == SECSuccess) {
>+        // found a valid certificate
>+        break;
>+      }
>+    }
>+  }
>+
Comment 11 Ludovic Hirlimann [:Usul] 2011-05-30 21:14:45 PDT
Shouldn't some of these comments go into the code ?
Comment 12 Robert Relyea 2011-06-01 09:37:19 PDT
Comment on attachment 536155 [details] [diff] [review]
Patch v2

r+

BTW, there has been requests out there to support certificates which do not have email addresses. We may want to also search the address book for a certificate based on email address, but that is a completely separate bug and separate issue (I only mention it because it may be attached to this code here).

This patch is a big improvement on what we currently have.

bob
Comment 13 Jürgen Brauckmann 2011-06-07 07:10:56 PDT
I think it's good that your patch does real verification of certificates instead of just checking the validity dates.
Comment 14 Kai Engert (:kaie) 2011-06-28 05:20:37 PDT
I tested this patch.

First, I tried to reproduce bug 558337 without this patch. I used a currently valid dual-key-certificate, and an expired single-key-certificate with the same subject. Both with and without this patch, we succeed to find the valid encryption-only cert.

(It might take a special internal ordering in the internal NSS to trigger bug 558337.)

Second, I tried to bug 337430 with the old code. I was able to reproduce the bug, using dual-key-certs with different subject.

Then I used the patch, and I confirm this patch fixes the bug.

In addition, I imported an older, expired cert for the same email address, and it still worked.
Comment 15 Kai Engert (:kaie) 2011-06-28 05:23:12 PDT
Created attachment 542432 [details] [diff] [review]
Patch v2 - fixed [checked in]

This is a fixed version of patch v2 - it adds a forgotten variable declaration.

As mentioned in the previous comment, I have tested this code.
Comment 16 Kai Engert (:kaie) 2011-06-28 05:29:15 PDT
Jürgen, thanks again for your contribution.

Checked in.
http://hg.mozilla.org/mozilla-central/rev/61c7d0cae035
Comment 17 Ian Neal 2011-06-28 05:38:25 PDT
Comment on attachment 542432 [details] [diff] [review]
Patch v2 - fixed [checked in]

Would be good to get this into aurora before the next merge so that it can make TB6 and SM2.3
Comment 18 Kai Engert (:kaie) 2011-06-28 05:42:01 PDT
(In reply to comment #17)
> 
> Would be good to get this into aurora before the next merge so that it can
> make TB6 and SM2.3

I agree, because it is a correctness patch, and fixes annoying usability issue.

In addition I would like to clarify:

This patch is NOT called from anywhere by stock Firefox.

It's ONLY used by email encryption code.
Comment 19 Mark Banner (:standard8) (afk until 26th July) 2011-06-29 06:33:31 PDT
Checked into aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/2b387d17d7cb
Comment 20 chokito 2011-09-24 07:12:38 PDT
And when will be fixed in SeaMonkey?
In SeaMonkey 2.4 is this not fixed.
Comment 21 chokito 2011-12-20 06:40:42 PST
In SeaMonkey 2.5 and 2.6 is this not fixed.
Comment 22 Kai Engert (:kaie) 2012-01-10 10:15:47 PST
Urs, I really believe the original issue is fixed. Until we prove differently, please let's assume that your problem is another variation.

It would be best if you filed a new bug and describe your issue, so we can keep the issues and discussions separate.

Please feel free to send me a S/MIME signed email with a problematic certificate that doesn't work for you.
Comment 23 chokito 2012-01-18 04:36:38 PST
Kai, sorry that it took so long to answer.
The problem is, when I have two or more certificates with the same subject (CN) but different e-mail addresses. I can then choose only one of them in the "Select Certificate" window! 
See https://bugzilla.mozilla.org/show_bug.cgi?id=278689

Note You need to log in before you can comment on or make changes to this bug.