Last Comment Bug 686150 - PSM attempts to validate client certificates when displaying the client cert picker
: PSM attempts to validate client certificates when displaying the client cert ...
Status: RESOLVED FIXED
[psm-arch][psm-auth][inbound]
: main-thread-io
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
:
Mentors:
Depends on:
Blocks: 674147
  Show dependency treegraph
 
Reported: 2011-09-10 12:41 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2011-10-18 05:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Do not verify client certificates when they are being used for SSL client auth (1.34 KB, patch)
2011-09-30 22:23 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
Details | Diff | Splinter Review
client cert that can be imported for testing (2.25 KB, application/octet-stream)
2011-10-06 17:33 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-10 12:41:23 PDT
nsNSS_SSLGetClientAuthData calls nsNSSCertificate::FormatUIStrings for each client cert that it is about to display. FormatUIStrings does certificate path validation via nsNSSCertificate::GetUsagesString.

This is wrong for two reasons:

1. It causes the main thread to block on certificate path validation (OCSP request I/O, in particular).

2. We should not be validating client certificates anyway, because we likely do not have the intermediate and/or root certificates needed to validate them, and/or those intermediate and/or root certificates won't be trusted.

I propose that we fix this by removing the verified usages display from the client cert picker UI.
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-30 22:23:44 PDT
Created attachment 563934 [details] [diff] [review]
Do not verify client certificates when they are being used for SSL client auth

This is a prerequisite for the SSL thread removal.

Watch https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=647a0a569156 for your results to come in.

Builds and logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bsmith@mozilla.com-647a0a569156.
Comment 2 Honza Bambas (:mayhemer) 2011-10-06 10:49:40 PDT
Comment on attachment 563934 [details] [diff] [review]
Do not verify client certificates when they are being used for SSL client auth

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

So, this will just remove the "Purposes:" line from the details?  I'm not aware of another way to get this info anywhere in the cert viewer.  Maybe we should add one (in a new bug, not a priority, until someone complains about it missing). 

CertInfoPurposes locale string can be removed with the code.
Comment 3 Honza Bambas (:mayhemer) 2011-10-06 10:51:23 PDT
And: please, do a good testing of client cert auth with and w/o the patch.  We don't have an automatic tests for this feature, my IIS is stupid to setup, so it is hard to say if all works or not.  Thanks.
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-06 17:33:53 PDT
Created attachment 565411 [details]
client cert that can be imported for testing

Thanks for the review.

You can import this client cert using Options -> Advanced -> Certificates -> Your certificates -> Import. The password is blank.

You can check the functionality via https://www.mikestoolbox.net/.

As you can see, there is no effect on the UI before/after the change, because the old UI omitted the usage info when it failed to validate the certificate. Client certificates will usually fail to validate because the root for client certificates is, in practice, usually not installed.

I also verified that the Microsoft IE UI does not include the usage information. For the most part, it is useless jargon for the user.
Comment 5 Kaspar Brand 2011-10-06 21:58:09 PDT
(In reply to Honza Bambas (:mayhemer) from comment #2)
> I'm not
> aware of another way to get this info anywhere in the cert viewer.  Maybe we
> should add one (in a new bug, not a priority, until someone complains about
> it missing). 

Originally, there was a "Purposes" column in most of the cert manager tabs, see bug 383969 comment 7.

Bug 384611 is about restoring that feature.
Comment 6 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-17 17:31:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/777a8404a63e
Comment 7 Marco Bonardo [::mak] 2011-10-18 05:42:35 PDT
https://hg.mozilla.org/mozilla-central/rev/777a8404a63e

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