Closed Bug 1020431 Opened 10 years ago Closed 10 years ago

nsRecentBadCerts can fetch not the latest cert for a host:port pair

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 1 obsolete file)

GetBadCert searches for recent certs using a linear search and uses the last entry with the hostname:port pair. However AddBadCert adds another entry on the last place of the current circular buffer, without looking that an entry may have been there. 

So If host A uses cert X and then migrates to use cert Y the cache entries can be:
XXXXX
(change cert, next location for entry is 0
YXXXX
and getBadCert will return cert X instead of Y.
Attached patch logging-on (obsolete) — Splinter Review
I don't think we should bother with enhancements to nsRecentBadCerts. Instead, we should just remove it from Gecko and let Thunderbird take it over if it still needs it. nsRecentBadCerts inherently causes race conditions in any code that tries to use it, so it isn't really acceptable for any use beyond the Thunderbird user interface issue that it was originally intended to solve. And, as we've seen in other bugs, trying to support/use it actively causes problems.
My goal for this bug is to solve Bug 659736, where two issues get compounded to get a bad experience
Blocks: 659736
cleanup
Attachment #8434265 - Attachment is obsolete: true
Assignee: nobody → cviecco
Comment on attachment 8434291 [details] [diff] [review]
logging-on (v1.1)

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

This is to solve an old bug. Tests will be tricky as there are no hooks to create an SSLStatus directly from JS or to read one from file. Probably the only solution would be to write a compiled in test if we think tests are required for this.
Attachment #8434291 - Flags: review?(dkeeler)
Comment on attachment 8434291 [details] [diff] [review]
logging-on (v1.1)

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

Let's remove nsIRecentBadCertsService.
Attachment #8434291 - Flags: review?(dkeeler) → review-
I do think we need the getRecenBadCert (in the near term) as we will need to allow certificate overrides for B2G email(https://bugzil.la/874346) and the fallback mechanism uses XHR and XHR has some blacklisted ports including secure imap (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#123).

I propose to fix this and file up two follow up bugs:
1. Make the fallback work on arbitrary ports (for that single connection)
2. remove getRecenBadCert
(In reply to Camilo Viecco (:cviecco) from comment #7)
> I do think we need the getRecenBadCert (in the near term) as we will need to
> allow certificate overrides for B2G email(https://bugzil.la/874346) and the
> fallback mechanism uses XHR and XHR has some blacklisted ports including
> secure imap
> (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.
> cpp#123).

nsRecentBadCert* is part of the implementation of the "Add Exception" button to the certificate manager's Server tab. We unfortunately reused the same dialog box for that button that we use for the other path to getting to the cert error dialog box, causing ourselves undue trouble. What may have been appropriate for the "Add Exception" UI element isn't appropriate for other needs.

Note that the B2G email app needs a *web* (DOM) API, not an XPCOM API, and the implementation of the associated web APIs is almost definitely going to be specified in a way that requires us to return exactly the cert (chain) used on a specific connection (XHR request, websocket connection, TCP connection, etc.), not just the most recent (modulo races) bad cert encountered for a given hostname. So, nsRecentBadCert* isn't going to help with the B2G mechanism, AFAICT.
Comment on attachment 8434291 [details] [diff] [review]
logging-on (v1.1)

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

asking for rereview after in person meeting
Attachment #8434291 - Flags: review- → review?(dkeeler)
Comment on attachment 8434291 [details] [diff] [review]
logging-on (v1.1)

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

I had some comments on the contents on the patch before I realized my overall concerns about making these changes were more significant than I thought.
Here are my concerns:
- We've agreed this functionality is fundamentally flawed and needs to be replaced by something that actually works, so we shouldn't be spending any more time fixing it.
- This bug only affects a vocal minority. Our resources are limited, so we need to focus on doing the most good for the majority of our users.
- Changes carry risk. We have no tests to mitigate this risk.

This is a long-standing bug. I think it would be worthwhile to spend a bit more time on a more complete fix that also advances our goals in other areas of the code rather than including this now and then replacing/removing it all again later.

::: security/manager/ssl/src/nsRecentBadCerts.cpp
@@ +24,5 @@
>  
>  using namespace mozilla;
>  
> +#ifdef PR_LOGGING
> +extern PRLogModuleInfo* gPIPNSSLog;

Let's not add logging to this implementation. If we're not going to be putting more engineering effort into it, we'll never need the logging.

@@ +51,2 @@
>    NS_ENSURE_ARG_POINTER(aStatus);
> +  if (!aHostNameWithPort.Length()) {

Let's not touch anything else in this file but AddBadCert.

@@ +142,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    {
>      ReentrantMonitorAutoEnter lock(monitor);
> +    size_t entryLocation = mNextStorePosition;

This implementation needs to be commented - why are we doing this?
Attachment #8434291 - Flags: review?(dkeeler) → review-
(In reply to Camilo Viecco (:cviecco) from comment #3)
> My goal for this bug is to solve Bug 659736, where two issues get compounded
> to get a bad experience

While this issue was mentioned there, this will not solve that bug, because the inability to add a security exception where the dialog controls are disabled is unrelated to getRecentBadCert AFAICT.

The only symptom from non-deterministic getRecentBadCert at that dialog is that an incorrect exception will be added (existing instead of a new one) and later it may ask again to add a security exception for the same page. This can usually be workedaround by restarting the browser or by reloading without cache (CTRL-F5).

Nevertheless, it's worth fixing, or just removing getRecentBadCert from the "add security exception" dialog, at least for Firefox.
Bug 940506 removed nsIRecentBadCerts:
https://hg.mozilla.org/mozilla-central/rev/ffc53bd04d49

So I guess this is now "invalid", or whatever status is most appropriate.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Depends on: 940506
Blocks: 1092369
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: