278689 - Multiple Certificates with the same subject are not shown in the digital signature select cert combo (only one is shown)

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Michael Østergaard Pedersen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041111 Firefox/1.0 (JTw)
Build Identifier: 

If you have two different certificates (from the same or different CAs, doesn't
matter) with the same subject, both of them show up in the certificate manager
dialog under personal certificates. However, only one is available in Mail &
NewsGroup Account Settings\Security\Digital signing\certificate select combo.

Both are valid certificates. If I only import one of them I am able to sign a
mail with it.

Since certificates in the NewsGroup Account Settings\Security\Digital
signing\certificate select combo are postfixed by the serial number I would
still be able to select the right one if both were shown, but as it is now, if I
import two different certificates with the same CA only the first one I imported
is shown.

I think this happens more often than one should think because the subject is
often a persons full name and a person can have certificates from different CAs.

Using Thunderbird 1.0

Reproducible: Always

Steps to Reproduce:
1. Import two different certificates with the same subject
2. Selecting Mail & Newsgroup Account settings, security, Digital signing 
3. Press select
Actual Results:  
In the certificate combo box only one certificate is shown

Expected Results:  
Both certificates should have been shown. They are postfixed by the serial
number in the combo box so they can still be distinguished..

Comment 1

[Mike Cowperthwaite]

*** Bug 248907 has been marked as a duplicate of this bug. ***

Comment 2

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 3

[Michael Østergaard Pedersen]

The problem still present in Thunderbird 1.5b1

Comment 4

[Michael Østergaard Pedersen]

It is not even possible to corrent manually since in the prefs.js file the
certificate is identitied by the subject like this:

user_pref("mail.identity.id1.encryption_cert_name", "Subject Name");

This should be something unique for each certificate even from different CAs.

Comment 5

[Ionut Marasescu]

*** Bug 248907 has been marked as a duplicate of this bug. ***

Comment 6

[Michael Østergaard Pedersen]

It surprises me that no one has confirmed this. I talk to people on a weekly basis who are annoyed by this issue. The common scenario I experience, is that people have two e-mail addresses that forward to the same account, and both of them can receive encrypted e-mail, hence they have two certificates issued to the same person (same Common Name) but different e-mail addresses.

They want to send signed e-mail from only one of these addresses (or they use multiple identitities in Thunderbird and would like to use a different certificate depending on the identity they have selected) but due to this problem they can only select one of them in the "Account Settings\Security\Digital signing\certificate select combo"

In prefs.js the Common Name is listed as described in the above posting, so just specifying it here, it's a game of luck to see if Thunderbird picks the right certificate. I haven't been able to figure out which one it picks. Perhaps it has something to do with the order in which the certificates are imported.

My proposed solution is:

In the "Account Settings\Security\Digital signing\certificate select combo", certificates should be listed under their respective CAs with the common name and serial number. This will allow the user to uniquely identify each certificate.

Internally (in prefs.js for example) Thunderbird should use something that is unique for each certificates (which the Common Name isn't). Perhaps the SHA1 fingerprint of the certificate would be useful here.

Comment 7

[Mike Cowperthwaite]

Confirming based on dupes -- assuming I made a correct dupe at comment 1.
I'm not using certificates -- I know very little about any of this domain -- so I can't do any testing.

Comment 8

[Michael Østergaard Pedersen]

I am going to attempt fixing this myself since this problem is driving me nuts. The problem is that I don't know where in the source to start looking. Anyone able to point me in the right direction? Looking for the code that picks the signing and encryption certificates, gets the text that is displayed in the dialog box and also how this selection is loaded and saved to prefs.js

Comment 9

[Michael Østergaard Pedersen]

I have now found the cause of this bug, and how it can quite easily be fixed. Unfortunately I am not yet able to build Thunderbird on my machine, so I have not tested the proposed solution.

Certificates are identified by nicknames, but these nicknames are not unique. The code actually has functions to take care of ensuring unique nicknames, but it fails under one circumstance. Namely if a certificate do have a DN, but where the nickname (taken as the CN from the certificate) matches the nickname of another certificate. This is a bug, and actually conflicting to what is written in a comment in nsPKCS12Blob.cpp:

/* validate cert nickname such that there is a one-to-one relation 
   between nicknames and dn's.  we want to enforce the case that the
   nickname is non-NULL and that there is only one nickname per DN.
   
In the same file there is a function "sec_pkcs12_validate_cert_nickname" that validates the cert nickname. The problem is that this function never tries to handle conflicting nicknames in case it can retrieve the nickname from the DN. If so, it just accepts that nickname, even though other certificates in the database might have the same nickname. If no nickname can be retrieved from DN, a callback function called "nsPKCS12Blob::nickname_collision" is called to handle this, but all it does is to pick a default nickname, and keep appending #1, #2, etc. to it until it is unique.

Here is what should be done IMHO:

Even when the nickname is extracted from the DN (in "sec_pkcs12_validate_cert_nickname"), check for conflicts. If there is a conflict then use the callback function to assign a new nickname that does not conflict with other nicknames.

This is a small and easy change, but the problem is that the callback function is incomplete. It has a parameter to accept the current conflicting nickname, but that parameter is never used (since the function (right now) is always called when there is no available nickname from the certificate). As described above, the callback function only picks a default nickname and makes sure that it is unique. What the callback function should do is the following:

Instead of assigning #1, #2, ... to a default nickname and waiting until we get one that is unique, do it to the conflicting nickname in case it's not empty. If it is empty, then just do it to the default nickname as it currently does. This is also a small fix as I see it.

With these two changes I believe this bug should be solved, since nicknames will always be unique.

Actually this is a bug in NSS, not Thunderbird.

Comment 10

[Michael Østergaard Pedersen]

After talking to some NSS people, I found out that changing the way nicknames are used is not an option. What should be done is to change the way Thunderbird gets the certificates and identifies them.

Instead of identifying and fetching them by nickname, it should use the issuer and serial number, which is unique for each certificate. There is even a method in NSS for retrieving certificates by these values.

Comment 11

[Michael Østergaard Pedersen]

I have produced a fix that works. I ended up changing the way to identify certificates. Instead of using the nickname, I use the dbKey which is unique among all certificates. I will describe my solution here in case anyone is interested.

First edit am-smine.js, find the following:

x509cert = picker.pickByUsage(window,
           certInfo.value,
           certUsage, // this is from enum SECCertUsage
           false, false, canceled);
           
and change it to

x509cert = picker.pickByUsage(window,
           certInfo.value,
           certUsage, // this is from enum SECCertUsage
           false, true, canceled);

This will allow certificates with duplicate nicknames to show up in the selction box. Then in the same file, replace all occurences of nickname with dbKey

Now the only change needed is to make sure that the certificates can be retrieved from the database by the dbKey instead of nickname. This means editing two files:

nsIX509CertDB.idl
-----------------

Find

/**
 *  Find the email encryption certificate by nickname.
 *
 *  @param aNickname The nickname to be used as the key
 *                   to find the certificate.
 *                
 *  @return The matching certificate if found.
 */
nsIX509Cert findEmailEncryptionCert(in AString aNickname);

/**
 *  Find the email signing certificate by nickname.
 *
 *  @param aNickname The nickname to be used as the key
 *                   to find the certificate.
 *                
 *  @return The matching certificate if found.
 */
nsIX509Cert findEmailSigningCert(in AString aNickname);

and replace with:

  /**
   *  Find the email encryption certificate by dbKey.
   *
   *  @param adbKey The dbKey to be used as the key
   *                   to find the certificate.
   *                
   *  @return The matching certificate if found.
   */
  nsIX509Cert findEmailEncryptionCert(in AString adbKey);

  /**
   *  Find the email signing certificate by dbKey.
   *
   *  @param adbKey The dbKey to be used as the key
   *                   to find the certificate.
   *                
   *  @return The matching certificate if found.
   */
  nsIX509Cert findEmailSigningCert(in AString adbKey);
  
nsNSSCertificateDB.cpp
----------------------

Replace the two functions

FindEmailEncryptionCert
FindEmailSigningCert

with:

/* nsIX509Cert getDefaultEmailEncryptionCert (); */
NS_IMETHODIMP
nsNSSCertificateDB::FindEmailEncryptionCert(const nsAString &adbKey, nsIX509Cert **_retval)
{
  if (!_retval)
    return NS_ERROR_FAILURE;

  *_retval = 0;

  if (adbKey.IsEmpty())
    return NS_OK;

  nsNSSShutDownPreventionLock locker;
  nsresult rv = NS_OK;
  CERTCertificate *cert = nsnull;  
  nsNSSCertificate *nssCert = nsnull;
  nsIX509Cert *x509Cert = 0;
  
  char *asciikey = NULL;
  NS_ConvertUCS2toUTF8 aUtf8dbKey(adbKey);
  asciikey = NS_CONST_CAST(char*, aUtf8dbKey.get());

  /* Find a good cert in the user's database */
  FindCertByDBKey(asciikey, NULL, &x509Cert);
  if (!x509Cert) { goto loser; }
  nssCert = NS_STATIC_CAST(nsNSSCertificate*, x509Cert);

  /* Check that the certificate we found can be used
   * 
   * This has been commented out for three reasons
   * 1. Mail/News, certificates found from that function
   *    are always of the correct type.
   * 2. CERT_CheckKeyUsage gives a linker error.
   * 3. If the usage turns out to be invalid, we need to free
   *    nssCert again somehow
   
  cert = nssCert->GetCert();
  if (CERT_CheckKeyUsage(cert, certUsageEmailRecipient) != SECSuccess) {
  	goto loser;
  }
  */
  NS_ADDREF(nssCert);

  *_retval = x509Cert;
loser:
  return rv;
}

/* nsIX509Cert getDefaultEmailSigningCert (); */
NS_IMETHODIMP
nsNSSCertificateDB::FindEmailSigningCert(const nsAString &adbKey, nsIX509Cert **_retval)
{
  if (!_retval)
    return NS_ERROR_FAILURE;

  *_retval = 0;

  if (adbKey.IsEmpty())
    return NS_OK;

  nsNSSShutDownPreventionLock locker;
  nsresult rv = NS_OK;
  CERTCertificate *cert = nsnull;  
  nsNSSCertificate *nssCert = nsnull;
  nsIX509Cert *x509Cert = 0;
  
  char *asciikey = NULL;
  NS_ConvertUCS2toUTF8 aUtf8dbKey(adbKey);
  asciikey = NS_CONST_CAST(char*, aUtf8dbKey.get());

  /* Find a good cert in the user's database */
  FindCertByDBKey(asciikey, NULL, &x509Cert);
  if (!x509Cert) { goto loser; }
  nssCert = NS_STATIC_CAST(nsNSSCertificate*, x509Cert);
    	
  /* Check that the certificate we found can be used
   * 
   * This has been commented out for three reasons
   * 1. Mail/News, certificates found from that function
   *    are always of the correct type.
   * 2. CERT_CheckKeyUsage gives a linker error.
   * 3. If the usage turns out to be invalid, we need to free
   *    nssCert again somehow
   
  cert = nssCert->GetCert();
  if (CERT_CheckKeyUsage(cert, certUsageEmailSigner) != SECSuccess) {
  	goto loser;
  }
  */
  NS_ADDREF(nssCert);

  *_retval = x509Cert;
loser:
  return rv;
}

And that is it. The last change is to make the lookup of certificates be done after the dbKey and not nickname as before. If other programs use these functions in NSS with the nickname, they will not work anymore. Probably a better idea to add two new functions with alternative names (like FindEmailSigningCertFromDbKey), instead of replacing the existing functions, and then let Thunderbird use these two new functions.

As you can see in the code, there are some comments whether or not the usage on the certificates should be checked and how. I didn't mess more with that, because honestly I don't know much about the internals of NSS.

Another side-effect of this patch is that users don't see the nickname in the text boxes in Thunderbird, but instead the database key. One solution here could be to also keep the nickname just for the purpose when it should be displayed to the user, and then use the database key for all other purposes.

At least this fixed the problem for me, and is in line with what I was told by someone working on NSS, that I should just use another way of identifying certificates instead of the nickname.

Finally I should mention that this was the 2.0a1 version of Thunderbird, updated to the latest (as of 2006-10-29) CVS version.

Comment 12

[Mike Cowperthwaite]

Great!  Use   cvs diff   to generate a patch file, upload it here, and request a review -- altho I don't know who the best reviewer might be.  I guess, start by requesting from David -- [e-mail] -- and if he doesn't feel comfortable reviewing it, he'll know how to find who should.

Do you wish to assign this bug to yourself?

Comment 13

[Michael Østergaard Pedersen]

Attachment: Proposed patch

A suggestion that fixes the mentioned problem in this bug report. However there are some commented code that need to be fixed before this patch is complete. See the comments for more details.

Comment 14

[Michael Østergaard Pedersen]

Here is the proposed patch. It works, but there is room for improvements as mentioned in comment #11.

#12: Yes, just assign the bug to me. I will do my best to get in fixed in a proper way

Comment 15

[timeless]

Comment on attachment 244012 [details] [diff] [review]
Proposed patch

notes :
1. you need to generate new uuids for each idl interface you change 
2. please confirm you fixed all callers, including  mozilla/mailnews
3. arguments should be aArg not _retval. 
4. the const cast is wrong use .BeginWriting or fix the signature 
5. please don't stick code into comments. use #ifdef FOO_WORKING or similar  
6. use  nsnull not 0 for pointers 
7. set flags to ? to request reviews 
8. please don't use tabs even in comments 
9. do you really want to return NS_OK when find cert fails ?

Comment 16

[Michael Østergaard Pedersen]

Thanks for your comments. Just trying to 

1. How do I do that? Any pointers to places where I can read about how to make proper changes to the source code?

2. I can confirm that. Don't know if the proper fix is to add new methods or change the existing methods. In the patch, I added new methods?

3. arguments should be aArg not _retval. 

I based my new function on the existing function nsNSSCertificateDB::FindEmailEncryptionCert(const nsAString &aNickname, nsIX509Cert **_retval) which uses _retval. Should I also change this and all the other function in nsNSSCertificateDB.cpp that already uses _retval?

9. do you really want to return NS_OK when find cert fails ?

Yes, this is the behaviour of the original function I used as a template. _retval is 0 and the function returns NS_OK

Comment 17

[David :Bienvenu]

Comment on attachment 244012 [details] [diff] [review]
Proposed patch

I'm going to cancel the sr request since it sounds like we're going to need a new patch anyway, and I'd definitely like to see Kaie's input.

Comment 18

[Michael Østergaard Pedersen]

I am going to add a new patch later today, taking the notes from #15 into account, and also making some other changes.

Comment 19

[Michael Østergaard Pedersen]

Attachment: Updated patch

This fixes the bug by doing a lookup by dbKey instead of nickname which is not always unique.

I have modified the existing methods 

FindEmailSigningCert
FindCertByEmailAddress

to use dbKey instead of nickname, since these methods are only used in Mail/News, so if we are changing something here, it makes no sense to leave the original methods IMHO.

The patch is still missing two things

1. It does not check if the key found actually has the right usage
2. It does not free memory when it exists

These should be easy to fix for someone more familiar with the source code than I am. I just hope that I have at least made it a bit easier for someone to fix this problem.

I am still using retval instead of aArg, since several methods do that in nsNSSCertificateDB.cpp. Also some of them use 0 instead of nsnull.

There are two issues with this patch

1. It will change the text showing the currently selected certificates to showing the dbKey instead of nickname, which is not that user friendly. If this is a problem it is probably possible to use the dbKey for selection, but showing the nickname to the user.

2. Users upgrading with an old configuration file, needs to select their certificates again for the right value to be stored in prefs.js

Comment 20

[Kai Engert [:KaiE:]]

Comment on attachment 244143 [details] [diff] [review]
Updated patch

Michael, we must not change any interfaces that are marked FROZEN in the IDL file, because of that I have to reject your patch.

One solution is to make a new interface function available in a not-yet-frozen interface. Maybe you can look at and extend nsIX509CertDB2?

You are asking why attempting to use CERT_CheckKeyUsage gives you a link error. This is because this function is not part of the external API of NSS, it is not exported in file nss.def

Comment 21

[Kai Engert [:KaiE:]]

(In reply to comment #19)
> 1. It will change the text showing the currently selected certificates to
> showing the dbKey instead of nickname

We should not do that.

Comment 22

[Michael Østergaard Pedersen]

> Michael, we must not change any interfaces that are marked FROZEN in the IDL
> file, because of that I have to reject your patch.

I should have guessed that was what FROZEN meant. It just seemed like a stupid idea to create some function for the exact same purpose in another file and leave two now completely unused functions in this interface, but I get the point.

> One solution is to make a new interface function available in a not-yet-frozen
> interface. Maybe you can look at and extend nsIX509CertDB2?

I will try to look at that later.

> You are asking why attempting to use CERT_CheckKeyUsage gives you a link error.
> This is because this function is not part of the external API of NSS, it is not
> exported in file nss.def

Would it be okay to export in then or what are the rules for that?

>> 1. It will change the text showing the currently selected certificates to
>> showing the dbKey instead of nickname

>We should not do that.

I agree, it looks really stupid (almost as stupid as "Imported Certificate #1" :) It can probably be changed to still display the nickname, while using the dbKey internally.

As long as the idea of looking up the certificate by dbKey is not rejected, then I will pursue this when I get the time. If everything fails, I can always use the patch on my own systems.

Comment 23

[Kai Engert [:KaiE:]]

(In reply to comment #22)
> > You are asking why attempting to use CERT_CheckKeyUsage gives you a link error.
> > This is because this function is not part of the external API of NSS, it is not
> > exported in file nss.def
> 
> Would it be okay to export in then or what are the rules for that?


You could file a bug against NSS / Libraries and request that it gets exported. The NSS team will then look at your request.

Add a dependency between this and your new bug.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

The described behavior is by design.  

The design of X.500 (and hence X.509) prevents two CAs from ever issuing
certs with the same subject name.  Of course, there are many people who 
play at being CAs who don't follow the rules, and they might violate that.
But mozilla's crypto security is designed to work with real by-the-book 
X.509 certs and CAs, not play certs and CAs.

If a single CA issues multiple certs with the exact same subject name, 
then they should be distinguished by either 
a) different key usages (e.g. signing vs encryption key encipherment), or
b) different validity periods,
and should be interchangable in all other respects.  

That is why NSS maps subject names to "nicknames" (a.k.a. friendly names)
and lets the user choose among nicknames.  If the CAs are playing X.509 by
the rules, then NSS should be able to choose a proper cert automatically
based on just the nickname.  

So, this bug is really an RFE, requesting a change in the PKI security model
used in mozilla.  Such a change involves a lot more than merely writing a 
patch.  It requires agreement that this new model is the right one for 
mozilla products to use.  I think any discussion of this subject is likely
to start with these questions:  Why can't the CAs follow the rules? and 
Why doesn't the user get a cert from a CA that follows the rules?

Comment 25

[Alaric]

well, lets see, if we check some certs... like my Thawte code signing certs subject...

CN = Pengdows, Inc.
OU = Secure Application Development
O = Pengdows, Inc.
L = Omaha
S = Nebraska
C = US

There is nothing in there that would make it unique to the CA. Or what about 2 different certs for totally DIFFERENT emails that share the same private key.  I am attaching 2 cert chains that Thunderbird can't tell apart, 1 is from Thawte, the other is from StartCom, both recognized CAs. 

The point is this, the subject can't be guaranteed to be unique, a better thought pattern is needed.  Thus its a design bug, not an RFE.  

Comment 26

[Alaric]

Attachment: Cert 1 (Thawte Cert)

Comment 27

[Alaric]

Attachment: Cert 2(StartCom)

Comment 28

[Wan-Teh Chang]

I'm also surprised that the design of X.509 prevents two CAs
from ever issuing certs with the same subject name.  How do
independent CAs coordinate to ensure that?

Comment 29

[Kaspar]

Attachment: Preliminary patch to support cert selection by subject (nickname) AND serial number

(In reply to comment #28)
> I'm also surprised that the design of X.509 prevents two CAs
> from ever issuing certs with the same subject name.  How do
> independent CAs coordinate to ensure that?

Maybe by looking them up in that one and only, world-encompassing, single-rooted X.500 directory the ITU people were dreaming of in the 90s...?

No, seriously: I think this problem here can be addressed without the need for redesigning Mozilla's security model. What Michael wrote in comment #10 still holds true, IMO:

> What should be done is to change the way Thunderbird
> gets the certificates and identifies them.
> 
> Instead of identifying and fetching them by nickname, it should use
> the issuer and serial number, which is unique for each certificate.

I'm attaching a preliminary patch which tries to accomplish that. No changes to NSS needed (neither to PSM), it only modifies the technique Thunderbird (or Seamonkey, for that matter) uses to select the signing and encryption certificates. Specifically, it

- allows to select individual certs with the same subject DN

- additionally stores the dbKey in the prefs file ("signing_cert_dbkey"/"encryption_cert_dbkey") after a cert has been selected

- uses PSM's FindCertByDBKey() instead of FindEmailSigningCert()/FindEmailEncryptionCert() to retrieve the "correct" cert if a dbKey is stored in the prefs file

By adding a separate preference for the dbkey, I think that we're actually maintaining backwards compatibility (if no "*_dbkey" preference is stored, we still stick to the "old" behavior).

The classic example where Thunderbird's/Seamonkey's current selection mechanism isn't good enough is the case where a user has separate certs for digitalSignature and nonRepudiation, both having the same subject (which is perfectly acceptable, I believe). With the current code, it's simply not possible to explicitly select either of these two certificates for signing... you will always end up with an "automagically" selected cert (FindEmailSigningCert() calls CERT_FindUserCertsByUsage(), which calls CERT_CreateSubjectCertList(), which creates the list by calling CERT_AddCertToListSorted(), which in turn depends on the outcome of  CERT_SortCBValidity(), cf. http://lxr.mozilla.org/seamonkey/ident?i=CERT_SortCBValidity).

I will be attaching two dummy certs so that those interested in observing what's happening in detail can give it a try themselves. If anybody manages to sign a message using the cert having serial ...:A0:FF (while both certs are installed in the cert DB) with the current version of Thunderbird or Seamonkey, then I'd really be surprised ;-)

If people think that this is the right way to address the issue, please let me know (I would do further testing and try to improve the patch, if necessary). Also, please advise whom I should ask for review in this case (it's no longer a PSM issue, right?).

Comment 30

[Kaspar]

Attachment: Screenshot showing cert selection in Thunderbird, w/o and with patch applied

Comment 31

[Kaspar]

Attachment: Sample user certificate (including private key), digitalSignature only (no passphrase set)

Comment 32

[Kaspar]

Attachment: Sample user certificate (including private key), nonRepudiation only (no passphrase set)

Comment 33

[Kaspar]

Attachment: Issuing CA certificate and private key (no passphrase set, for those wanting to create additional user certs - NOT required for message signing tests)

Comment 34

[Kaspar]

Attachment: Root CA certificate and private key (no passphrase set, for those wanting to create additional issuing CA certs - NOT required for message signing tests)

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #28)
> I'm also surprised that the design of X.509 prevents two CAs
> from ever issuing certs with the same subject name.  

The original design prevented it, but as Kaspar noted, that design is
not the world in which we live.  But the present design still requires
it, and most major CAs understand this and take the simple steps to 
ensure it. 

> How do independent CAs coordinate to ensure that?

Look at the subject names in certs issued by (say) Verisign and Comodo 
to see how they do it.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Kaspar, when the user has multiple certs from the same issuer with the same subject name but different usages, the user should certainly NOT be the one 
to determine which cert is the right one.  The software should do that.  
The software should not present the user with any potentially wrong choices.
Yes, it's a bug that the software presently does not do that (very well),
but the answer is NOT to push that burden on to the user.  

I find your example certs rather contrived.  7 byte serial numbers is most
uncommon.  20 bytes is more common.  How does it look with "normal" certs?

Also, how does your code handle certs with no common name, or with very long 
common names?  

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

In reply to my own comment 35, I take it back.  The present design does
allow two different CAs to issue certs with the same subject name, 
provided that they don't overlap in time, OR that they are differentiated
in some other way that is relevant to cert path building, such as having
different policy OIDs.  That is how cross certification works.  

Comment 38

[Kaspar]

(In reply to comment #36)
> Kaspar, when the user has multiple certs from the same issuer with the same
> subject name but different usages, the user should certainly NOT be the one 
> to determine which cert is the right one.  The software should do that.

Why attempt to be smarter than the user? Just looking at the subject DN and the KU/EKU extensions to filter what certs should be selectable for the user isn't always sufficient... there are other extensions (think e.g. of subjectAltName, certificatePolicies etc.) where certificates can differ. Why should the user not be able to select the proper certificate in this situation?

> The software should not present the user with any potentially wrong choices.

I agree, it makes sense to filter out *inappropriate* certs. However, the two sample certs I attached actually illustrate a case where *both* can be used with S/MIME in a useful way:

- the one with digitalSignature can be used for authentication/integrity protection
- the one with nonRepudiation can be used to sign some sort of legally relevant/binding statement

> I find your example certs rather contrived.  7 byte serial numbers is most
> uncommon.  20 bytes is more common.  How does it look with "normal" certs?

Are you referring to the UI display (indicating the selected nickname)? If the nickname plus serial is too long, then it's just truncated at the right (the same happens with long nicknames currently).

The *serial numbers* of my certs are rather contrived, I agree... just have a look at the first line of their Base64 encoded form to find out why ;-) [makes it rather easy to distinguish between the two in this format, right?]

> Also, how does your code handle certs with no common name, or with very long 
> common names?

My code still relies on the nickname, no changes in this behavior (I just happened to use the commonName as the "friendly name" when I created the PKCS#12 files).

If the nickname does not fit into the text box, then yes, it's difficult (although not impossible) for the user to find out which cert is selected. But if truncated nicknames are really considered a problem, I think this should be the subject for a separate bug (the issue already exists right now, irrespective of whether we append the serial number or not).

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

> Why attempt to be smarter than the user?
Because most users aren't very ... uh ... sophisticated in this respect.
If users can get this stuff wrong, they will.  It's vital that we not
give users the chance to get it wrong.

> My code still relies on the nickname, no changes in this behavior 
OK, thanks for that clarification.  I thought you were switching from 
nickname to common name.  

Comment 40

[Kaspar]

(In reply to comment #39)
> If users can get this stuff wrong, they will.  It's vital that we not
> give users the chance to get it wrong.

Ok, fine. No disagreement here, definitely... we should not allow the user to select certificates which are not suitable for a specific usage - that's a no-brainer.

The point I was trying to make is that cases *will* exist where more than one certificate can be used for signing/encrypting (why would the cert picker be needed, otherwise?), and for these situations we need a change.

One case occurs when keys with different usages (digitalSignature and nonRepudiation, in our example) exist, where both keys can be reasonably used for S/MIME signing.

Another example would be two (or more) certs with the same subject DN, but with different e-mail addresses in their subjectAltName extensions. With the current code, the user has no way to choose one of these certs - he will only be able to use the one which gets automatically selected by NSS.

How to proceed, then? I would be willing to push ahead with the patch - but only if there's a good chance that its general idea (already previously suggested by the original reporter, see e.g. #comment 22) is being accepted by whoever will decide on this change.

If, on the other hand, the consensus among the NSS and PSM experts is that the cert picker mechanism should be left as is, then it would make sense to add a comment with such a statement to this bug and close it as invalid, IMO.

Comment 41

[Kai Engert [:KaiE:]]

At initial sight, it seems like a reasonable change to enable users to select a cert based on its serial number. After all, the UI already displays the serial number!

Can you please paste an example of such a dbkey? Just curious how it looks in the prefs.

You propose to lookup by dbkey first, and use it if found. Only fall back to nickname if dbkey didn't work.

I'm concerned about a regression in "smartness" with this new behaviour.
I own email certs from Thawte.
When it expires, I get a new one.
The new one has an identical subject.

With the code we have as of today, searching by nickname, the code will automatically find the most recent cert. Getting a new cert is sufficient.
It's not necessary to open mail security prefs.

I believe with your proposed patch it would be necessary :-/
Can you think of a way to enhance that?

...

I can not think of such a way yet.
I think we should be smarter.


My dream is that eventually we make it unnecessary having to manually select a cert at all, and only offer it as an option to explicitly specify one.

In the old days of NS Communicator 4.x it was sufficient to own a personal cert that contains an email address that matches your current mail account, and the cert would be selected automatically.


So maybe we should focus on the scenarios when such individual selection would be necessary.


You mentioned two different ones so far:
a) different usage, digitalSignature vs. nonRepudiation
b) different email addresses in cert

Regarding a), why not offer it as a high level decision and save the user from having to look at cert extensions manually?

We could offer a two-radio-button that says "for signing prefer * digitalSigatnure cert * nonRepudiation cert" and have our code deal with identifying the correct one automatically.

Regarding b), I'm surprised why the email addresses are not included in the subject. I think it should.


(Also your patch has code to disable the prompt want-to-use-same-cert-for-both-encryption-and-signing. I think we should keep that.)

Comment 42

[Kai Engert [:KaiE:]]

Reading comment 0 again, instead of storing "nickname + unique key" (where unique key is either a serial number or a dbkey), we also have the scenario where one has multiple certs from different CAs that happen to have the same subject.

I would prefer to avoid storing a key.

I would prefer to store the high level information, and I think it would be ok to store more than we do today.

What about storing all of:
- cert nickname
- issuer nickname
- a single string of all email addresses in the cert like "[e-mail],[e-mail]"
- the digitalSignature/nonRepudation preference

If a user gets new certs with new validity, but same attributes, there is no need to manually configure again.

But I'm still thinking out loud.
Using all of the above information to query the list of certs and matching the correct one might be tricky.


Maybe we should go with a solution like Kaspar's patch, and in addition introduce a little helper assistant?
If the certificate that got explicitly configured (by serial number) has expired or is invalid, we could dynamically bring up a dialog like "your cert configured for S/Mime is not valid, would you like to select a different one now?"

Comment 43

[Julien Pierre]

Kai, re: comment 41, if you have an expired cert, the code should be smart enough not to pick it as a default cert or force you to go through a dialog to select the current cert. I believe there is only really an issue when you have multiple unexpired certs with the same subjects, which is when the code can't automatically choose the right certs. The UI could do its filtering (by usage, validity, etc) and only open the selection dialog if there is still more than one to choose from. 

Comment 44

[Alaric]

We are talking about a handful of user certs, not storing the public key of every website a user has every visited, I have more certs than most people with the 7 I have...  and still that is only 7, and I only use 2 on a day to day basis.  

Even storing the entire public key for Seven 2048 bit certs means that we are talking about 14k, not a heck of a lot if you are concerned about space, and if that IS too much for some reason, why not store a hash of the public key? Furthermore, if you are concerned with storing sensitive data, there is nothing sensitive in it, that is why they are public keys.  

Comment 45

[Kaspar]

(In reply to comment #41)

Thanks for your valuable comments, Kai. So I understand there's still a chance of addressing this issue (other than resolving the bug as invalid ;-)

> Can you please paste an example of such a dbkey? Just curious how it looks in
> the prefs.

Yes, the dbkey pref for the cert in comment #31 looks like this:

user_pref("mail.identity.id1.signing_cert_dbkey", "AAAAAAAAAAAAAAAHAAAANw/3YoLIoP8wNTEWMBQGA1UEChMNRXhhbXBsZSwgSW5j\r\nLjEbMBkGA1UEAxMSRXhhbXBsZSBJc3N1aW5nIENB");

As is obvious, the dbKey is a Base64 encoded blob, when looking at it with JS - but in PSM, its handled as the concatenation of serial number and issuer DN (cf. http://lxr.mozilla.org/seamonkey/ident?i=GetDbKeycomposed - apparently there were plans to add module and slot IDs, too).

> I'm concerned about a regression in "smartness" with this new behaviour.
> I own email certs from Thawte.
> When it expires, I get a new one.
> The new one has an identical subject.
> 
> With the code we have as of today, searching by nickname, the code will
> automatically find the most recent cert. Getting a new cert is sufficient.
> It's not necessary to open mail security prefs.
> 
> I believe with your proposed patch it would be necessary :-/
> Can you think of a way to enhance that?

Yes... it just means we have to stick to the current format for the *{signing,encryption}_cert_name prefs, i.e. instead of

  user_pref("mail.identity.id1.signing_cert_name", "Example User [0F:F7:62:82:C8:A0:FF]");

we would continue to use

  user_pref("mail.identity.id1.signing_cert_name", "Example User");

I don't consider this a big issue, I only thought it would be useful to display what certficate exactly the user has selected. But if we leave out the "[serial]" suffix in the *_cert_name pref, this will actually make my life for patching am-smime.js easier (the most complex thing in my first version of the patch was dealing with the new *_cert_name format...).

If we keep the current *_cert_name format for the prefs, then I would propose to use code like this in nsMsgComposeSecure.cpp:

if (!mEncryptionCertDBKey.IsEmpty()) {
  /* if stored in the prefs, retrieve certs by dbkey */
  certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mEncryptionCertDBKey).get(),
                          NULL, getter_AddRefs(mSelfEncryptionCert));
  if (mSelfEncryptionCert) {
    PRUint32 verification_result;
    if (NS_FAILED(
        mSelfEncryptionCert->VerifyForUsage(nsIX509Cert::CERT_USAGE_EmailRecipient,
                                            &verification_result))
        || verification_result != nsIX509Cert::VERIFIED_OK)
      mSelfEncryptionCert = 0;
  }
}
/* if no suitable cert found by dbkey, search again by nickname */
if ((mSelfEncryptionCert == nsnull) || mEncryptionCertDBKey.IsEmpty())
  certdb->FindEmailEncryptionCert(mEncryptionCertName,
                                  getter_AddRefs(mSelfEncryptionCert));
/* same procedure for the signing cert */
if (!mSigningCertDBKey.IsEmpty()) {
  certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mSigningCertDBKey).get(),
                          NULL, getter_AddRefs(mSelfSigningCert));
  if (mSelfSigningCert) {
    PRUint32 verification_result;
    if (NS_FAILED(
        mSelfSigningCert->VerifyForUsage(nsIX509Cert::CERT_USAGE_EmailSigner,
                                         &verification_result))
        || verification_result != nsIX509Cert::VERIFIED_OK)
      mSelfSigningCert = 0;
  }
}
if ((mSelfSigningCert == nsnull) || mSigningCertDBKey.IsEmpty())
  certdb->FindEmailSigningCert(mSigningCertName,
                               getter_AddRefs(mSelfSigningCert));

(Note that I've added suitable calls to VerifyForUsage(), this was something which was missing in my preliminary version of the patch.)

With this code, if an expired cert is retrieved by dbKey, we will fall back to the nickname search - and find the newest cert with the same subject DN (which would do the trick for your use case with the Thawte cert, I assume).

> My dream is that eventually we make it unnecessary having to manually select a
> cert at all, and only offer it as an option to explicitly specify one.

Sounds like a good dream, but is this really within the scope of this bug...?

> So maybe we should focus on the scenarios when such individual selection would
> be necessary.
> 
> 
> You mentioned two different ones so far:
> a) different usage, digitalSignature vs. nonRepudiation
> b) different email addresses in cert
> 
> Regarding a), why not offer it as a high level decision and save the user from
> having to look at cert extensions manually?
> 
> We could offer a two-radio-button that says "for signing prefer *
> digitalSigatnure cert * nonRepudiation cert" and have our code deal with
> identifying the correct one automatically.

Sure, you can add all these sorts of tuning knobs (check boxes, drop-down lists etc.), but this might soon become a neverending story... there are simply too many options a CA has to populate its certificate extensions. (And occasionally, CAs also screw up with some extensions, so you would have to consider that, too.)

Apart from keyUsage and EKU, there are extensions like certificatePolicies (OIDs with a possibly very CA-specific meaning, there are almost no "industry-wide" OIDs defined) or subjectDirectoryAttributes (which is just a bag for yet another pile of data with random content "[SEQUENCE SIZE (1..MAX) OF Attribute]") - and all these would have to be considered in some way when "automagically" selecting the right certificate. (I'm not against this idea in general, I just believe a huge amount of work would be required for this, and I'm not sure if it's worth the effort.)

> Regarding b), I'm surprised why the email addresses are not included in the
> subject. I think it should.

No, putting e-mail addresses into the subject DN has been deprecated in the S/MIME RFCs for a long time already (see e.g. section 3 in RFC 2632, from June 1999). The emailAddress attribute defined in PKCS#9 wasn't really meant to be used as a subject RDN in an X.509 certificate (although it became - and still is - very popular, I know).

> (Also your patch has code to disable the prompt
> want-to-use-same-cert-for-both-encryption-and-signing. I think we should keep
> that.)

This wouldn't be needed if we stick to the old prefs format for *_cert_name. (But in any case, it wasn't designed to disable the prompt completely, it would just apply to one specific case [at least that's what my intention was].)

(In reply to comment #42)
> What about storing all of:
> - cert nickname
> - issuer nickname
> - a single string of all email addresses in the cert like "[e-mail],[e-mail]"
> - the digitalSignature/nonRepudation preference

I'm not opposed to this idea, though this would probably mean that changes to PSM (and/or NSS) are necessary as well. With the current proposal, we can confine ourselves to modifying {mailnews,mail}/extensions/smime/.

> Maybe we should go with a solution like Kaspar's patch, and in addition
> introduce a little helper assistant?
> If the certificate that got explicitly configured (by serial number) has
> expired or is invalid, we could dynamically bring up a dialog like "your cert
> configured for S/Mime is not valid, would you like to select a different one
> now?"

With my second proposal (leave _cert_name prefs as is, but use _cert_dbkey if available and pointing to a valid cert), this wouldn't be necessary... the only drawback with this solution is that the user won't really know that we ignore the _cert_dbkey preference he explicitly set in the past (when this cert has expired).

Comment 47

[julien.stern]

(In reply to comment #24)
> The described behavior is by design.  
> 
> The design of X.500 (and hence X.509) prevents two CAs from ever issuing
> certs with the same subject name.  Of course, there are many people who 
> play at being CAs who don't follow the rules, and they might violate that.
> But mozilla's crypto security is designed to work with real by-the-book 
> X.509 certs and CAs, not play certs and CAs.

I believe this is not correct. The thing a CA should not do is to issue two certificates with the same DN to two different entities. Issuing two certificates with the same DN to the same entity is not only a common practice but is mandatory in many cases (in the case of renewal for instance).

What the "select certificate" dialog should do is to present all certificates matching the email address configured in the account. This email address should be searched in the SubjectAltName extension (they can be several emails) and in the EMAILADDRESS part of the DN if you want to support old non compliant PKIs.
 

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Julien Stern, You're saying that ANY ONE CA may do this routinely for renewal.
I agree.  The statement to which you replied clearly said "TWO CAs"...

Comment 49

[julien.stern]

(In reply to comment #48)
> Julien Stern, You're saying that ANY ONE CA may do this routinely for renewal.
> I agree.  The statement to which you replied clearly said "TWO CAs"...

Nelson, I still believe that TWO CAs issuying certificates to the same entity may (and actually should) use the same DN. Indeed, the DN in X.500 is supposed to be a unique identifier for an entity.

At any rate, I was not trying to nitpick but simply to underline that even if you have two email certificates issued by the SAME CA with the same DN but two different email addresses, you have no way of selecting the correct email address in the current versions...

Actually, it seems somewhat worse: even if only the CNs match, it seems that the user is not given the choice.

Comment 50

[Kaspar]

Attachment: Patch v2 - support cert selection by nickname and serial number

Here is a new version of the patch, which tries to address most of the issues raised so far:

1) it still relies on storing the dbKey in addition to the nickname, but it is considerably smarter when retrieving the preference attribute (for more information, see the detailed comment in nsMsgComposeSecure.cpp in the patch). This will e.g. solve the "renewal issue" pointed out by Kai in comment 41.

2) By slightly extending nsCertPicker::PickByUsage(), it will filter the list of certificates to only display those with an e-mail address matching the one configured for this identity (as suggested in comment 47). If no matching certificate is found, an alert indicating the e-mail address in question is shown (I will attach another screenshot shortly).

Somewhat related to this patch is bug 392790, which I filed separately since that one truly is a PSM bug, while I would propose to change the component of this one here to "MailNews: Security" - it only applies to Thunderbird and Seamonkey (nsCertPicker::PickByUsage() is only used by these two, which is why I kept the changes to nsIUserCertPicker.idl and nsCertPicker.cpp with this patch). Additionally, bug 392208 should be fixed (in any case), since bogus dbkey prefs will crash PSM, otherwise.

In my opinion, having this patch would be a noticeable improvement for both the "normal" user (who might happen to have more than one certificate, but each with different e-mail addresses) and the expert user who wants to have very fine-grained control over the cert selection.

Kai, I would be glad if you could review the patch - or if you are too busy, let me know if there's anybody else I could ask for review (maybe David or Scott? would it make sense to Cc them anway, btw?). Thanks.

Comment 51

[Kaspar]

Attachment: Warning message shown when no certificate for a particular email address is available

Comment 52

[Kaspar]

(In reply to comment #50)
> Additionally, bug 392208 should be fixed (in any case), since bogus
> dbkey prefs will crash PSM, otherwise.

That should have been bug 392780, actually. Can somebody add this as a dependency, anyway? (I don't seem to have enough privileges to do so.)

Comment 53

[Kaspar]

(changing component as suggested in comment 50, adding dependency to bug 392780)

Comment 54

[Kai Engert [:KaiE:]]

(In reply to comment #51)
> Warning message shown when no certificate for a particular email address is
> available

> 2) By slightly extending nsCertPicker::PickByUsage(), it will filter the list
> of certificates to only display those with an e-mail address matching the one
> configured for this identity (as suggested in comment 47). If no matching
> certificate is found, an alert indicating the e-mail address in question is
> shown (I will attach another screenshot shortly).

Unfortunately there are people who want to use certs that contain no email address at all. I think we must continue to offer a way to select any cert.

If you want to be really smart, we could enhance cert picker to show more UI:
- show "select a cert to be used with email address ...@...com"
  (as passed in as a parameter to the dialog)
- have a checkbox in the UI which says "show only certs that are valid for
  this email address", checked by default, list is filtered
- if the user unchecks the checkbox, populate the list with all certs


(In reply to comment #50)
> 1) it still relies on storing the dbKey in addition to the nickname, but it is
> considerably smarter when retrieving the preference attribute (for more
> information, see the detailed comment in nsMsgComposeSecure.cpp in the patch).
> This will e.g. solve the "renewal issue" pointed out by Kai in comment 41.

So, when the cert for a dbkey is no longer valid, you'll attempt a lookup by nickname.

While this is good for those scenarios where the user is ok with a selection based on nickname only, it's not perfect for those users who had carefully selected one cert from multiple ones with the same nickname.

Automatically selecting a newer, but incorrect one, might be unexpected to those users.


I wonder if the update-selected-cert-after-expiration should be user assisted. The lookup for self's signing+encryption certificate happens at the time the user clicks "send" or "show info for this message".

This means, the cert lookup happens during a user initiated activity.

If there are indeed better certs available (we can look before prompting), I wonder if we should take the opportunity to:
- inform the user "your action can not complete because the cert selected in the past is no no longer valid"
- give the user a choice of:
  - have Thunderbird automatically choose one of your newer certs
    (and update the pref, this should be ok, because the user agreed to it)
  - offer to cancel the action and require the user to manually update
    the selected cert
    (either by simply cancel and have the user open prefs himself,
     or, if we can, directly open the security
     prefs pane for the current account. We should not show a cert picker,
     as we should make sure both signing+encryption certs are in synch.)

Does that make sense?


>  If we have retrieved a stale preference (such as a dbKey which no longer
>  exists, or an outdated nickname + serial combo), then clean up the preference
>  attribute as needed.

I think, when reading prefs only while looking for a matching cert, we should not clean up prefs. Imagine the prefs were written by a future version of your code. I someone with Thunderbird v 4.0 goes back to Thunderbird v 3.0, then v3 should not change the stored prefs, unless the user explicitly makes changes to the prefs.

Does that make sense?

Comment 55

[Angelo Rosenfelder]

(In reply to comment #29)
> The classic example where Thunderbird's/Seamonkey's current selection mechanism
> isn't good enough is the case where a user has separate certs for
> digitalSignature and nonRepudiation, both having the same subject (which is
> perfectly acceptable, I believe). With the current code, it's simply not
> possible to explicitly select either of these two certificates for signing...
> you will always end up with an "automagically" selected cert
> (FindEmailSigningCert() calls CERT_FindUserCertsByUsage(), which calls
> CERT_CreateSubjectCertList(), which creates the list by calling
> CERT_AddCertToListSorted(), which in turn depends on the outcome of 
> CERT_SortCBValidity(), cf.

Hi. Is there any work done on this? Because I got a Token (sold by the swiss post for officially signing documents) which contains 2 certificates. They have the same subject, but different labels (nicknames) and different usages (one for signing, one for non-repudiation). But no matter which certificate I select in the security options, the mail alwyays gets signed with the non-repudiation certificate because of the mechanism described above.
I would like to help to fix this bug, whom should I contact to discuss such issues?

Comment 56

[Wayne Mery (:wsmwk)]

Angelo, I don't know why no one in the bug has answered your offer of assistance.  (hello??)  I am surprised - usually such offers are greeted with enthusiasm.  I think essentially the last patch is r- based on comment 54, but perhaps kaie is asking to be persuaded otherwise?

Nelson, kai, I don't know the technical aspects of this bug, but if there is a core issue and peripheral issues, would it make sense to resolve the core issue of this bug and resolve the rest in a different bug?

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Wayne, I agree that comment 54 (which was made when the patch was about 
one month old) seems to be expressing r- for the patch, but that the 
patch (now a year old) still appears to be awaiting review. 

Maybe Kaspar is waiting for an r+ or r-, and maybe if Kai gave it r+ or r-, 
that would drive this forward.  Or Maybe Kaspar has no further wish to work 
on this.  I don't know.  I would like to encourage Kaspar's continued 
participation in PSM contributions.

As for Angelo's offer and question in comment 55, Discussion of these issues
should probably best take place in the dev-tech-crypto newsgroup rather than
in private email, but the email addresses of the principal individuals are 
all in this bug (on this bug page).  

Angelo: you don't need permission is needed to write a patch and attach it 
to this bug and request review.  I would say there is an unwritten standing
invitation to anyone who is capable to help with any bug report by writing
patches for it.

Comment 59

[Kai Engert [:KaiE:]]

Wayne:

It wasn't necessary for Angelo to talk to anyone, if he intend to help with this bug. This is the same as saying "may I ask an question". Of course you do. Just go ahead an ask.

The same applies here. If you want to help, go ahead an help. All information in this bug is already present. Kaspar had attached a patch, and I had given my review / my concerns.

Apparently nobody has have had anough time to produce a better patch, since we're all very busy.

You're free to contribute a patch that takes all concerns into consideration and attach it to this bug.

Comment 60

[Kai Engert [:KaiE:]]

Comment on attachment 277289 [details] [diff] [review]
Patch v2 - support cert selection by nickname and serial number

r-, see comment 54

Comment 61

[Andi]

Not sure but i guess this also lead to the problem that Thunderbird is choosing the wrong public key for encryption if more than one is available with the same CN, no? Stumbled across this today with s/mime certificates from trustcenter.de.
What is coming next?

Comment 62

[Hilarion]

If it helps in diagnostics/testing, TC TrustCenter offers free certificates that can be used to sign e-mails. The certificate subject contains only users name and country and e-mail address is in alternative certificate name:
http://www.trustcenter.de/en/products/tc_internet_id.htm
I generated such certificates (with same data except of e-mail address) for my e-mail accounts and because of the bug I can't use those.
As a contrast, Comodo offers free certificates here:
http://www.comodo.com/home/email-security/free-email-certificate.php
Those use e-mail address only (as subject) and can be used without any problems (but lacking real user name have lesser value for signing purposes).

Comment 64

[Ludovic Hirlimann [:Usul]]

Could this have been fixed by 596221 ?

Comment 65

[Kaspar]

(In reply to comment #64)
> Could this have been fixed by 596221 ?

No, bug 596221 is about locating a certificate for encrypting to some recipient (see also bug 596221 comment 18).

This bug is mainly about fixing nsMsgComposeSecure::MimeCryptoHackCerts (plus the UI around it). The patch I proposed four years ago didn't/doesn't need any changes to any of the findCertXXX methods in nsNSSCertificateDB.cpp, it uses findCertByDBKey/findEmailEncryptionCert/findEmailSigningCert as is.

Comment 68

[Karl-Heinz Fischbach]

still a problem in thunderbird 7 on Ubuntu

Comment 69

[Karl-Heinz Fischbach]

still a problem in thunderbird 8 on ubuntu.
A pitty, nobody takes the bug to solve it.

Comment 70

[chokito]

Still a problem in SeaMonkey 2.6 on Windows...

Comment 71

[Martin Benedict]

I also have the same problem with TB 8 and Win 7. 
I have two certificate that I get from my universitys CA. Both have the same subject. but different e-mail adresses. 
I can choose only one of them.

Comment 73

[Casey Jones]

I've been experiencing this bug for a few years with TrustCenter certificates.  VERY frustrating...  I was almost frustrated enough to dive into the code myself, but after seeing what an ordeal it's been for other would-be contributors, I've thought twice.

I tried to follow the comment thread, but sort of lost patience and I'm not sure if I really have a clear grasp of what's going on or not.  Michael and Kaspar have both submitted patches and revised patches, fixing the problem.  From what I can understand, things basically came to a standstill after comment 54, where Kai commented on Kaspar's second and hitherto final patch.  I'm curious why Kaspar didn't respond.  I suppose he must be frustrated or has no comments to make, but he can speak for himself.  As for my take on Kai's comments -- without getting technical -- I think they're quite valid points, but they seem to me more like minor improvements to a working patch that actually fixes the bug.  Can't we apply already and worry about those finer points later?

Comment 74

[Casey Jones]

Happy seventh anniversary to bug 278689!

Comment 76

[torkelsson]

Still a problem in TB 12. Any chance that this will get fixed?

Comment 78

[gmeier44]

Still a problem in TB 15.0.1 (under Win7)

Any perspective on it getting fixed? It renders tb useless for certain applications.

Comment 79

[christian.feldmann]

The problem still exists in Thunderbird 17.0.

One workaround for this problem is (if you have good contact to your CA) to get two certificates for different E-Mail Adresses with slightly different Subjects.
For example I now have two certificates, one with an OU in the Subject and one without an OU.
If the certificates differ in this way you can select both in the "Select Certificate" dialog.

Comment 80

[nevix]

Happy 8. anniversary to bug 278689!
I can confirm the problem in Thunderbird 17.0.2.

Comment 81

[mozilla]

Can confirm the problem in Thunderbird 24.5.0.

Comment 82

[Adrian]

Can also confirm for 24.1.0 and 31.2.0 (31.2.0-1.fc20).

Is there a workarround? The name + serial number scheme used in "View Certicates" can be used in prefs.js?

Comment 84

[murof]

So we have just celebrated the tenth anniversary now???
I think, TB's dead, isn't it? :-((

Comment 85

[fettucini]

It would be highly appreciated, if super-reviewer Kai Engert, who ceased responding to the original patch suppliers would continue his work on a solution on the problem - after 10 years have passed, the issue is ripe and ready to fix.

Either that, or someone else with responsibility give the o.k. for the patch, since no real blocker arguments have been raised during that time. It's better to have many small improvements but to dream on about the perfect solution.

Not to wonder why, the usability of S/MIME and encryption in general has not improved significantly all this time, if help is being regularly turned down this way!

Comment 86

[Wayne Mery (:wsmwk)]

It's unclear who would take this up, but the patch is not in Kai's responsibilty

Comment 87

[fettucini]

From what I see, from #45 on (9 years ago), the reviever-discussion got stuck in non-vital details, thus attracting even more commentors and experts. 
Eventually, the reach for a solution drifted off into nirvana, when Kaspar Brand, the second patch contributor finally ran out of patience after more than a year of fruitless jabber. As it happened with the first real contributor. 
As said, the design if buggy, so a patch is needed and the perfect solution will not appear with a big bang, so it seems. And Kaspar Brand seemed to have a good-enough working fix for the problem.
Unless, of course, some people at Mozilla don't want any improvements in security and encryption in general. Where's the mozilla hq located again, in USA?

Comment 88

[fettucini]

Sorry, for being a bit harsh maybe. It's just not the first time I find bugs (or file them) on an open source project, only to see no progress in months or years on that - despite many others confirming it. Now, seeing a possible solution at hand here, being neither rejected, nor accepted, just discussed to death within a 10 years perion, just got me carried away a little. I'd suggest to let contributor Kaspar Brand finalize and commit the patch and finally move on.

Comment 89

[Kaspar]

Attachment: Patch v3 - support cert selection by nickname and serial number, c-c part

Comment 90

[Kaspar]

Attachment: Patch v3 - support cert selection by nickname and serial number, m-c part

Comment 91

[Kaspar]

Comment 85 and comment 87 capture the situation fairly well. Since kaie is no longer authoritative for this area, and the most recent reviewers of code under mailnews/extensions/smime (jcranmer, mkmelin) have been more supportive in accepting external contributions, I resurrected the patch from 2007 and have adapted for the current trunk versions of Thunderbird and Seamonkey.

A tryserver build would be helpful for users affected by this issue, I assume.

The changes to CertPicker would need approval from keeler (Cc'ed with needinfo), I guess, but note that bug 1142350 has recently been filed - so it might be the right opportunity to move it to c-c on this occasion.

Comment 92

[Dana Keeler (she/her) [:keeler]]

(In reply to Kaspar Brand from comment #91)
> The changes to CertPicker would need approval from keeler (Cc'ed with
> needinfo), I guess, but note that bug 1142350 has recently been filed - so
> it might be the right opportunity to move it to c-c on this occasion.

I think that's a reasonable approach.

Comment 93

[Julien Pierre]

Kasapar,

Nickname + serial number is equivalent to Subject + serial number.
Unfortunately, that does not uniquely identify a certificate/key.

You can still have 2 cert with identical subjects but 2 different serial numbers, if they are issued by 2 different CAs.

I would recommend patching this bug in a way that allows uniquely identify the cert and key, otherwise, you are not fully solving the problem.

I can give you some feedback in terms of how we improved cert selection in another product (which doesn't use NSS).

What we chose was a triplet of (subject, issuer, serial number). With NSS, this would map to cert nickname, issuer nickname, and cert serial number.

The following combinations are guaranteed to always uniquely identify a cert, at least in a properly implemented PKI where serial numbers don't get reused by the CA :
issuer + serial number
cert subject + issuer + serial number

For the general case, however, our other product also allows combinations that don't uniquely identify the cert, specifically :
cert subject

However, if the runtime finds that there are multiple certs that match that subject, it will error and complain that this needs to be disambiguated. In this case that translates to a server error.

For Firefox, this could be a pop-up inviting the user to select the specific cert - and then that could get stored in the config with some of the unique parameter combination I listed above.

Comment 94

[Kaspar]

Julien, there's no need to further educate me on how a certificate is uniquely identified in X.509 (or NSS, for that matter). I'm quite familiar with that kind of background.

If this is just your reaction to seeing "support cert selection by nickname and serial number" in the latest patch description, the I suggest you either have a look at the implementation itself (attachment 8600681 [details] [diff] [review]), or read comment 29 and comment 45, which should make it obvious that the proposed patch will identify the cert by serial number + issuer DN, in the end (see http://mxr.mozilla.org/mozilla-central/search?string=nsNSSCertificate%3A%3AGetDbKey for how the dbKey gets assembled).

If, on the other hand, you want to repeat the 2007 discussions (where NSS and PSM veterans were each coming up with their own wish list, Weltanschauung and dreams, including contrived cases where "it's not perfect", effectively suppressing any progress), then let me know - I will then immediately stop wasting more time on this. I guess that a number of Thunderbird users (it has nothing to do with Firefox, JFTR) would appreciate a less ideological approach towards a solution of this problem, however.

Comment 95

[Julien Pierre]

Kaspar, it was the former - I only read the patch description. If your patch actually will identify the cert by serial number + issuer DN in the config, rather than subject  + serial number as stated in the description, then all is well and you can ignore my comment.

Comment 96

[fettucini]

Well, so this means, that the bug can finally be resolved by Kaspar now, no more objections? Then... 3... 2... 1... go :)

Comment 97

[Kent James (:rkent)]

Kaspar, do you plan to ask for review? Please ask for review from :keeler on the m-c part. After that is approved, we'll figure out what to do with the c-c part.

Comment 98

[Kaspar]

Attachment: Patch v4 - support cert selection by nickname and serial number, m-c part

(In reply to Kent James (:rkent) from comment #97)
> Kaspar, do you plan to ask for review? Please ask for review from :keeler on
> the m-c part.

Ok, here we go... I have polished that part of the patch a bit meanwhile.

This changes the signature of the (only) method of nsCertPicker, so I assume it needs a uuid change. I'm not too familiar with the policy for interface changes these days (frozen and all that stuff), but am-smime.js is the only caller in Mozilla's trees, I think (and would be adapted in the c-c part accordingly). Perhaps someone with access to the XPI code search could check if there's any extension which makes use of pickByUsage - though I doubt that it's the case.

Note that extending nsCertPicker by the emailAddress argument is only tangentially related to the primary topic of this bug - I have added this capability because it was brought up in comment 36 ff. If it makes things too complicated, we can also drop the m-c changes and stick with the existing behavior in am-smime.js (i.e., it lets you select non-matching certificates, as has been the case for the past 8 years).

Comment 99

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8608022 [details] [diff] [review]
Patch v4 - support cert selection by nickname and serial number, m-c part

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

Interfaces aren't really frozen anymore. In any case, it doesn't look like any addons are using this one. LGTM with comments addressed.

::: security/manager/ssl/public/nsIUserCertPicker.idl
@@ +14,5 @@
>                            in wstring selectedNickname,
>                            in long certUsage, // as defined by NSS enum SECCertUsage
>                            in boolean allowInvalid,
>                            in boolean allowDuplicateNicknames,
> +                          in AString emailAddress,

Since this is optional, let's document that. Also, from what I can tell from the implementation, this is only taken into account if the candidate certificates have email addresses in them (i.e. a certificate without an email address will match regardless of if this argument is provided). This should be documented.

::: security/manager/ssl/src/nsCertPicker.cpp
@@ +93,5 @@
>  
>      if (tempCert) {
>  
> +      PRUint32 num = 0;
> +      PRUnichar **addrs;

nit: use uint32_t and char16_t
Also, scope these inside the 'if (!emailAddress.IsEmpty())' block since they're not used outside of it.

@@ +95,5 @@
>  
> +      PRUint32 num = 0;
> +      PRUnichar **addrs;
> +
> +      // if a (non-empty) argument is supplied to PickByUsage,

nit: "emailAddress argument"

@@ +98,5 @@
> +
> +      // if a (non-empty) argument is supplied to PickByUsage,
> +      // skip the certificate if it doesn't match for this address
> +      if (!emailAddress.IsEmpty()) {
> +        tempCert->GetEmailAddresses(&num, &addrs);

Check for and handle failure here:

nsresult rv = tempCert->GetEmailAddresses...
if (NS_FAILED(rv)) {
  return rv;
}

Although, my understanding is that this is just to see if the certificate has any email addresses. Why not call tempCert->GetEmailAddress instead? That way, there doesn't need to be any manual memory management (although the rv should still be checked/handled).

@@ +101,5 @@
> +      if (!emailAddress.IsEmpty()) {
> +        tempCert->GetEmailAddresses(&num, &addrs);
> +        if (num > 0) {
> +          bool match = false;
> +          tempCert->ContainsEmailAddress(emailAddress, &match);

Check rv here too.

Comment 100

[Kaspar]

Attachment: Patch v5 - support cert selection by nickname and serial number, m-c part

Many thanks for the fast review. v5 hopefully addresses your comments/concerns.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #99)
> Although, my understanding is that this is just to see if the certificate
> has any email addresses. Why not call tempCert->GetEmailAddress instead?
> That way, there doesn't need to be any manual memory management (although
> the rv should still be checked/handled).

GetEmailAddress will always return a string ("(no email address)" in case of an "e-mail-less" certificate), so it can't really be used for this purpose. I believe checking the number returned by GetEmailAddresses is the only reliable method for determining if the cert includes at least one address.

Comment 101

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8608518 [details] [diff] [review]
Patch v5 - support cert selection by nickname and serial number, m-c part

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

I think I found an implementation detail that will prevent this working as intended (see comment), but it's simple to fix, so r=me with that addressed.

::: security/manager/ssl/public/nsIUserCertPicker.idl
@@ +17,5 @@
>                            in boolean allowDuplicateNicknames,
> +                          in AString emailAddress, // optional - if non-null,
> +                                                   // skip certificates which
> +                                                   // have at least one e-mail
> +                                                   // address, but do not

nit: I think the comment is more clear without this comma

::: security/manager/ssl/src/nsCertPicker.cpp
@@ +97,5 @@
> +      // skip the certificate if it doesn't match for this address
> +      if (!emailAddress.IsEmpty()) {
> +        uint32_t num = 0;
> +        char16_t **addrs;
> +        rv = tempCert->GetEmailAddresses(&num, &addrs);

Looking at the implementation of GetEmailAddresses, I think it can return NS_ERROR_OUT_OF_MEMORY if there aren't any email addresses in the certificate, which is not what we want. Maybe a simpler approach would be to use node->cert directly and see if CERT_GetFirstEmailAddress(node->cert) is non-null to determine if the certificate has any email addresses.

@@ +108,5 @@
> +          rv = tempCert->ContainsEmailAddress(emailAddress, &match);
> +          if (NS_FAILED(rv)) {
> +            return rv;
> +          }
> +          if (match != true) {

nit: 'if (!match) {'

Comment 102

[Kaspar]

Attachment: Patch v6 - support cert selection by nickname and serial number, m-c part

Thanks a lot for your careful checking. CERT_GetFirstEmailAddress indeed looks like the way to go, and allows making the check much more efficient. Your other comments are also addressed with v6.

I also realized that I inadvertently dropped the proper nickname assignment which was part of the v3 patch - without this adjustment the wrong nickname may be used (as we're not incrementing CertsToUse when skipping a cert). v6 now properly handles this case, too.

Comment 103

[Kaspar]

Attachment: Sample user certificate (including private key), S/N 0F:F7:62:82:C8:A1:00, digitalSignature only, valid through May 2017 (no passphrase set)

Attaching refreshed test certificates (1 of 3), and mark the older ones as obsolete.

Comment 104

[Kaspar]

Attachment: Sample user certificate (including private key), S/N 0F:F9:E8:9E:B7:AA:00, nonRepudiation only, valid through May 2017, (no passphrase set)

refreshed test certificate, 2 of 3

Comment 105

[Kaspar]

Attachment: Sample user certificate (including private key), S/N 0F:F9:E8:7A:66:A2:98:00, digitalSignature only, without any e-mail address, valid through May 2017 (no passphrase set)

refreshed test certificate, 3 of 3 (does not include any e-mail address, so can be used for special-case testing)

Comment 106

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8609996 [details] [diff] [review]
Patch v6 - support cert selection by nickname and serial number, m-c part

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

I noticed another couple of issues, but they're easy to fix. r=me with those addressed.

::: security/manager/ssl/src/nsCertPicker.cpp
@@ +109,4 @@
>        // XXX we really should be using an nsCOMPtr instead of manually add-refing,
>        // but nsNSSCertificate does not have a default constructor.
>  
>        NS_ADDREF(tempCert);

Come to think of it, I'm fairly sure we'll leak each tempCert we skip, so we should use a RefPtr as in https://hg.mozilla.org/mozilla-central/annotate/e537a1ba501b/security/manager/ssl/src/SSLServerCertVerification.cpp#l661

RefPtr<nsNSSCertificate> tempCert(nsNSSCertificate::Create(node->cert));

(and remove that comment)

@@ +115,4 @@
>        nsAutoString nickWithSerial;
>        nsAutoString details;
>  
> +      rv = tempCert->GetNickname(i_nickname);

If I'm understanding the code correctly, this reverts the functionality that appends the "(expired)"/"(not yet valid)" strings onto the nicknames for expired/not yet valid certificates. One solution might be to split CertsToUse into two variables: one for indexing nicknames and one for actually counting how many certs are going to be in the dialog.

Comment 107

[Kaspar]

Attachment: Patch v7 - support cert selection by nickname and serial number, m-c part

(In reply to David Keeler [:keeler] (use needinfo?) from comment #106)
> Come to think of it, I'm fairly sure we'll leak each tempCert we skip

Indeed, thanks for pointing this out.

> , so we should use a RefPtr

Done - and IIUC, this makes NS_ADDREF/NS_RELEASE unnecessary, so I dropped them.

> If I'm understanding the code correctly, this reverts the functionality that
> appends the "(expired)"/"(not yet valid)" strings onto the nicknames for
> expired/not yet valid certificates.

Good catch. It's not an issue with the current usage of PickByUsage in am-smime.js (allowInvalid is hardcoded to false, i.e. the list won't include expired or not-yet-valid certs), but I agree we should properly deal with this.

> One solution might be to split CertsToUse into two variables: one for
> indexing nicknames and one for actually counting how many certs are going
> to be in the dialog.

Done, I have added a second counter to the for loop (and switched back to retrieving the nick from the nicknames array).

Comment 108

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8611126 [details] [diff] [review]
Patch v7 - support cert selection by nickname and serial number, m-c part

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

Great - just a couple of comments. r=me with those addressed.

::: security/manager/ssl/src/nsCertPicker.cpp
@@ +80,5 @@
>      free(certDetailsList);
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  int32_t i, CertsToUse;

nit: it would be nice to use more descriptive names: e.g. nicknameIndex and numCertsInDialog or something

@@ +87,2 @@
>         !CERT_LIST_END(node, certList.get()) &&
>           CertsToUse < nicknames->numnicknames;

This should be i (or whatever its new name is) instead of CertsToUse

Comment 109

[Kaspar]

Attachment: Patch v8 - support cert selection by nickname and serial number, m-c part

"i" changed to "nicknameIndex", as suggested, and the for loop termination expression adapted. I didn't rename CertsToUse, since it's also used in more places further down, and from a semantic point of view, the patch doesn't change the purpose/behavior of this variable (it would only add unnecessary noise to the diff, IMO).

As bug 1164714 has just landed, I also took the new file locations into account with v8.

Can we consider the m-c part done? I would then concentrate on the c-c part next.

Comment 110

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8612441 [details] [diff] [review]
Patch v8 - support cert selection by nickname and serial number, m-c part

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

Great - r=me.

::: security/manager/ssl/nsCertPicker.cpp
@@ +80,5 @@
>      free(certDetailsList);
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  int32_t nicknameIndex, CertsToUse;

I suppose technically these should each be on their own line according to the style guidelines.

@@ +94,5 @@
>      if (tempCert) {
>  
> +      // if a (non-empty) emailAddress argument is supplied to PickByUsage,
> +      // skip the certificate if it doesn't match for this address
> +      if (!emailAddress.IsEmpty() && CERT_GetFirstEmailAddress(node->cert)) {

Maybe add a comment that the cert must also have at least one email address, hence the call to CERT_GetFirstEmailAddress.

Comment 111

[Kaspar]

Attachment: Patch v8 - support cert selection by nickname and serial number, c-c part

(In reply to Kent James (:rkent) from comment #97)
> After that is approved, we'll figure out what to do with the c-c part

Kent, we're now at that point, I think. The c-c part touches stuff under mailnews/extensions/smime/, which is unowned as per the list under https://wiki.mozilla.org/Modules/MailNews_Core. The last bigger code change took place in bug 1018259, with reviews by jcranmer mostly (who is the overall owner of mailnews/, as you're surely aware of).

For the sake of consistency with the m-c part, I have renamed the c-c part to "v8" as well, but with the exception of a tiny change to moz.build, it's the same as v3.

Comment 112

[Kaspar]

(In reply to Kaspar Brand from comment #111)
> (In reply to Kent James (:rkent) from comment #97)
> > After that is approved, we'll figure out what to do with the c-c part
> 
> Kent, we're now at that point, I think.

Kent - ping?

Comment 113

[Kent James (:rkent)]

Sorry Kaspar, we're still quite busy with the Thunderbird 38 release.

If you are just looking for a reviewer, I'd try [e-mail], jcranmer, then myself (rkent) in that order. If they don't get to the patch in a week or two, try the next reviewer.

Comment 114

[Magnus Melin [:mkmelin]]

Comment on attachment 8613210 [details] [diff] [review]
Patch v8 - support cert selection by nickname and serial number, c-c part

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

Some drive-by nits. Also get rid of all the trailing spaces. (Seen in red if you check the review link.)

::: mailnews/extensions/smime/content/am-smime.js
@@ +332,5 @@
>  
> +        if (!gSignCertName.value.length)
> +          // after the encryption cert has been selected,
> +          // only prompt the user to select a signing cert
> +          // if no one is set (to prevent looping)

Capitalize, period at the end. Also, no need to make it this narrow (80ch is the guideline)

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +939,5 @@
> +      }
> +    }
> +  }
> +  
> +  /* same procedure for the signing cert */

prefer // style comments (otherwise it's hard to temporarily comment out large parts later)

@@ +943,5 @@
> +  /* same procedure for the signing cert */
> +  if (!mSigningCertDBKey.IsEmpty()) {
> +    certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mSigningCertDBKey).get(),
> +                            NULL, getter_AddRefs(mSelfSigningCert));
> +    if ((mSelfSigningCert == nullptr)

mozilla style is to use if (!mSelfSigningCert) for stuff like this

@@ +944,5 @@
> +  if (!mSigningCertDBKey.IsEmpty()) {
> +    certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mSigningCertDBKey).get(),
> +                            NULL, getter_AddRefs(mSelfSigningCert));
> +    if ((mSelfSigningCert == nullptr)
> +        || (certVerifier->VerifyCert(mSelfSigningCert->GetCert(),

always || (and &&) operator on the previous line

@@ +954,5 @@
> +        mSigningCertDBKey.Truncate();
> +        aIdentity->SetUnicharAttribute("signing_cert_dbkey", mSigningCertDBKey);
> +    }
> +  }
> +  if ((mSelfSigningCert == nullptr) || mSigningCertDBKey.IsEmpty()) {

!mSelfSigningCert

Comment 115

[Kaspar]

Attachment: Patch v9 - support cert selection by nickname and serial number, c-c part

(In reply to Kent James (:rkent) from comment #113)
> If you are just looking for a reviewer, I'd try [e-mail], jcranmer,
> then myself (rkent) in that order.

Ok, let's do this then. Neil, this patch includes two components: a) modifications to the S/MIME settings handling in the Account Manager (selection of certs and their storage via prefs) and b) the companion changes to nsMsgComposeSecure::MimeCryptoHackCerts. The comment in nsMsgComposeSecure.cpp hopefully provides a more detailed description of the suggested changes to the pref handling.

v9 should also address the nits observed by Magnus (thanks for your review).

Comment 116

[Kaspar]

Attachment: Patch v9 - support cert selection by nickname and serial number, m-c part

David, sorry to bother you again - when further testing the c-c part of my patch, I realized that mucking with certNickNameList and certDetailsList in nsCertPicker.cpp is the wrong approach. While the display looks fine, PickByUsage can sometimes return the wrong nsIX509Cert, for those cases where one of the certificates has been filtered out (since after the dialogs->PickCertificate call, selectedIndex is pointing to an incorrect list entry).

The proper way is to filter the list *before* building certNickNameList and certDetailsList, instead. v9 implements this approach - thanks in advance for your review.

Comment 117

[Dana Keeler (she/her) [:keeler]]

Good catch. I might not have time to review this this week, but I'll get to it as soon as possible.

Comment 118

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8625119 [details] [diff] [review]
Patch v9 - support cert selection by nickname and serial number, m-c part

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

Seems like a good approach. r=me with comments addressed.

::: security/manager/ssl/nsCertPicker.cpp
@@ +70,5 @@
> +  /* if a (non-empty) emailAddress argument is supplied to PickByUsage, */
> +  /* remove non-matching certificates from the candidate list */
> +
> +  if (!emailAddress.IsEmpty()) {
> +    CERTCertListNode* freenode = nullptr;

nit: declare this where it's used (i.e. in the !match case)

@@ +133,5 @@
>  
>        if (NS_SUCCEEDED(tempCert->FormatUIStrings(i_nickname, nickWithSerial, details))) {
>          certNicknameList[CertsToUse] = ToNewUnicode(nickWithSerial);
>          certDetailsList[CertsToUse] = ToNewUnicode(details);
> +        if (!selectionFound) {

Why is this necessary? Is this a pre-existing bug?

Comment 119

[Kaspar]

Attachment: Patch v10 - support cert selection by nickname and serial number, m-c part

(In reply to David Keeler [:keeler] (use needinfo?) from comment #118)
> Comment on attachment 8625119 [details] [diff] [review]
> nit: declare this where it's used (i.e. in the !match case)

Done in v10.

> Why is this necessary? Is this a pre-existing bug?

It's for the case where am-smime.js calls PickByUsage with a "new-style" form of the "selectedNickname" argument - nickname+serial instead of nickname only. I have added two comments in v10 in those places which deal with selectedNickname.

Comment 120

[Kaspar]

Comment on attachment 8625118 [details] [diff] [review]
Patch v9 - support cert selection by nickname and serial number, c-c part

(In reply to Kent James (:rkent) from comment #113)
> If you are just looking for a reviewer, I'd try [e-mail], jcranmer,
> then myself (rkent) in that order. If they don't get to the patch in a week
> or two, try the next reviewer.

So, let's ask the next on the list: Joshua, just for the sake of completeness, let me repeat my quick remarks from comment 115: this patch includes two components: a) modifications to the S/MIME settings handling in the Account Manager (selection of certs and their storage via prefs) and b) the companion changes to nsMsgComposeSecure::MimeCryptoHackCerts. The comment in nsMsgComposeSecure.cpp hopefully provides a more detailed description of the suggested changes to the pref handling.

Comment 121

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8625118 [details] [diff] [review]
Patch v9 - support cert selection by nickname and serial number, c-c part

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

I probably don't have time to do a thorough review of this the near future but here's something that popped out at me when I glanced over the review.

(Adding Neil in case he has more review bandwidth than I do).

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +426,5 @@
>    else
>      PR_ASSERT(0);
>  
>    aIdentity->GetUnicharAttribute("signing_cert_name", mSigningCertName);
> +  aIdentity->GetUnicharAttribute("signing_cert_dbkey", mSigningCertDBKey);

If the dbkey is an ASCII value, then you should be using CharAttribute instead of UnicharAttribute.

Comment 122

[[e-mail]]

Comment on attachment 8625118 [details] [diff] [review]
Patch v9 - support cert selection by nickname and serial number, c-c part

Sorry for the delay.

>+  if (!mEncryptionCertDBKey.IsEmpty()) {
>+    certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mEncryptionCertDBKey).get(),
>+                            NULL, getter_AddRefs(mSelfEncryptionCert));
nullptr?

>+    if (mSelfEncryptionCert ||
Well, you have two options here. Either use &&, or use !, and then...

>+  if ((mSelfEncryptionCert == nullptr) || mEncryptionCertDBKey.IsEmpty()) {
... these become redundant; just use
if (!mSelfEncryptionCert) {

>+      nsAString::const_iterator start, end, sep;
>+      mEncryptionCertName.BeginReading(start);
>+      mEncryptionCertName.EndReading(end);
>+      // set the search iterator to the beginning of the string
>+      sep = start;
>+      // and search from the end of the string (RFind)
>+      if (RFindInReadable(NS_LITERAL_STRING(" ["), sep, end)) {
mEncryptionCertName is an nsString, so .RFind(" [") should suffice, no?

>+  // same procedure for the signing cert
>+  if (!mSigningCertDBKey.IsEmpty()) {
>+    certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mSigningCertDBKey).get(),
>+                            NULL, getter_AddRefs(mSelfSigningCert));
>+    if (mSelfSigningCert ||
Ditto.

Comment 123

[Kaspar]

Attachment: Patch v10 - support cert selection by nickname and serial number, c-c part

(In reply to [e-mail] from comment #122)
> Comment on attachment 8625118 [details] [diff] [review]
> Patch v9 - support cert selection by nickname and serial number, c-c part
> 
> >+  if (!mEncryptionCertDBKey.IsEmpty()) {
> >+    certdb->FindCertByDBKey(NS_ConvertUTF16toUTF8(mEncryptionCertDBKey).get(),
> >+                            NULL, getter_AddRefs(mSelfEncryptionCert));
> nullptr?

Yes, corrected.

> >+    if (mSelfEncryptionCert ||
> Well, you have two options here. Either use &&, or use !, and then...

Indeed. After another look, I think that the code in nsMsgComposeSecure.cpp for finding mSelfEncryptionCert and mSelfSigningCert can be considerably simplified compared to what is in v9 of the patch. In v10, it's now directly falling back to the "bare nickname" form if the search by DBKey fails (searching by "nickname [serialnumber]" is highly unlikely to ever return a result).

> >+      nsAString::const_iterator start, end, sep;
> >+      mEncryptionCertName.BeginReading(start);
> >+      mEncryptionCertName.EndReading(end);
> >+      // set the search iterator to the beginning of the string
> >+      sep = start;
> >+      // and search from the end of the string (RFind)
> >+      if (RFindInReadable(NS_LITERAL_STRING(" ["), sep, end)) {
> mEncryptionCertName is an nsString, so .RFind(" [") should suffice, no?

Thanks for pointing this out. Yes, this was way too baroque, an RFind is sufficient.

Additional changes in v10: dbkey is now a CharAttribute (see comment 121), and in am-smime.js, checkOtherCert() now properly updates the DBKeys in the "userWantsSameCert" case.

Comment 124

[[e-mail]]

Comment on attachment 8642129 [details] [diff] [review]
Patch v10 - support cert selection by nickname and serial number, c-c part

Great! Although I'm going to mark this as r+ since it's technically correct, I have however since found one other subtle bug in your patch:
>   if (!otherCertInfo.value.length) {
>-    if (matchingOtherCert) {
>+    if (matchingOtherCert && (matchingOtherCert.dbKey == cert.dbKey)) {
>       userWantsSameCert = askUser(gBundle.getString(msgNeedCertWantSame));
>     }
>     else {
>       if (askUser(gBundle.getString(msgNeedCertWantToSelect))) {
>         smimeSelectCert(pref);
>       }
>     }
>   }
>   else {
>-    if (matchingOtherCert) {
>+    if (matchingOtherCert && (matchingOtherCert.dbKey == cert.dbKey)) {
>       userWantsSameCert = askUser(gBundle.getString(msgWantSame));
>     }
>+    else {
>+      if (askUser(gBundle.getString(msgNeedCertWantToSelect))) {
>+        smimeSelectCert(pref);
>+      }
>+    }
>   }
There are four cases to consider here:
1. The user hasn't selected (an encryption) certificate, and the selected (signing) certificate is valid for (encryption). The user should be prompted to use the same certificate for (encryption).
2. The user hasn't selected (an encryption) certificate, but the selected (signing) certificate is not valid for (encryption). The user should be prompted to select a certificate for (encryption).
3. The user has previously selected an (encryption) certificate, and the selected (signing) certificate is valid for (encryption). The user should be prompted to use the same certificate for (encryption).
4. The user has previously selected an (encryption) certificate, and the selected (signing) certificate is not value for (encryption). The current code does nothing in this case, which is correct. You should not prompt the user for an (encryption) certificate, because the user already has one selected.
Admittedly the code is badly written and might be more readable like this:
if (matchingOtherCert && (matchingOtherCert.dbKey == cert.dbKey)) {
  userWantsSameCert = askUser(gBundle.getString(msgNeedCertWantSame));
}
else if (!otherCertInfo.value.length) {
  if (askUser(gBundle.getString(msgNeedCertWantToSelect))) {
    smimeSelectCert(pref);
  }
}

I'd also like you to consider the following changes:
>     x509cert = picker.pickByUsage(window,
>       certInfo.value,
>       certUsage, // this is from enum SECCertUsage
>-      false, false, canceled);
>+      false, true,
>+      gIdentity.email ? gIdentity.email : null,
>+      canceled);
I looked at the code for pickByUsage in the m-c patch and spotted that it actually checks whether gIdentity.email is empty rather than the null that is documented. Would it be possible to update the documentation so there's no need to coerce empty to null here? (Null is empty but empty isn't null.)

>+var gEncryptionCertDBKey = null;
...
>+var gSignCertDBKey  = null;
Your choice of global variables to store these values leads to some awkward code. I think you would be better off creating extra properties on the existing DOM nodes to store the values. Some examples:

>     gEncryptionCertName.value = gIdentity.getUnicharAttribute("encryption_cert_name");
>+    gEncryptionCertDBKey = gIdentity.getCharAttribute("encryption_cert_dbkey");
gEncryptionCertName.dbKey = gIdentity.getCharAttribute("encryption_cert_dbkey");

>+    otherCertInfo.value = cert.nickname + " [" + cert.serialNumber + "]";
>+    if (email_recipient_cert_usage == usage) {
>+      gEncryptionCertDBKey = cert.dbKey;
>+    } else {
>+      gSignCertDBKey = cert.dbKey;
>+    }
otherCertInfo.dbKey = cert.dbKey;

>   gIdentity.setUnicharAttribute("encryption_cert_name", gEncryptionCertName.value);
>+  gIdentity.setCharAttribute("encryption_cert_dbkey", gEncryptionCertDBKey);
gIdentity.setCharAttribute("encryption_cert_dbkey", gEncryptionCertName.dbKey);

>-  if (otherCertInfo.value == nickname)
>+  if (otherCertInfo.value == (cert.nickname + " [" + cert.serialNumber + "]"))
if (otherCertInfo.dbKey == cert.dbKey)

(In reply to Kaspar Brand from comment #123)
> Indeed. After another look, I think that the code in nsMsgComposeSecure.cpp
> for finding mSelfEncryptionCert and mSelfSigningCert can be considerably
> simplified compared to what is in v9 of the patch. In v10, it's now directly
> falling back to the "bare nickname" form if the search by DBKey fails
> (searching by "nickname [serialnumber]" is highly unlikely to ever return a
> result).
Then would it be better to just save the nickname and not the serial number in the first place? You would still want to display the serial number in the UI, looking it up from the dbKey if necessary, but only saving the actual nickname. For example:

certInfo.value = cert.nickname + " [" + cert.serialNumber + "]";
certInfo.nickname = cert.nickname;
certInfo.dbKey = cert.dbKey;
...
gIdentity.setUnicharAttribute("encryption_cert_name", gEncryptionCertName.nickname);
gIdentity.setCharAttribute("encryption_cert_dbkey", gEncryptionCertName.dbKey);

Comment 125

[Kaspar]

Attachment: Sample user certificate (including private key), S/N 7A:77:3F:FF, keyEncipherment only, valid through May 2017, (no passphrase set)

Attaching an encryption-only certificate to allow further testing of dual-keying setups.

Comment 126

[Kaspar]

Attachment: Sample user certificate (including private key), S/N 00:B2:28:1E:9D, digitalSignature and keyEncipherment, valid through May 2017, (no passphrase set)

And a fifth test certificate, usable for both signing and encryption.

Comment 127

[Kaspar]

Attachment: Patch v11 - support cert selection by nickname and serial number, c-c part

(In reply to [e-mail] from comment #124)
> 4. The user has previously selected an (encryption) certificate, and the
> selected (signing) certificate is not value for (encryption). The current
> code does nothing in this case, which is correct. You should not prompt the
> user for an (encryption) certificate, because the user already has one
> selected.

My original thought was that it would make sense to ask the user in any case (as it's a good opportunity to verify if the selected encryption cert should really be kept), but I have no problem with keeping the current behavior, so I dropped the respective lines.

> Admittedly the code is badly written and might be more readable like this:
> if (matchingOtherCert && (matchingOtherCert.dbKey == cert.dbKey)) {
>   userWantsSameCert = askUser(gBundle.getString(msgNeedCertWantSame));
> }
> else if (!otherCertInfo.value.length) {
>   if (askUser(gBundle.getString(msgNeedCertWantToSelect))) {
>     smimeSelectCert(pref);
>   }
> }

I didn't change this logic, because when implementing your suggestion, we would no longer differentiate between the "msgNeedCertWantSame" and "msgWantSame" messages in the UI (the former has an initial "You should also specify a certificate..." sentence).

To help with further testing, I have attached two additional certs which are suitable for encryption.

> I looked at the code for pickByUsage in the m-c patch and spotted that it
> actually checks whether gIdentity.email is empty rather than the null that
> is documented. Would it be possible to update the documentation so there's
> no need to coerce empty to null here? (Null is empty but empty isn't null.)

Will update the m-c patch once it's clear that this is the only remaining change, yes. In the c-c part, I have now switched to using "" instead of null.

> Your choice of global variables to store these values leads to some awkward
> code. I think you would be better off creating extra properties on the
> existing DOM nodes to store the values.

Very good suggestion, thanks - makes things more straightforward, indeed.

> Then would it be better to just save the nickname and not the serial number
> in the first place? You would still want to display the serial number in the
> UI, looking it up from the dbKey if necessary, but only saving the actual
> nickname. For example:
> 
> certInfo.value = cert.nickname + " [" + cert.serialNumber + "]";
> certInfo.nickname = cert.nickname;
> certInfo.dbKey = cert.dbKey;

Another very good idea. I've implemented this in v11, with the following logic: in smimeInitializeFields(), try to figure out the serial number by looking up the cert via findCertByDBKey, and if found, adapt .value accordingly and remember the nickname in .nickname. When updating a setting, make sure that both .value and .nickname are set accordingly, and in smimeSave(), preferrably use .nickname for storing as the pref.

In nsMsgComposeSecure.cpp, I dropped the code which tried cutting off the " [...]" part in the *_cert_name prefs (no longer needed), and changed the *_cert_dbkey code to only clear the pref if it's an unsuitable cert (otherwise, nsMsgComposeSecure::MimeCryptoHackCerts would potentially clear dbkey prefs when a cert is not found because a token is not inserted and such).

Comment 128

[[e-mail]]

(In reply to Kaspar Brand from comment #127)
> I didn't change this logic, because when implementing your suggestion, we
> would no longer differentiate between the "msgNeedCertWantSame" and
> "msgWantSame" messages in the UI (the former has an initial "You should also
> specify a certificate..." sentence).
Whoops, sorry about that, I'd meant to include a ?: to choose the correct message.

Comment 129

[[e-mail]]

Comment on attachment 8650915 [details] [diff] [review]
Patch v11 - support cert selection by nickname and serial number, c-c part

Sorry for the delay but I've been ill.

>+  gIdentity.setUnicharAttribute("encryption_cert_name", gEncryptionCertName.nickname ?
>+                                gEncryptionCertName.nickname : gEncryptionCertName.value);
Nit: use gEncryptionCertName.nickname || gEncryptionCertName.value

>+  gIdentity.setUnicharAttribute("signing_cert_name", gSignCertName.nickname ?
>+                                gSignCertName.nickname : gSignCertName.value);
(ditto)

>+  if ((otherCertInfo.dbKey && (otherCertInfo.dbKey == cert.dbKey)) ||
>+      (otherCertInfo.value == cert.nickname))
So, if I've got this right, we're comparing dbKeys, but we're also comparing the other value against our nickname, because it might still be using the old preference?
Nit: the otherCertInfo.dbKey && check seems redundant here.

>+      gIdentity.email ? gIdentity.email : "",
Nit: the ? : is redundant here, just use gIndentity.email

Comment 130

[Kaspar]

Attachment: Patch v12 - support cert selection by nickname and serial number, c-c part

(In reply to [e-mail] from comment #129)
> Sorry for the delay

No worries, I have been slow with reacting to your reviews anyway, so no particular need to hurry.

Thanks for catching my nits, corrected in v12.

> >+  if ((otherCertInfo.dbKey && (otherCertInfo.dbKey == cert.dbKey)) ||
> >+      (otherCertInfo.value == cert.nickname))
> So, if I've got this right, we're comparing dbKeys, but we're also comparing
> the other value against our nickname, because it might still be using the
> old preference?

Good point. After having realized that checkOtherCert() is only called after a dbKey has been set via smimeSelectCert(), it doesn't make sense to fall back to comparing bare nicknames, actually. Changed to a simple "if (otherCertInfo.dbKey == cert.dbKey)" therefore.

Comment 131

[Kaspar]

Attachment: Patch v12 - support cert selection by nickname and serial number, m-c part

(In reply to Kaspar Brand from comment #127)
> (In reply to [e-mail] from comment #124)
> > I looked at the code for pickByUsage in the m-c patch and spotted that it
> > actually checks whether gIdentity.email is empty rather than the null that
> > is documented. Would it be possible to update the documentation so there's
> > no need to coerce empty to null here? (Null is empty but empty isn't null.)
> 
> Will update the m-c patch once it's clear that this is the only remaining
> change, yes.

Keeler, there's just a tiny little comment change (wording) in this updated patch, but it still needs an r+ from you I assume. Note that there's no v11 for the m-c part, as I thought it is more consistent to use the same numbering as for the latest c-c patch.

Comment 132

[Kaspar]

Could someone (Magnus perhaps?) push this to try-comm-central for me? I guess that's the next step before I'm allowed to set the checkin-needed keyword. Thanks!

Comment 133

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 134

[Kaspar]

(In reply to Kaspar Brand from comment #132)
> Could someone (Magnus perhaps?) push this to try-comm-central for me?

Magnus, could you do me this favor...? Thanks a lot.

Comment 135

[Magnus Melin [:mkmelin]]

Applying patches to mozilla-central too is a bit complicated. Hopefully I got it correct. (Expect a bunch of failures, only see if there's new ones...)

remote: View your changes here:
remote:   https://hg.mozilla.org/try-comm-central/rev/20210eab20cf
remote:   https://hg.mozilla.org/try-comm-central/rev/71f1b61a5ad0
remote:   https://hg.mozilla.org/try-comm-central/rev/ff4e7e4aba5d
remote:   https://hg.mozilla.org/try-comm-central/rev/d4e13a025eb6
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d4e13a025eb6


Should still also send the m-c patches to mozilla-central try. I'll do that a bit later...

Comment 136

[Magnus Melin [:mkmelin]]

m-c try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=400ad0091d6b

Comment 137

[Kaspar]

Many thanks, Magnus. Compared to the try builds appearing in the queue just before those from comment 135 and comment 136, the two patches don't introduce new failures, AFAICT. Adding the checkin-needed keyword therefore.

Comment 138

[aleth [:aleth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/540c6bc5346e87905d80bcd0c47d554480f81946
Bug 278689 - Multiple Certificates with the same subject are not shown in the digital signature select cert combo (only one is shown) r=dkeeler

Comment 139

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/540c6bc5346e

Comment 140

[Kaspar]

Thanks for checking in the mozilla-central part (attachment 8657443 [details] [diff] [review])!

What still needs checkin, however, is the comm-central part - i.e. attachment 8657442 [details] [diff] [review]. Reopening and adding checkin-needed again.

Comment 141

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/7794a853df8d08677ae04d81629dfea355af7d11
Bug 278689 - Multiple Certificates with the same subject are not shown in the digital signature select cert combo (only one is shown) r=neil

Comment 143

[Nicolas T]

I think Martin Pecka found a related bug more than a year ago, but no one saw it even now that I've confirmed it.

Maybe we can have some of your help there?

It's Bug 1371243

Comment 144

[chico]

I am facing a similar problem with Thunderbird 78. I used to be able to select a signing certificate stored in Smartcard in Thunderbird 68, but after upgrading to 78, I can only select encryption cert from my smartcard. Whenever I try to select a certificate for signing - Thunderbird always replies: "Certificate Manager can't locate a valid certificate that can be used to digitally signing your messages with an address of <[e-mail]>". The problem is the certificate is there if I browse through the Certificate Manager. The only issue is that I have separate encryption and signing certificates with the same subject name, just different key usage options - it may cause confusion to Thunderbird.

In Thunderbird 68, I used to be able to workaround by first selecting the encryption cert and then select the signing certificate. But in Thunderbird 78, I just can't do that.

What kind of diagnostic dump can I post here to troubleshoot this issue?

Comment 145

[Kai Engert [:KaiE:]]

Please file a new bug instead. If you believe it's similar related to an old bug, you can add a reference to an old bug, for example using the "see also" bugzilla attribute.

If you can, in the new bug, please attach a copy of your certificates (excluding the private keys).
Display the cert in cert manager, and save the chain to a file.