remove nsIRecentBadCerts and implementation

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

({addon-compat})

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

nsRecentBadCerts does some unnecessary things. These include breaking down and re-building an nsISSLStatus/CERTCertificate/etc. on every store/load (instead of sharing the memory) and using a lock (it's only accessed on the main thread). Also, there are a number of style nits that can be updated.
Posted patch patch (obsolete) — Splinter Review
r? to Camilo.
Also, Brian - I'd appreciate you taking a look at this to see if there are any pitfalls with doing it this way.
Attachment #8334656 - Flags: review?(cviecco)
Attachment #8334656 - Flags: feedback?(brian)
Keeler I dont think you can make this non-thread safe. Take a look at http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#287 this funcion needs NOT to run in the main thread and add a new cert. And now you require this to run on the main thread. 

ie. What happens with this patch an a cert error?
I think there's some confusion, here. CheckCertOverrides must run on the main thread:

286 SSLServerCertVerificationResult *
287 CertErrorRunnable::CheckCertOverrides()
288 {
289   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p][%p] top of CheckCertOverrides\n",
290                                     mFdForLogging, this));
291 
292   if (!NS_IsMainThread()) {
293     NS_ERROR("CertErrorRunnable::CheckCertOverrides called off main thread");
294     return new SSLServerCertVerificationResult(mInfoObject,
295                                                mDefaultErrorCodeToReport);
296   }
Comment on attachment 8334656 [details] [diff] [review]
patch

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

My las comment was incorrect. Thanks keeler.

::: security/manager/ssl/src/nsRecentBadCerts.h
@@ +27,5 @@
>  
>    void Clear()
>    {
>      mHostWithPort.Truncate();
> +    mSSLStatus = nullptr;

I assume nsCOMPtr do the right thing here.
Attachment #8334656 - Flags: review?(cviecco) → review+
Does this solve a real problem, or is this just cleanup?

nsRecentBadCerts is only useful for Thunderbird. If you want to clean things up, just remove it along with the corresponding button in the certificate manager. Thunderbird can add it back.
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #5)
> Does this solve a real problem, or is this just cleanup?
> 
> nsRecentBadCerts is only useful for Thunderbird. If you want to clean things
> up, just remove it along with the corresponding button in the certificate
> manager. Thunderbird can add it back.

This is not particular to Thunderbird - it's just a cache for the certificate error dialog. I suppose neither Firefox nor Thunderbird need it as long as we're okay re-fetching the certificate whenever a user wants to add an override for a bad certificate they've found. It would save memory. The one thing is we're not guaranteed to get the same certificate again. This particularly impacts certificate error reporting.
(In reply to David Keeler (:keeler) from comment #6)
> This is not particular to Thunderbird - it's just a cache for the
> certificate error dialog. I suppose neither Firefox nor Thunderbird need it
> as long as we're okay re-fetching the certificate whenever a user wants to
> add an override for a bad certificate they've found. It would save memory.
> The one thing is we're not guaranteed to get the same certificate again.
> This particularly impacts certificate error reporting.

IIRC, It was originally added in Thunderbird because there was no UI for adding cert overrides to Thunderbird and you had to go into the cert manager to do it. I think that subsequently changed in Thunderbird.

You are right that it is also used in the cert error override dialog box. But, even with the cache, you still have TOCTOU-type issues. It is possible for a given hostname:port to use different certs. For example, for a long time (if not still), citibank.com:443 would use different certs for every different IP address the hostname resolved to.

Ideally, we'd remove nsIRecentBadCerts and change the cert error override dialog box to take the bad cert as an input, so that it didn't need to look it up or fetch it itself. However, I looked into doing it before and it wasn't trivial. It probably is not hard either, but I never got motivated to do it. Anyway, I guess the behavior of using nsIRecentBadCerts is better than not using it, until we change the cert error override UI to work the better way.
Comment on attachment 8334656 [details] [diff] [review]
patch

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

The old code keeps the certificate in memory as a DER-encoded blob of bytes, not as an nsNSSCertificate object. This means that it won't keep a reference to a CERTCertificate object. This means that, with the old code, the certs in the recent bad certs list will not cause any effect in path building.

The new code keeps the certificate in memory with a reference to the CERTCertificate. That means the certs in the recent bad cert list potentially will help or hinder subsequent path building.

So, I think that this patch is moving in the wrong direction in terms of it allowing the bad certs to interfere more with path building than they currently do.

Also, we're changing code that doesn't have any tests, that doesn't need to be changed. I agree that it isn't the prettiest code in the world, but I'd rather work on making the code unnecessary so we can delete it, rather than prettying it up. So, I suggest we resolve this bug WONTFIX or INVALID.
Attachment #8334656 - Flags: feedback?(brian) → review-
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #8)
> The old code keeps the certificate in memory as a DER-encoded blob of bytes,
> not as an nsNSSCertificate object. This means that it won't keep a reference
> to a CERTCertificate object. This means that, with the old code, the certs
> in the recent bad certs list will not cause any effect in path building.

Got it.

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #7)
> But, even with the cache, you still have TOCTOU-type issues. It is possible
> for a given hostname:port to use different certs. For example, for a long
> time (if not still), citibank.com:443 would use different certs for every
> different IP address the hostname resolved to.
>
> Ideally, we'd remove nsIRecentBadCerts and change the cert error override
> dialog box to take the bad cert as an input, so that it didn't need to look
> it up or fetch it itself. However, I looked into doing it before and it
> wasn't trivial. It probably is not hard either, but I never got motivated to
> do it. Anyway, I guess the behavior of using nsIRecentBadCerts is better
> than not using it, until we change the cert error override UI to work the
> better way.

Right - I had a look at doing that too, and it looked like it would involve non-trivial changes to the docshell, which I want to avoid. And anyway, even if we could pass the certificate directly to the override page, it still wouldn't work properly, because as soon as you connected to the same host with a different IP address, there would be a different certificate.

So, maybe removing the cache and letting the override page do the lookup itself isn't a bad way to go?

The reason I want to improve or remove this code is because I'll need to change it anyway to support keeping track of the entire certificate chain, which we're going to need for the certificate error reporting project (as an added bonus, I'll need to write tests for that, which means that whatever implementation ends up sticking around will finally also get some tests).
We're now in a position to be able to remove nsIRecentBadCertsService entirely, so I'm morphing this bug.
Status: NEW → ASSIGNED
Depends on: 1025332
Summary: update nsRecentBadCerts.{cpp,h} for style and to remove unnecessary work → remove nsIRecentBadCertsService and implementation
Attachment #8334656 - Attachment is obsolete: true
Of course, the file is called nsIRecentBadCertsService.idl while the interface is called nsIRecentBadCerts.
Summary: remove nsIRecentBadCertsService and implementation → remove nsIRecentBadCerts and implementation
Comment on attachment 8449606 [details] [diff] [review]
patch

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

r+ assuming that you answer my question affirmatively.

::: security/manager/ssl/src/SharedSSLState.cpp
@@ -54,5 @@
> -    bool certDBExists = sCertDBExists.exchange(false);
> -    if (certDBExists) {
> -      sCertDBExists = true;
> -      nsCOMPtr<nsIX509CertDB> certdb = do_GetService(NS_X509CERTDB_CONTRACTID);
> -      if (certdb) {

It's a no-brainer to remove the nsIRecentBadCert stuff, but is it OK to avoid initializing the cert DB?
Attachment #8449606 - Flags: review?(brian) → review+
Yes. If my understanding is correct, the cert DB notifies SharedSSLState that since it has been initialized, there may be entries in the recent bad cert list, and if the user wants to clear private data, those entries may need to be cleared. Since they won't exist any more, this whole thing doesn't need to happen now.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #14)
> Yes. If my understanding is correct, the cert DB notifies SharedSSLState
> that since it has been initialized, there may be entries in the recent bad
> cert list, and if the user wants to clear private data, those entries may
> need to be cleared. Since they won't exist any more, this whole thing
> doesn't need to happen now.

Sounds reasonable to me.
Sorry for breaking the build. I've been told maybe a clobber is in order. Here's a try run:
https://tbpl.mozilla.org/?tree=Try&rev=31ee012cdac5
Flags: needinfo?(dkeeler)
https://hg.mozilla.org/mozilla-central/rev/ffc53bd04d49
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1057029
Blocks: 1059474
Blocks: 1059476
Blocks: 1020431
You need to log in before you can comment on or make changes to this bug.