403910 - CERT_FindUserCertByUsage() returns wrong certificate if multiple certs with same subject available

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Angelo Rosenfelder]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre
Build Identifier: NSS used by Mozilla Thunderbird Version 2.0.0.9

I have two certificates on a smart card, which are nearly identical, except that they have a different CKA_Label. Most notably, they have the same CKA_Subject.

No matter with which nickname I call CERT_FindUserCertByUsage(), it returns always the same certificate, which in half of the cases is one with another nickname than the requested certificate.


Reproducible: Always

Steps to Reproduce:
1. Import 2 email signing certificates with same CKA_Subject, but different CKA_Label in Thunderbird
2. Select one certificate for signing
3. Sign an email
4. Repeat step 2. and 3. with the other certificate

Actual Results:  
In both cases, CERT_FindUserCertByUsage() gets called with the right nickname as argument.
In both cases, CERT_FindUserCertByUsage() returns the same certificate.
Both emails have been signed with the same certificate.


Expected Results:  
CERT_FindUserCertByUsage() should return a certificate with the same nickname as supplied in the argument.
Both emails should have been signed with the certificate which has been selected.


In the comment to CERT_FindUserCertByUsage() it explains argument 'nickname' as following:
 * "nickname" - nickname to match

So I expect this function to return a certificate with the same usage _and_ the same nickname as in the arguments.

IMO there are 2 possible solutions to this problem:
1. In CERT_FindUserCertByUsage(), call CERT_CreateNicknameCertList() instead of CERT_CreateSubjectCertList().
   CERT_CreateNicknameCertList() only has a function prototype at the moment.

2. In addition to CERT_FilterCertListByUsage(), do also a CERT_FilterCertListByNickname()
   CERT_FilterCertListByNickname() would be a new function.

I'm ready to provide you with a patch to solution 2 if this is a good solution.

Comment 1

[Martin Vicentini]

I have a similar token with two certificates with the same CKA_SUBJECT but different CKA_LABEL.
For signing an email I can select both of the certificates, but Thunderbird uses always the same one.

Behaviour confirmed!

I'd go for the second solution from Angelo.

Comment 2

[Angelo Rosenfelder]

Attachment: First proposal for a fix.

Filters the certlist not only by usage, but also by nickname. This should not destroy existing behaviour, but will fix the issue described in this bug.

Comment 3

[Wan-Teh Chang]

Angelo, Bob and Nelson should look at the patch.

I guess that the current code is intended to create
the illusion that certs with the same subject are one
cert, and Thunderbird may depend on this.  For example,
when you select a signing cert in Thunderbird, it will
ask you if you want to use "the same certificate"
for encryption.  If I understand the Thunderbird code
correctly, the following links trace the message
"You should also specify a certificate for other people
to use when they send you encrypted messages. Do you
want to use the same certificate to encrypt & decrypt
messages sent to you?" eventually to a call to the
CERT_FindUserCertByUsage function:

http://lxr.mozilla.org/seamonkey/search?string=encryption_needCertWantSame
http://lxr.mozilla.org/seamonkey/search?string=checkOtherCert
http://lxr.mozilla.org/seamonkey/search?string=findEmailEncryptionCert
http://lxr.mozilla.org/seamonkey/ident?i=FindEmailEncryptionCert
http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#1370

If that's the case, then your patch will break this.

I think we can maintain the current behavior by checking
if 'cert' is a user cert and has the correct usage right
after here:
http://lxr.mozilla.org/seamonkey/source/security/nss/lib/certhigh/certhigh.c#288

287     if ( cert != NULL ) {
288         /* collect certs for this nickname, sorting them into the list */

without constructing 'certList'.

Bob, Nelson, what do you think?

Comment 4

[Wan-Teh Chang]

I should elaborate on the "illusion" I referred to.
It is common practice to issue a pair of signing and
encryption certs to each user.  I believe that Netscape
and Netscape-derived clients such as Thunderbird try
to hide this complexity from the user and create an
abstraction that there is only one cert.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Any user facing text that read "the same cert" is bad text. 
It should read "cert with the same subject name".
That would be a PSM security UI bug

Comment 6

[Robert Relyea]

So the semantic within NSS is that subject name certs all point to the same identity. Great lengths are taken to make sure that these certs all have the same nickname (label). Changing the behavior of CERT_FindUserCertByUsage() will break applications that depend on it.

The problem here is that PKCS #11 modules can (and often do) have different labels for different certs leading to confusion. The current answer is 'don't depend on the label'. If you need to reference a specific cert, do so by issuer/SN. Most modern apps dispense with nickname altogether. If you are seeing 2 different nicknames in PSM, that would be a bug.

If you have a case where it is truly ambiguous, your app should use CERT_FindUserCertsByUsage(), have the user select from the list, then store the issue/sn for that exact cert and use just that cert. The reason for this is NSS will set the label/subject_name the same when it writes a cert, so if you need to disambiguate between 2 certs of the same subject and usage, you can't depend on nickname.

That being said, if there is a case where CERT_FindUserCertByUsage is returning the wrong cert for that context, that would be a bug. That is, if you have 2 or more certs with the same subject, those certs *should* have different purposes, but the same 'privilege'  (that is the case where one cert is valid for authentication, another for key exchange is OK, but the case where one cert is for authentication with privilege 'A' and another cert with authentication with privilege 'B' is not ok). So if there are cases where the wrong cert is being returned based on the usage, then that would be a bug in CERT_FindUserCertByUsage().

Unfortunately this isn't well documented, and there is a lot of historical cruft we are carrying along (sigh).

[ on ambiguous case is email signing. Do you sign with the authentication cert or the non-repudation cert. The old philosophy was email signing was document signing, so you use the non-repudation cert. The modern thought is it is really just part of the protocol, and is really an authentication signing operation. I'm pretty sure PSM hasn't made a final decision on this ].

bob

Comment 7

[Robert Relyea]

Comment on attachment 290385 [details] [diff] [review]
First proposal for a fix.

r- for this patch, but adding a CERT_FilterCertListByNickname() as an exported function might be an ok addition. My worry is it extends the fiction that apps should use nicknames to identify individual certificates.

bob

Comment 8

[Wan-Teh Chang]

As part of the fix for this bug, we should document what
CERT_FindUserCertByUsage is supposed to do in the header
file.  Bob, could you do that?

The application in question is Thunderbird.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #6)
> So the semantic within NSS is that subject name certs all point to the same
> identity. 

Bob, what are "subject name certs"?

> Great lengths are taken to make sure that these certs all have the
> same nickname (label). Changing the behavior of CERT_FindUserCertByUsage() 
> will break applications that depend on it.

AFAIK, NSS's softoken PKCS11 module assumes that all certs with the same 
subject name must/will have the same CKA_LABEL (name "nickname"), but 
clearly no other vendor's PKCS#11 module makes this same assumption.  

Does NSS make this assumption elsewhere than in Softoken? 
Is there any good reason that NSS should make this assumption outside of 
Softoken?  

Comment 10

[Angelo Rosenfelder]

So the comment for CERT_FindUserCertByUsage() is misleading, since it says: 'nickname to match'.
But I think I understand the mechanism now and accept that my patch would break some things.

Bob, in your comment #6 you mentioned the ambiguous case of email signing. Until this final decision is taken, I think you should be able to at least select wether you want to choose the non-repudiation cert or the auth cert. The UI even gives you this choice, but ignores this decision as described in this bug (this is what I meant with 'returns wrong certificate').

Wan-Teh Chang mentioned in his comment #3 a possible solution. If {PK11,CERT}_FindCertByNickname() already returns a cert which is a user cert and has the correct usage, just return this cert. In the other case, construct the list and filter it as the function does it now.
If I understood everything correctly, there cannot be 2 certs with the same subject _and_ the same usage, so the cert returned before constructing the list would also be the cert returned after constructing and filtering the list (except for the case which triggered this issue). In all other cases the function acts as it does now anyway.

So this fix wouldn't break anything (AFAICS), but will fix my issue of not being able to sign mails with my auth cert.


Here are the changes I would make (around line #284):


     if (cert == NULL) {
         cert = CERT_FindCertByNickname(handle,nickname);
     }
 
     if ( cert != NULL ) {
+        rv = CERT_KeyUsageAndTypeForCertUsage(usage,
+                                              PR_FALSE,
+                                              &requiredKeyUsage,
+                                              &requiredCertType);
+        if ( rv != SECSuccess ) {
+            goto loser;
+        }
+
+        /* If we already found the right cert, just return it */
+        if ( (CERT_MatchNickname(cert->nickname, nickname) ) &&
+             (CERT_CheckKeyUsage(cert, requiredKeyUsage) == SECSuccess) &&
+              CERT_IsUserCert(cert) )
+        {
+            return(cert);
+        }
+
         /* collect certs for this nickname, sorting them into the list */
         certList = CERT_CreateSubjectCertList(certList, handle, 
                                               &cert->derSubject, time, validOnly);
 
         CERT_FilterCertListForUserCerts(certList);

Comment 11

[Wan-Teh Chang]

Angelo, thank you for creating a patch for my suggested fix in comment 10.
I believe that the CERT_MatchNickname call is guaranteed to return PR_TRUE
and therefore is not necessary because |cert| is returned by either
PK11_FindCertFromNickname or CERT_FindCertByNickname.

Could you attach this patch as an attachment?  It'll be easier for us to review.

Comment 12

[Robert Relyea]

Nelson,

Softoken's assumption of nickname == subject name was inherited from NSS proper. NSS itself assumes that all certs with the same subject DN are represents the same entity.

When NSS writes new cert/key objects is uses that exact semantic for the label. Not all applications force label to match subject, so NSS treats any label mapping to the same subject as identical (that is why CERT_FindCertByUsage calls looks up a single cert by nickname, then uses that cert to search for all certs with that subject. If label == subject was always true, we could simple search for all certs with the same label).

Angelo, 

Even if we 'nail down the ambiguity', there is probably the need to allow the user to override the choice.

The proper way of doing that would be to take the cert the user specified in the drop down and access that certificate by issuer/serial # (assuming the cert has appropriate usage). We can guarantee the latter by only including certs with the correct usage. PSM already does this for SSL client auth, it looks like email was never updated. Matching nicknames only won't allow you to match 2 ambiguous certs that happen to have the same nicknames (because NSS created them).

Comment 13

[Kaspar]

(In reply to comment #12)
> The proper way of doing that would be to take the cert the user specified in
> the drop down and access that certificate by issuer/serial # (assuming the cert
> has appropriate usage). We can guarantee the latter by only including certs
> with the correct usage. PSM already does this for SSL client auth, it looks
> like email was never updated.

This is what I suggested with my patch for bug 278689 (attachment 277289 [details] [diff] [review]), but with limited success so far, unfortunately. The proposed changes to nsMsgComposeSecure will solve Angelo's problem, but getting a patch accepted for bug 278689 seems to have become quite difficult, since the more recent comments turn that bug mostly into a UI/usability issue (maybe it would help to separate the backend changes [nsMsgComposeSecure and nsCertPicker] from the frontend changes and try to get them in first?).

Comment 14

[Angelo Rosenfelder]

Attachment: Patch from comment #10

Patch created from the proposal mentioned in comment #3, #10 and #11.

Ok, I attached this patch as an attachment. The only part where the old behaviour could be changed is when CERT_KeyUsageAndTypeForCertUsage() causes the function to return no certificate. But AFAICS this cannot happen in a situation where the original function wouldn't fail too.

Comment 15

[Angelo Rosenfelder]

Bob,

may I ask you if you will be reviewing this patch? And if yes, when do you think you are going to do it?

Because there are quite some people who will be glad to be able to use their token with mozilla products.

Thanks!

Comment 16

[Robert Relyea]

Comment on attachment 290872 [details] [diff] [review]
Patch from comment #10

The only thing wrong with this patch is it continues the fiction that nickname is a reliable way of identifying and individual cert.

I do prefer seeing the work Kaspar is doing on  bug 278689 as the final solution.

bob

Comment 17

[Wan-Teh Chang]

Attachment: Patch v2

I found that Angelo's patch is missing two checks:
- expiration
- cert type

This patch adds these checks and also fixes the formatting.
Angelo, could you test this patch?

I think a better approach would be to just search for the
original 'cert' in 'certList' at the end of this function,

Comment 18

[Wan-Teh Chang]

Attachment: Alternative patch

Angelo, could you test this patch, too?  This patch implements the
alternative approach I described at the end of comment 17.

Comment 19

[Robert Relyea]

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

r+ good catch wan-teh

Comment 20

[Wan-Teh Chang]

I checked in patch v2 (attachment 297467 [details] [diff] [review]) on the NSS trunk for NSS 3.12.

Checking in certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.43; previous revision: 1.42
done

Comment 21

[Angelo Rosenfelder]

I tested both patches, and both of them worked well. As I can see, you already have checked in patch v2.
Thanks a lot.

Comment 22

[Wan-Teh Chang]

Comment on attachment 297472 [details] [diff] [review]
Alternative patch

Bob, what's your preference between this patch and patch v2?

I think this patch is easier to maintain than patch v2 because
this patch doesn't depend on how the cert list is constructed
and filtered.

However, this patch does assume that the CERTCertificate* pointers
for the same cert have the same value.  It relies on this assumption
to look for the cert found by nickname in the cert list (see the
while loop at the end).  I'm not sure if that assumption is valid.

Comment 23

[Robert Relyea]

Comment on attachment 297472 [details] [diff] [review]
Alternative patch

yes, this is slightly better than the previous patch. 

r+