Last Comment Bug 670454 - Certificates usage in Certificate Viewer is always shown as "could not verify this certificate for unknown reasons"
: Certificates usage in Certificate Viewer is always shown as "could not verify...
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: 6 Branch
: All Other
: -- blocker (vote)
: mozilla8
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks: psm-pkix
  Show dependency treegraph
 
Reported: 2011-07-09 11:58 PDT by Uwe Geuder
Modified: 2011-07-29 09:38 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
screen shots: Aurora 7.0a2 top, FF 3.6.18 bottom (169.11 KB, image/png)
2011-07-09 11:58 PDT, Uwe Geuder
no flags Details
about:support (1.47 KB, text/plain)
2011-07-09 12:01 PDT, Uwe Geuder
no flags Details
about:buildconfig (1.27 KB, text/plain)
2011-07-13 10:27 PDT, Uwe Geuder
no flags Details
Fix usage display in cert viewer for non-libpkix case only (3.13 KB, patch)
2011-07-20 08:25 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review-
Details | Diff | Splinter Review
Patch v1 - restore the old behaviour (2.68 KB, patch)
2011-07-20 08:28 PDT, Kai Engert (:kaie)
brian: review+
Details | Diff | Splinter Review
Patch v1 with comment added (2.89 KB, patch)
2011-07-20 09:11 PDT, Kai Engert (:kaie)
kaie: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Uwe Geuder 2011-07-09 11:58:20 PDT
Created attachment 545008 [details]
screen shots: Aurora 7.0a2 top, FF 3.6.18 bottom

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:7.0a2) Gecko/20110709 Firefox/7.0a2
Build ID: 20110709042004

Steps to reproduce:

Start Aurora with a new Profile just to be sure
Open Menu Preferences 
Select  Advanced | Encryption , View Certificates
Select tab certificates, scroll down to Thawte
Select first Thawte Certificate, click "view..." button 

(Actually the problem is not specific to this single certificate. I tried many more built-in root certificates and they all had the same issue.)



Actual results:

Certificate Viewer opens, it says "could not verify this certificate for unknown reasons"


Expected results:

Certificate Viewer opens, it should say "this certificates has been verified for the following uses" and a list of uses.
Comment 1 Uwe Geuder 2011-07-09 12:01:31 PDT
Created attachment 545009 [details]
about:support

about:support info attached
Comment 2 Uwe Geuder 2011-07-13 10:27:09 PDT
Created attachment 545697 [details]
about:buildconfig

Aurora has been updated several times since I originally reported the bug. The problem persists. Someone on irc wanted to see "my" about:buildconfig (I guess this really is Aurora's configuration), so I attach it here.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-18 15:11:52 PDT
This is caused by bug 479393:

+ verifyResult =
+  CERT_VerifyCertificateNow(defaultcertdb, mCert, PR_TRUE,
 			    certificateUsageSSLClient |
 			    certificateUsageSSLServer |
 			    certificateUsageSSLServerWithStepUp |
 			    certificateUsageEmailSigner |
 			    certificateUsageEmailRecipient |
 			    certificateUsageObjectSigner |
 			    certificateUsageSSLCA |
 			    certificateUsageStatusResponder,
 			    NULL, &usages);
...

+ if (verifyResult != SECSuccess) {
+     int err = PR_GetError();
+     verifyFailed(_verified, err);
+     return NS_OK;
+ }

We are expecting that CERT_VerifyCertificateNow will return SECSuccess if at least one of the given usages is valid. However, it actually returns SECFailure unless all of the given usages are valid. I remember that Kai's original patch was correctly ignoring the return value of CERT_VerifyCertificateNow, but he "corrected" it when I told him we should check the return value. My bad. :(

The fix is simple: restore the error detection logic to the way it was done before:

- if (count == 0) {
-     verifyFailed(_verified, err);		
- } else {		
-     *_verified = nsNSSCertificate::VERIFIED_OK;		
- }

Also, we should add a comment about why we are not checking the return value.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-19 13:53:10 PDT
This affects Firefox 6, so we might want to consider fixing it there before shipping (I'm not exactly sure how critical this is).
Comment 5 Kaspar Brand 2011-07-20 00:03:28 PDT
(In reply to comment #4)
> (I'm not exactly sure how critical this is).

Fairly "critical", IMO, as it affects the display of *all* certs in cert viewer (Fx 6 will show "Could not verify this certificate for unknown reasons" even if a server cert is perfectly valid - try https://addons.mozilla.org e.g.).
Comment 6 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-20 07:33:05 PDT
This causes the certificate viewer to show a misleading message in the General tab, and prevents the general tab from displaying the usages for which the certificate is valid. The information is still available from the Details tab. No other certificate functionality is affected by the bug--in particular, it doesn't affect any other parts of the UI like the site identity block, it doesn't stop valid SSL connections from working, and it doesn't allow invalid SSL connections to work.
Comment 7 Kai Engert (:kaie) 2011-07-20 07:50:25 PDT
Should be a blocker.
Comment 8 Kai Engert (:kaie) 2011-07-20 08:08:28 PDT
Let's be clear which parts need to be reverted and attach a patch.
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-20 08:25:50 PDT
Created attachment 547103 [details] [diff] [review]
Fix usage display in cert viewer for non-libpkix case only

Here is the fix.

The libpkix case is still broken, but that doesn't matter because libpkix is not enabled yet and can only be enabled with an unsupported hidden pref. I filed 672811 for fixing the libpkix case.
Comment 10 Kai Engert (:kaie) 2011-07-20 08:28:40 PDT
Created attachment 547104 [details] [diff] [review]
Patch v1 - restore the old behaviour
Comment 11 Kai Engert (:kaie) 2011-07-20 08:31:10 PDT
Comment on attachment 547103 [details] [diff] [review]
Fix usage display in cert viewer for non-libpkix case only

I don't like your patch, because you call PR_GetError() very late. Please call it as early as we did in the past, immediately after the action - this avoids all risk that other activity might change the error code.
Comment 12 Kai Engert (:kaie) 2011-07-20 08:33:23 PDT
Also, you're calling PORT_GetError, while the old code used PR_GetError. Let's use exactly what we did in the past.
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-20 08:36:07 PDT
Comment on attachment 547104 [details] [diff] [review]
Patch v1 - restore the old behaviour

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

r+. Please add my comment and (void) in front of the call to CERT_VerifyCertificateNow, so we don't make this mistake again.
Comment 14 Kai Engert (:kaie) 2011-07-20 08:39:55 PDT
(In reply to comment #9)
> 
> The libpkix case is still broken, but that doesn't matter because libpkix is
> not enabled yet and can only be enabled with an unsupported hidden pref. I
> filed 672811 for fixing the libpkix case.

My patch works in the libpkix case, too.
Comment 15 Kai Engert (:kaie) 2011-07-20 08:50:02 PDT
(In reply to comment #14)
> 
> My patch works in the libpkix case, too.

I'm wrong.
Comment 16 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-20 09:00:35 PDT
Kai, lets worry about the libpkix case in bug 672811. I will comment there.
Comment 17 Kai Engert (:kaie) 2011-07-20 09:11:41 PDT
Created attachment 547113 [details] [diff] [review]
Patch v1 with comment added

carrying forward r=bsmith
Comment 18 Kai Engert (:kaie) 2011-07-20 09:23:24 PDT
http://hg.mozilla.org/mozilla-central/rev/c9cdc5df55f4
Comment 19 Kai Engert (:kaie) 2011-07-20 09:26:00 PDT
Comment on attachment 547113 [details] [diff] [review]
Patch v1 with comment added

Requesting approval for beta and aurora.
Firefox 6 would be the first release to show up this regression bug.
Comment 20 christian 2011-07-21 14:46:17 PDT
Comment on attachment 547113 [details] [diff] [review]
Patch v1 with comment added

Approved for releases/mozilla-aurora and releases/mozilla-beta. We'll also track this to make sure we don't ship with this regression
Comment 21 Simona B [:simonab ] -PTO- back Sept 5th 2011-07-27 04:34:34 PDT
Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110726 Firefox/8.0a1

Verified issue on mozilla central using the STR from Comment 0, on Windows XP, Windows 7, Ubuntu and Mac OS X 10.6.

The target milestone of this bug is set to mozilla 6. Will this land on Firefox beta 6 and Aurora also?
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-28 14:04:19 PDT
Yes, it will.
Comment 24 Uwe Geuder 2011-07-29 08:07:43 PDT
Thanks everybody, works again in the Aurora release I just received via auto update. Not sure about Mozilla's bugzilla process. Should I as the original reporter set the status to verified?
Comment 25 Michael Lefevre 2011-07-29 09:38:57 PDT
(In reply to comment #24)
> Thanks everybody, works again in the Aurora release I just received via auto
> update. Not sure about Mozilla's bugzilla process. Should I as the original
> reporter set the status to verified?

You could have done, or anyone else who could reproduce the problem and verify the fix.  I'll set it now, based on your comment.  Thanks for the report.

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