370136 - Firefox 2.0.0.1 and later breaks automatic client certificate authentification

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Gilles]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

When automatic client certificate selection is activated in Firefox 2.0.0.1 in advanced options / cryptography (default), client certificate authentification fails with code -12227 when there are two or more valid certificates for a website.
Authentificating by manually selecting one of the certificate succeeds.
Firefox 2.0 did not show this bug.


Reproducible: Always

Steps to Reproduce:
1)Launch Firefox 2.0.0.1 
2)Import two or more personal certificates into Firefox.
These certificates must be valid for the website you want to browse.
Note : "valid" means valid for Firefox. I.e. certificate revocation does not matter here since Firefox will not know it. So valid means : "from the good CA" and "not expired"
3) Make sure automatic client certificate selection is activated in advanced options / cryptography
4) Browse a website that requires the imported certificates for client certificate authentification

Actual Results:  
Authentification will fail with error code "-12227"


Expected Results:  
Authentification should succeed with one of the certificates


People having the problem and talking about it on geckozone :
http://www.geckozone.org/forum/viewtopic.php?t=49104&postdays=0&postorder=asc&start=0

Products that show this bug :
- Firefox 2.0.0.1 on Windows XP SP2 and Mac OS X 10.4
- Seamonkey 1.1 on Windows XP SP2
- Firefox 3.0 alpha 2 on Windows XP SP2
- probably more...

This bug may be related to the following bug https://bugzilla.mozilla.org/show_bug.cgi?id=364587

Comment 1

[Momtchil Momtchev]

This bug was introduced by the patch fixing 364587

You need at least 2 certificates with the nonRepudiation bit set in order to trigger it

Here's a patch which solves it:


===================================================================
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v
retrieving revision 1.97.2.15
diff -u -p -b -u -8 -p -r1.97.2.15 nsNSSIOLayer.cpp
--- nsNSSIOLayer.cpp    17 Dec 2006 02:37:44 -0000      1.97.2.15
+++ nsNSSIOLayer.cpp    15 Feb 2007 18:03:23 -0000
@@ -2254,16 +2254,17 @@ SECStatus nsNSS_SSLGetClientAuthData(voi
       }

       node = CERT_LIST_NEXT(node);
     }

     if (!cert && low_prio_nonrep_cert) {
       cert = low_prio_nonrep_cert;
       low_prio_nonrep_cert = NULL; // take it away from the cleaner
+      privKey = PK11_FindKeyByAnyCert(cert, wincx);
     }

     if (cert == NULL) {
         goto noCert;
     }
   }
   else {
     /* user selects a cert to present */

Comment 2

[Kai Engert (:KaiE:)]

Momtchil Momtchev, thanks a lot for your comment.
Your patch seems correct to me.

But you are refering to the change we made with bug 328346.
Bug 364587 might be a duplicate of this bug.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 4

[Kai Engert (:KaiE:)]

Bob, this is a regression, could you please review?

We had changed the loop to no longer exit on the first found cert, but possibly remember the first good candidate.

When finally deciding to take a remembered candidate cert, we must obtain the private key again - as this is a return parameter of the function.

Comment 5

[Kai Engert (:KaiE:)]

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

r=kengert

Comment 6

[Robert Relyea]

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

r+ assuming we fix reference leak with privKey.

There are 2 leaks here:
The first leak is an existing leak in 2.0.0.1:

When we determine the certificate is a low_prio_nonrep_cert, we should free the privKey (and NULL it out).

The second leak is the privKey we clobber with the new PK11_FindKeyByAnyCert. If we fix the first leak, privkey should be NULL in the new case.

bob

Comment 7

[Kai Engert (:KaiE:)]

(In reply to comment #6)
> There are 2 leaks here:
> The first leak is an existing leak in 2.0.0.1:
> 
> When we determine the certificate is a low_prio_nonrep_cert, we should free the
> privKey (and NULL it out).

Ok, agreed.


> The second leak is the privKey we clobber with the new PK11_FindKeyByAnyCert.
> If we fix the first leak, privkey should be NULL in the new case.

I don't see the second leak.
The privKey gets returned to the caller.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 9

[Robert Relyea]

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

r+ = relyea
yup, that does the trick.

Comment 10

[Kai Engert (:KaiE:)]

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

Is it still time to get this into 1.8.1.2 ?

Comment 11

[Kai Engert (:KaiE:)]

Patch checked in to the trunk, as well as a required #include statement, marking fixed.

Momtchil Momtchev, thanks again for your help!

Comment 12

[Momtchil Momtchev]

(In reply to comment #2)

> But you are refering to the change we made with bug 328346.
> Bug 364587 might be a duplicate of this bug.

      Sorry, my fault. Yes, bug 364587 looks like a duplicate and bug 328346 introduced the bug.

      Thanks for the speedy answer. Does this mean that we'll have this fix in 2.0.0.2?

      There's also another discussion going here, about whether it's OK to honor the NR flag when the extension isn't critical (it is supposed to always be critical according to RFC3280). It seems IE ignores it completely and it uses only the Extended Key Usage. Can someone confirm me this?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Kai, does this fix also fix bug 364587 ?

Comment 14

[Tristan Nitot]

Just a bit of context: in two weeks from now, the impots.gouv.fr (French equivalent of the IRS) website will be used a lot for the annual declaration of taxes. This service's usage is quite amazing. People have to log in using a certificate delivered by the site. Since it's an annual process, people are likely to forget that they already requested a cert the previous year, and ask again for another cert, hence getting two of them, which triggers the regression documented on this bug report.

Of course, people will log in using either a competing product, or will go through the tedious task of calling the IRS tech support which will explain how to manually select a cert in order to log in.

In short, this is going to be a terrible user experience, potentially reaching several hundreds thousands users in a single country, France.

Therefore, it would be nice to have this in Firefox 2.0.0.2.

Comment 15

[Daniel Veditz [:dveditz]]

unfortunately this isn't going to make 1.8.1.2 (FF2.0.0.2), we're already into our 4th release candidate which is already 3 stop-ship problems more than we want in a security update release.

Let's get this into a nightly and test this area better to make sure there aren't more regressions hiding in here, and ship a clean 1.8.1.3

Comment 16

[Tim Riley [:timr]]

(In reply to comment #14)
Wow, this is very bad.  But as dveditz says, we have already delayed 2.0.0.2 many .  Important security fixes are already about 2-3 weeks later the expected.  Is there any way to delay the French release of 2.0.0.2 and include a fix for this?

Comment 17

[Kai Engert (:KaiE:)]

Tristan, I have a question. Do the certificates issued by impots.gouv.fr have a "non-repudation" usage set?

If you happen to own such a certificate, could you please go to advanced prefs, open certificate manager, look at the details of your certificate, and find details like "key usage" or "certificate usage"?

Is "non-repudiation" listed explicitly anywhere in those sections?

Comment 18

[Kai Engert (:KaiE:)]

I just had the chance to look at such a certificate on the laptop of a french person, and unfortunately the non-repudiation bit is set explicitly.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

2.0.0.2 is released, this patch should get approval for the 1.8.0.3 branch since it is impacting up to 1 million Firefox users in France. If we are forced to release a 2.0.0.3 version early and this patch is included, most of the damages would be avoided (people always pay their taxes at the last moment...)

Comment 20

[Pascal Chevrel:pascalc (PTO until April 26)]

sorry, meant 1.8.1.3 branch of course :)

Comment 21

[Marcia Knous [:marcia]]

Has someone actually verified this fix on the trunk?

Comment 22

[Kai Engert (:KaiE:)]

Gilles, Momtchil, I think you are both able to reproduce the original failure.

Could you please try to run a nightly build from
  ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
and verify it is working for you?

(pick the build for your platform that has the most recent date, thanks)

Comment 23

[Momtchil Momtchev]

Yes, I confirm that the nightly builds fix that issue.

Comment 24

[Momtchil Momtchev]

However the latest 2.0.0.3 prerelease is still broken
(http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/), build from 4th March, 18:41.

Comment 25

[Gilles]

I confirm the comments from Momtchil.

Latest nightly build fixes the problem :
- version tested : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070304 Minefield/3.0a3pre
- more than two non-repudiation certificates in the user store
- url tested : www.impots.gouv.fr
=> result : authentification succeeds. The user is able to access his personal area of the website

Latest 2.0.0.3 pre still shows the problem :
- version tested : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070304 BonEcho/2.0.0.3pre
- more than two non-repudiation certificates in the user store
- url tested : www.impots.gouv.fr
=> result : authentification fails with a popup showing an error code "-12227". The user is unable to access his personal area of the website

Comment 26

[Kai Engert (:KaiE:)]

Thanks a lot for your help in testing the fix.

What you report is as expected.
The nightly trunk/1.9a* builds contain the fix, and you report they work.

The latest 2.0.0.3 nightly builds do not yet include the fix, so it is expected that you still get a failure with those.

Comment 27

[David :Bienvenu]

According to Kaie, we want this fix for TB 2.0, so it would be nice to get approval for the 1.8.1 branch sooner rather than later (today would be ideal). It might help bug 368611 for us.

Comment 28

[Mike Connor [:mconnor]]

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

a=mconnor on behalf of drivers for 1.8.1.3

Comment 29

[Kai Engert (:KaiE:)]

checked in to mozilla_1_8_branch

Comment 30

[Marcia Knous [:marcia]]

Kai: From a QA perspective, is there anything specific we should focus on in our regression testing? We can't easily verify this fix, so would be helpful to know if you have any other pointers. Thanks.

Comment 31

[Kai Engert (:KaiE:)]

(In reply to comment #30)
> Kai: From a QA perspective, is there anything specific we should focus on in
> our regression testing? We can't easily verify this fix, so would be helpful to
> know if you have any other pointers. Thanks.

Yes, this is difficult to verify, because it requires a special setup, a combination of client certs and a matching server.

The fix of this patch only affects sites that use certificate based login.

Unless you have such a setup, you will not see any difference.

I propose we take the answers from Momtchill and Gilles as VERIFIED.

Should you have candidate builds of Firefox 2.0.0.x, we might want to send them to Momtchill and Gilles and ask them to verify.

Comment 32

[Momtchil Momtchev]

The latest 2.0.0.3 prerelease seems to have to bugfix included, it works for me.
BonEcho 2.0.0.3pre, build from 07-Mar-2007 05:46 from 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/

Comment 33

[Robert Relyea]

It may be useful for mozilla qa to have a client auth server which accepts certs from a local mozilla qa certificate server. Then we could set up different profiles to test these client auth cases and make sure we don't have any regressions.

bob

Comment 34

[Marcia Knous [:marcia]]

Bob: If you can give us some pointers on this or point us in the right direction to get started, that would be great. I agree this would be useful to have down the road for future testing. Thanks.

(In reply to comment #33)
> It may be useful for mozilla qa to have a client auth server which accepts
> certs from a local mozilla qa certificate server. Then we could set up
> different profiles to test these client auth cases and make sure we don't have
> any regressions.
> 
> bob
> 

Comment 35

[Gilles]

I confirm that the latest 2.0.0.3pre fixes the problem :
- version tested : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070312 BonEcho/2.0.0.3pre
- more than two non-repudiation certificates in the user store
- url tested : www.impots.gouv.fr
=> result : authentification succeeds. The user is able to access his personal
area of the website

Comment 36

[Marcia Knous [:marcia]]

adding ray to the bug to contemplate Comment 33.

Comment 37

[Kai Engert (:KaiE:)]

Could you please confirm that Firefox 2.0.0.3 final has this bug fixed?

If yes, I would like to remove the "blocks 1.8.1.4" flag in order to take it off the 1.8.1.4 radar.

Comment 38

[Momtchil Momtchev]

Yes, Firefox 2.0.0.3 has the bug fixed.

Comment 39

[Kai Engert (:KaiE:)]

Thanks Momtchil!

Setting verified1.8.1.3 flag and removing blocking1.8.1.4 flag.