1243901 - Replace Certificate Authority without user notification, wrong CA displayed

Status: RESOLVED DUPLICATE of bug 1230321

(Firefox :: Address Bar, defect)

Description

[Frank Ganske]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Iceweasel/38.5.0
Build ID: 20151216080806

Steps to reproduce:

I've built Mo Cuishle - a caching proxy for offline use - http://ganskef.github.io/MoCuishle. For caching purposes it creates a CA certificate and imports it into Firefox by an add-on. This cert is used to generate server certificates on the fly - Man-In-The-Middle. It provides a toolbar action to enable/disable the caching proxy. 

http://ganskef.github.io/MoCuishle/mocuishle-binary-preview/mocuishle-1.0-20160125.xpi


Actual results:

With nsIX509CertDB2 it's possible to list existing root certificates to pick one by name. This is bad since an attacker can choose one to replace it. 

With nsIX509CertDB2 it's possible to replace it and set permissions for websites, mail and developers with no user notification, in no time, by an add-on, multiple times. This is bad, since an attacker can hide his activities.

The point is: On switching proxy/no proxy the https icon isn't invalidated. Example:

Open https://www.google.de, 
 - switch to MITM, refresh, icon shows "Verified by: Google Inc" -> bad, 
 - click on the symbol, dialog shows "Verified by: Google Inc" -> bad, 
 - click More Informations..., settings pane shows Mo Cuishle -> okay, 
 - it's refreshed by switching between tabs -> okay,
 - it's not happening on sites with green Owner Informations like https://bugzilla.mozilla.org -> okay

An attacker can enable and disable MITM by an add-on with no trace, in milliseconds. That's bad. 


Expected results:

Importing a root certificate have to require a user action like it's modeled in nsIX509CertDB -> nsIX509CertDB2 is invalid, I think. 

Requesting a certificate to delete it for example have to require an ID like it's modeled in nsIX509CertDB -> nsIX509CertDB2 is invalid, I think since an attacker can gather the installed IDs. 

The security info of https sites has to be invalidated on every page load, do display the actual informations. 

It's untested with mobile browser since there is no toolbar action available.

Comment 1

[:Gijs (away/busy until Mar 10)]

(In reply to Frank Ganske from comment #0)
> Open https://www.google.de, 
>  - switch to MITM, refresh, icon shows "Verified by: Google Inc" -> bad, 
>  - click on the symbol, dialog shows "Verified by: Google Inc" -> bad, 
>  - click More Informations..., settings pane shows Mo Cuishle -> okay, 
>  - it's refreshed by switching between tabs -> okay,
>  - it's not happening on sites with green Owner Informations like
> https://bugzilla.mozilla.org -> okay
> 
> An attacker can enable and disable MITM by an add-on with no trace, in
> milliseconds. That's bad. 

Right, OK. This is essentially the fact that if you refresh a website without changing the location, we don't update the certificate information in the location bar. That's a bug, and I think we should fix that.


> Expected results:
> 
> Importing a root certificate have to require a user action like it's modeled
> in nsIX509CertDB -> nsIX509CertDB2 is invalid, I think. 
> 
> Requesting a certificate to delete it for example have to require an ID like
> it's modeled in nsIX509CertDB -> nsIX509CertDB2 is invalid, I think since an
> attacker can gather the installed IDs. 

I'm not entirely sure if I understand what you're trying to say here, especially the "is invalid, I think". Are you basically saying that you believe:
- importing a root certificate into the certificate store by an add-on should require visual user confirmation
- requiring an ID for certificate deletion is not helpful security-wise because the add-on can gather the installed IDs ? [I expect that this isn't intended as a security measure, but I don't actually know almost anything about the TLS infra in our code]

Flagging you for needinfo to confirm that this is what you're saying here. :-)

Tim, can you pick up the location bar UI part of this and/or recommend who can comment on the add-on cert install portion of it?

Comment 2

[Frank Ganske]

Thank you very much. I agree the first paragraph "- importing".

I'll try to clarify the second point "- requiring". My code:

    /* Requires new nsIX509CertDB2 */
    let certs = certDB.getCerts();
    let en = certs.getEnumerator();
    while (en.hasMoreElements()) {
      let each = en.getNext();
      each.QueryInterface(Ci.nsIX509Cert);
      if (each.nickname === "Proxy for offline use - Mo Cuishle") {
        /* Requires old nsIX509CertDB */
        let db = x509certdb.getService(Ci.nsIX509CertDB);
        db.deleteCertificor exampleate(each);
        console.info("Certificate Authority " + each.nickname + " deleted");

I think it's bad to have a collection of all installed certificates getCerts(). The other (old/regular?) interface nsIX509CertDB isn't exposing this. Here you have to require a certificate explicit. 

Please consider do double check where nsIX509CertDB2 is needed. What's the reason for this? Why these methods aren't in nsIX509CertDB? Is it a secret interface?

Comment 3

[Dana Keeler (she/her) [:keeler]]

The issue with not updating the certificate information when the server certificate changes is the same as bug 1230321.
As for nsIX509CertDB2, it was folded into nsIX509CertDB and no longer exists, so that might be part of the confusion here. With regard to the functionality the interface provides, consider that addons are very powerful in general. An addon could replace a user's certificate database entirely even without nsIX509CertDB just by using file system APIs. Thus the existence of this API is not of particular concern.

Comment 4

[Frank Ganske]

Thank you. Is there anything to do for me with this bug, or simply leave?

Comment 5

[:Gijs (away/busy until Mar 10)]

(In reply to Frank Ganske from comment #4)
> Thank you. Is there anything to do for me with this bug, or simply leave?

It's probably not useful to keep this security-sensitive considering the other bug is public, so I'm unhiding this.