Closed Bug 370136 Opened 13 years ago Closed 13 years ago

Firefox 2.0.0.1 and later breaks automatic client certificate authentification

Categories

(Core :: Security: PSM, defect, major)

1.8 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: gurv, Assigned: KaiE)

References

()

Details

(Keywords: regression, verified1.8.1.3)

Attachments

(1 file, 1 obsolete file)

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
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 */
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.

Status: UNCONFIRMED → NEW
Depends on: 328346
Ever confirmed: true
Version: Trunk → 1.8 Branch
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #255266 - Flags: review?(rrelyea)
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.
Keywords: regression
Comment on attachment 255266 [details] [diff] [review]
Patch v1

r=kengert
Attachment #255266 - Flags: review+
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
Attachment #255266 - Flags: review?(rrelyea) → review+
(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.
Attached patch Patch v2Splinter Review
Attachment #255266 - Attachment is obsolete: true
Attachment #255280 - Flags: review?(rrelyea)
Comment on attachment 255280 [details] [diff] [review]
Patch v2

r+ = relyea
yup, that does the trick.
Attachment #255280 - Flags: review?(rrelyea) → review+
Comment on attachment 255280 [details] [diff] [review]
Patch v2

Is it still time to get this into 1.8.1.2 ?
Attachment #255280 - Flags: approval1.8.1.3?
Attachment #255280 - Flags: approval1.8.1.2?
Patch checked in to the trunk, as well as a required #include statement, marking fixed.

Momtchil Momtchev, thanks again for your help!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.3?
(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?
Kai, does this fix also fix bug 364587 ?
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.
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
(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?

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?
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.
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...)
sorry, meant 1.8.1.3 branch of course :)
Attachment #255280 - Flags: approval1.8.1.2?
Has someone actually verified this fix on the trunk?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.1.3+
Attachment #255280 - Flags: approval1.8.1.3?
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)
Yes, I confirm that the nightly builds fix that issue.
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.
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
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.
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 on attachment 255280 [details] [diff] [review]
Patch v2

a=mconnor on behalf of drivers for 1.8.1.3
Attachment #255280 - Flags: approval1.8.1.4?
Attachment #255280 - Flags: approval1.8.1.3?
Attachment #255280 - Flags: approval1.8.1.3+
checked in to mozilla_1_8_branch
Keywords: fixed1.8.1.3
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.
(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.
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/
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
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
> 

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
adding ray to the bug to contemplate Comment 33.
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.
Yes, Firefox 2.0.0.3 has the bug fixed.
Thanks Momtchil!

Setting verified1.8.1.3 flag and removing blocking1.8.1.4 flag.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.4+
QA Contact: psm
You need to log in before you can comment on or make changes to this bug.