686135 - Extensions cannot find out when a certificate fails certificate chain validation

Status: RESOLVED WONTFIX

(Core :: Security: PSM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #686095 +++
+++ This bug was initially created as a clone of Bug #644640 +++

In bug 686095 comment 2, Peter Eckersley wrote:
> One example of why we're tearing our hair out over this issue is that the <a
> href="https://trac.torproject.org/projects/tor/wiki/doc/HTTPSEverywhere/
> SSLObservatorySubmission">Decentralized SSL Observatory</a> can't get copies
> of invalid TLS certificates that are sent to the user's browser unless the
> user clicks through the security dialog.  What that means is that our
> extension can't detect/study an attack like this one:

In PSM, when we reject a certificate, we notify the callbacks for the socket via the nsIBadCertListener2 interface. The callbacks are set by the code that creates the socket, and there is no way for an extension to hook into every one. For the reason Peter pointed out, among others, it seems like a good idea to augment this mechanism to allow extensions to take additional actions upon encountering a bad certificate.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This is very low-cost performance-wise and risk-wise. The code that handles error paths is in security/manager/ssl/src/nsNSSIOLayer.cpp; search for nsIBadCertListener2. This is a rare error path, so it wouldn't affect performance in the common case. We already have to run that code on the main thread, so syncing the socket/ssl thread with the main thread for this will not be an issue, which means most extensions should be able to be written in 100% JS. 

My initial thought is that we should provide a way for extensions to register their own nsIBadCertListener2 implementations that get called in addition to the bad cert handler obtained from the socket's callbacks.

I am eager to help outside contributors on implementing the infrastructure for this, including reviewing their patches and pushing this through security review.

Comment 2

[Ian Melven :imelven]

this might make a good Jetpack API ?

Comment 3

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

sec review triage = removing review flag

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This may be really easy to do. In SSLServerCertVerfication.cpp and/or nsNSSIOLayer.cpp (depending on the version of the code), we already check if the channel's notification callback object implements the nsIBadCertListener2 interface, and call its notifyCertProblem method.

We could additionally make nsIBadCertListener2 act like an XPCOM service, giving it a contract ID, so that we can do:

   nsCOMPtr<nsIBadCertListener2> globalListener = do_GetService("contract id");
   if (globalListener) {
     globalListener->notifyCertProblem(...);
   }

This would be somewhat limited because there could only be one global bad cert listener with this approach. Ideally, we'd like to support multiple such listeners. Perhaps that means we'd be better off using the observer service instead, but I'm not sure we can pass the necessary information to an observer if we were to do so.

Comment 5

[Honza Bambas (:mayhemer)]

Observer service is an easy option.  We can have a new interface that holds all the information we now pass through notifyCertProblem and pass an object of it in the subject.

Comment 6

[foudfou]

Hi all, not sure to understand the various issues, but, answering Peter Eckersley's bug 686095 comment 2, it is possible to get the cert without having the user click anywhere: for example by retrieving the bad cert from nsIRecentBadCertsService during the onSecurityChange of a TabsProgressListener attached to gBrowser, or by attaching a nsIBadCertListener2 to the request passed to onStateChange of the TabsProgressListener. See https://github.com/foudfou/skipCertError/blob/4147f586cb96c20781b0b7331417fab5d9a46af6/src/chrome/content/overlay.js for illustration.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I agree with foudfou that no new API is needed. In fact, I think Peter also found that nsIBadCertListener2 would be good enough for his addon, *if* we changed it so that it is notified of all certificate validation failures and not just overridable failures. So, I think this bug is just "report all certificate validation failures to the bad cert listener."

Comment 9

[Josh Matthews [:jdm]]

Brian, are you still willing to commit to mentoring this bug?

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Josh Matthews [:jdm] from comment #9)
> Brian, are you still willing to commit to mentoring this bug?

If it is just a matter of doing what I suggested in comment 7, then yes, I will have time to mentor for this.

Comment 11

[Zorro]

Hi Brian!

I am willing to work on this. I hope this could be done by a newbie. Please give me some pointers to proceed.

Comment 12

[Peter Eckersley]

It turns out that extensions can actually get this information.  See the branch merged on the 2nd of April here for an implementation:

https://github.com/EFForg/https-everywhere/commits/master/src/components/ssl-observatory.js

Comment 13

[Atul Kumar [:AtKr2]]

Hi Brian,
Are you still available for mentor ship? If yes, can we start work on this?

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

No, I'm not available to mentor stuff outside of mozilla::pkix right now.

Let's ask David who is free or whether this is still wanted.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Based on comment 12, this doesn't seem necessary anymore. Peter - is the use-case of https-everywhere/ssl-observatory being met?

Comment 16

[Peter Eckersley]

Our code that tried to glean this information from other APIs wound up being reverted as a workaround for a (possibly unrelated) crash bug:https://github.com/EFForg/https-everywhere/issues/262 .  So I'd say that a new API isn't required; there may or may not be issues with the existing ones; this bug can in any case be closed.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

WONTFIX as per comment 16.