1024871 - stop offering to import CA certificates when browsed to

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[JacobHenner]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140514164129

Steps to reproduce:

When visiting a site that serves certificate authority certificates over HTTP, a user can import the CA directly into Firefox's trust database.


Actual results:

A simple menu asks the user whether the certificate should authenticate web sites, software developers, or email contacts. After selecting a combination of these options, the user can store the certificate in the trust database.


Expected results:

A warning should be displayed before the menu appears, informing the user that importing a CA certificate over plaintext HTTP can compromise his or her security, as an adversary performing a man-in-the-middle could send a compromised certificate to the client; taking advantage of the lack of authentication in HTTP. This warning should be able to be ignored in a similar manner to other TLS errors (confirming exceptions, etc).

Comment 1

[:Gijs (he/him)]

Dan, do you know where something like this would live and/or if we have plans already? And I would assume this is sec-want, but I could be wrong... :-)

Comment 2

[Daniel Veditz [:dveditz]]

--> Security UI/PSM

Wow, importing a root cert over HTTP is really stupid. In fact, adding a root at all is pretty inadvisable but I guess there are some corporate use cases we can't really prevent. I wouldn't stop at a warning people are going to ignore, if we do anything we should just refuse to import. The only acceptable schemes should be https: and file:

For corporations who are caught out by such a change the workaround would be to download the file (or mail it) and then install it from the local file location. Would not be any more secure doing that, but it's harder enough that hopefully admins will be prodded to provide SSL.

There are two URLs involved; do they both need to be served securely? Clearly we don't want the root itself loaded insecurely because a different root could be substituted by a MITM (and a MITM is in the best position to take advantage of the cert). But what about an insecure (http:) page with a link to a securely-loaded (https:) root? It's just as easy for our hypothetical attacker to change the link as it is to substitute the root. We should go ahead and block that case too (pretty sure we can get ahold of the internal/real referrer on the channel used to load the root).

Comment 3

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

I don't think we should offer to import a root just by browsing to it (over any scheme). If people want to add a root, they can download it and then use the certificate manager ui to import it.

Comment 4

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

Attachment: patch

This patch stops firefox from offering to import certificates when browsed to.

Comment 5

[Camilo Viecco (:cviecco)]

Why remove the importing of user certificates?

Comment 6

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

Comment on attachment 8446157 [details] [diff] [review]
patch

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

I am fine with it. When you remove user-facing features from Firefox you should ask a Firefox person. I suggest dolske in this case. Also, please update the bug summary.

Comment 7

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

(In reply to Camilo Viecco (:cviecco) from comment #5)
> Why remove the importing of user certificates?

Here's my reasoning: very few people use user certificates, and they can still import them using the certificate manager. Removing importing of all types of certificates this way means we can remove PSMContentDownloader entirely. This is good because it removes an attack surface and it's one less thing for us to maintain.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 8446157 [details] [diff] [review]
patch

I don't really have much feedback -- in general I'm happy to blanket r+ anything that removes crufty old PSM UI. :)

The only question that comes to mind is if we expect such UI to be needed by some annoyingly-just-big-enough usecase that removing it causes too much pain. Doesn't sound like that's the case here (given an alternate method of performing the same function), but if we were concerned we could add telemetry to measure and/or a pref to softly disable as a first step.

Comment 9

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

Thanks for the reviews!
The only concern I know of at the moment is some mozmill tests import a test CA this way. I'll coordinate with them to use a different method.

Comment 10

[Kaspar]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #7)
> (In reply to Camilo Viecco (:cviecco) from comment #5)
> > Why remove the importing of user certificates?
> 
> Here's my reasoning: very few people use user certificates, and they can
> still import them using the certificate manager.

I would like to echo Camilo's concerns: dropping support for X509_USER_CERT from PSMContentDownloader would amount to a serious loss of functionality: nsNSSCertificateDB::ImportUserCertificate is also needed for processing the reply for an SPKAC ("<keygen>") request - i.e. when a private key has been created by the browser, but no certificate is in the DB yet, cf.

https://wiki.mozilla.org/CA:Certificate_Download_Specification#Importing_Certificates_into_Firefox

Removing support for the "application/x-x509-user-cert" MIME type would completely break browser-based enrollments like https://secure.comodo.com/products/frontpage?area=SecureEmailCertificate. Also, note that the only way to reach nsNSSCertificateDB::ImportUserCertificate right now is through PSMContentDownloader, it can't be done with a file-based import.

Comment 11

[Kai Engert (:KaiE:)]

The feature you'd be breaking is browser based certificate enrollment.

Comment 12

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

Comment on attachment 8446157 [details] [diff] [review]
patch

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

OK, I will take back my r+. I understand and fully support the efforts to remove the non-standard, non-used, and/or bad parts of PSM but I think we should keep the <keygen>-related stuff working. If you redo the patch to limit it to removing the special support for CA certificates, I will r+ that.

Comment 13

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

Attachment: patch 2/2: import user certificates via certificate manager

This patch enables user certificates obtained through processes similar to what Kaspar linked to in comment 10 to be imported through the "Your Certificates" tab of the certificate manager.

Comment 14

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

Comment on attachment 8447378 [details] [diff] [review]
patch 2/2: import user certificates via certificate manager

This patch is orthogonal to this bug. Please file it as a new bug.

Also, I have almost never reviewed changes to the UI code other than deletions and I don't think this is a particularly good time for me to start. Let's have somebody else do the review of this patch.

Regarding the current bug: Isn't the convenient way of importing user certificates via PSMContentDownloader somewhat of a facilitator of the more important change of removing the legacy window.crypto.* stuff by making <kegen> more tolerable of an alternative for the people to be inconvenienced by the pending removal of that functionality? I feel like removing the user certificate import now may be counterproductive to that more-urgent/important change.

Comment 15

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

Filed bug 1031543.
I don't think the extra steps of save/import will make <keygen> significantly less usable.

Comment 16

[Kaspar]

The patch in comment 13 (now attached to bug 1031543) seems like a useful enhancement, but I do not think that comment 15 is true: currently, the user simply has to open the link in Firefox, and the keygen "reply" is automatically processed. If PSMContentDownloader is completely removed, then the file will end up in the "Downloads" folder, and it's really cumbersome to get to the "Import..." button of the "Your Certificates" tab in Certificate Manager from there - it's buried deep down in one of the "Options" panels (or "Preferences" if you happen to be on OS X), so in terms of user support, considerably more documentation would have to be provided (all of this with references to language-dependent UI elements, of course), the "Import..." button most likely won't open with the "Downloads" folder by default etc. Compared to other browsers, this would make browser-based enrollment in Firefox a real pain, IMO.

Comment 17

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

Comment on attachment 8446157 [details] [diff] [review]
patch

Since bug 1031543 landed (thus enabling user certs to be imported via the certificate manager), I'm asking for a re-review of these changes. Thanks.

Comment 18

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

Comment on attachment 8446157 [details] [diff] [review]
patch

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

David, could you expand on your thinking about why you think this won't hurt usability of <kegen>-based web applications? AFAICT, it is a significant usability regression for the admittedly-small number of people that use <keygen>. My concern is that <keygen> (or the obsolete window.crypto stuff we're removing) seems to be being used by StartSSL as part of them giving away free certificates. I think it would be bad for us to significantly reduce the usability of getting free certificates just because we want to delete a little bit of code. (window.crypto legacy junk is another story, because it is all exposed to the DOM.) I don't see what problem the patch is solving beyond "delete some code," except that I do see and mostly agree with the idea that we shouldn't make it so easy to import *root* certificates. But, maybe I am missing something?

Comment 19

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

I agree that the process of use <keygen> -> receive signed certificate from server -> download -> import via certificate manager is less streamlined than <keygen> -> receive signed certificate from server -> import dialog automatically pops up. However, in addition to being a small number of people, those who need this functionality are among the most technical users of Firefox. They can handle the additional steps. The reason that this is more significant than simply deleting code is that this is an unnecessary implementation in a memory-unsafe language (and note that like window.crypto, this is actually exposed to web content - any page the browser visits can cause this code to run). If someone finds the new workflow untenable, they can write a safe addon in JS to match the old functionality.

Comment 20

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

Brian, to look at this another way, say we didn't have this feature and someone filed a bug saying, "I want to change Firefox so that whenever a website sends a response with a certain mime type, instead of offering to download that file or open it in another application like usual, Firefox will instead open a modal dialog offering to add it to your certificate database. And even though this could be implemented in JS, I'm going to do it in C++."
Personally, I would respond, "The vast majority of our users don't need this and it can be implemented in JS, so this feature would be more appropriate as an addon."
What do you think?

Comment 21

[Kaspar]

Attachment: Remove support for application/x-x509-ca-cert from PSMContentListener/PSMContentDownloader

Maybe it would be good to recall what this bug was originally about: it was filed with the description "Firefox should display warning message when importing CA certificate over plaintext HTTP", and until comment 3, everyone agreed that the concern is with importing root certs (only) via this route.

Throwing out PSMContentDownloader completely and deliberately breaking a feature which has been supported for 13+ years (<keygen>-based enrollment, bug 77983), while pointing out that it can be reimplemented in an addon feels like a really strange argument. (If the primary concern is those about 100 lines in PSMContentDownloader with buffer juggling in OnStartRequest/OnDataAvailable/OnStopRequest, then I'm absolutely fine if it is reimplemented in JS, but it needs to be part of Firefox, not put into an addon.)

I'm attaching a patch which does what was suggested in comment 12 (disable direct import of root or intermediate CA certificates).

Comment 22

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

Comment on attachment 8459101 [details] [diff] [review]
Remove support for application/x-x509-ca-cert from PSMContentListener/PSMContentDownloader

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

r=me with the suggested changes to nsIX509CertDB.importCertificates. Thanks for writing the patch and thanks for your constructive comments in this discussion.

::: security/manager/ssl/src/PSMContentListener.cpp
@@ -172,5 @@
>    }
>  
>    switch (mType) {
> -  case PSMContentDownloader::X509_CA_CERT:
> -    return certdb->ImportCertificates((uint8_t*)mByteData, mBufferOffset, mType, ctx); 

I think it is no longer necessary to have importCertificates in the nsIX509CertDB interface, so let's remove it. Note that ImportCertificates is used internally by nsNSSCertificateDB::ImportCertFromFile so when you remove it from nsIX509CertDB.idl, you will need to add the declaration in the private section of nsNSSCertificateDB. Also update the comment above the implementation of nsNSSCertificateDB::ImportCertificates to remove the (already-wrong) reference to the IDL declaration. Don't forget to update the UUID of the interface.

Comment 23

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

Kaspar, would you have time to update your patch this weekend? If so, I can land this patch for you for Firefox 34, which I'd really like to do.

David,

Kaspar already said, basically, what I was intending to say. We should revert this bug back to its original scope and fix the bug as originally scoped.

Also, please see Ryan Sleevi's comments in the similar Chromium bug[1]: "The concern with this UI/behaviour comes from the fact that <keygen> and application/x-x509-user-cert are two different components. The former is part of HTML5, and the latter a de facto standard (along with the rest of application/x-x509-*), supported by Opera, Firefox, and many mobile browsers, but not formally specified." and "My own position on this is that the same UI experience that Firefox takes with regards to <keygen> and application/x-x509-user-cert is probably appropriate here."

I do think that this is an area where we could use some security review and probably some improvement. It seems like, if we identify aspects of the feature that we feel need improvement for security, that there may be contributors that can step up and help implement those improvements. Especially if/when we remove the analogous window.crypto API, the people that are interested in certificate enrollment are likely to be motivated to work on these issues. So, let's identify (in another bug) the concrete security issues with the current implementation so that we can measure contributors' interest.

If you still think complete removal of PSMContentDownloader is warranted, let's do that in a separate bug. But, for Firefox 34, let's at least get rid of the most dangerous part of the feature.

[1] https://code.google.com/p/chromium/issues/detail?id=65541

Comment 24

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

David,

Sorry to reply multiple times, but I want to make sure you feel like I've at least considered your concerns and that I've attempted to address them:

(In reply to David Keeler (:keeler) [use needinfo?] from comment #20)
> Brian, to look at this another way, say we didn't have this feature and
> someone filed a bug saying, "I want to change Firefox so that whenever a
> website sends a response with a certain mime type, instead of offering to
> download that file or open it in another application like usual, Firefox
> will instead open a modal dialog offering to add it to your certificate
> database."

If they noted that other browsers have this functionality, and they've written the code, I would consider accepting the patch.

> And even though this could be implemented in JS, I'm going to do
> it in C++."

I would probably ask them to write the functionality in JS, if it were a new patch.

> Personally, I would respond, "The vast majority of our users don't need this
> and it can be implemented in JS, so this feature would be more appropriate
> as an addon." What do you think?

Again, because there is quite a bit of agreement amongst browsers on the feature, I would say that this is a feature-parity issue and I would consider accepting the functionality. That is a key difference between this functionality and the window.crypto stuff that I support the removal of; AFAIK, no browser will ever implement that window.crypto stuff and so it's 100% proprietary to us.

Also, the amount of code in the window.crypto stuff makes it difficult to even start trying to secure, whereas here the task seems much more manageable.

Consider how your proposal would work on a mobile phone. How would the user browse their filesystem on a mobile phone in the first place, when generally phones can't browse the fileystem.

Really this is how <keygen> is supposed to work, AFAICT. I understand you are concerned that this is inconsistent with the way that we download other things, but there really isn't any inconsistency: assuming the user wanted the certificate downloaded at all, they almost definitely want it to be installed in the way we are doing now. Compare this with how we use pdf.js to render PDFs because we assume that users want to see the PDF and not download it, for example.

Comment 25

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

Ok - I understand your reasoning. Thank you for taking the time to explain your position.

Comment 27

[David E. Ross]

Can someone explain why a bug report over 11 years old (bug #267515) is closed as a duplicate in favor of this two-year-old bug report?  Should not this be the other way around?  Or is someone trying to hide the fact that a security bug remains open for over a decade?

Comment 28

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

Hi David,

This bug will fix bug 267515 by removing the feature entirely, which is why I marked that one as a duplicate of this one. Sometimes it's beneficial to mark an older bug as a duplicate of a newer one for various reasons (such as in this case or if the newer one has more information or is more clear).

Comment 29

[Djfe]

Hi,

I don't want to sound rude, but what is the current state of this bug? (it's been assigned after all)
It pretty much seems to be blocking bug: 1241447
So it would be nice if someone could add it to the list in-case he/she thinks so too

tl;dr of that bug:
Right now you need other browsers than Firefox on Android if you want to download and install certificates for enterprise wifi like https://doc.itc.rwth-aachen.de/display/CERT/Installation+der+Wurzelzertifikate , because Firefox tries to import them into its own certificate store and doesn't offer a way to download them for an import into the system's store instead

Comment 30

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

This bug is basically blocked on the issue that there is no certificate management UI in Firefox for Android. Without this feature or some sort of replacement, there is no way to import a new CA on that platform.

Comment 31

[Djfe]

but it's not what I and many others need
we need to be able to atleast download (as an alternative) .der certificates (just like .cer certificates for which this works already) 
in-case this bug report really isn't about that, just tell me so
thx :)

Comment 34

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

Attachment: bug 1024871 - stop importing certificates when navigated to r?kjacobs,jld

Before this patch, PSM would register content type handlers to handle certain
certificate types. This was an easy way to install a client certificate after
generating a key with <keygen>, but keygen has been removed. This was also an
easy way to install root certificates, but that's actually a considerable
security risk. We kept this functionality for so long because it was the only
way to add a 3rd-party root certificate to Fennec's certificate store. Now that
Fennec is EOL, we can remove it. (Fenix will need a way to trust 3rd party root
certificates, but the path forward there is to implement the enterprise roots
feature for Android.)

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bc24808649e
stop importing certificates when navigated to r=kjacobs,jld

Comment 36

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0bc24808649e