825583 - Edit Trust in Certificate Manager - Server sets dangerous wrong values

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Hayo]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 2012111600

Steps to reproduce:

- Opened "Zertifikat-Manager" ('Certificate Manager'), tab 'Server'. 
- Selected entry 'The USERTRUST Network' -> 'www.google.com'.
- 'Ansehen ('View') the certificate: "Dieses Zertifikat konnte aus unbekannten Gründen nicht verifiziert werden"  (Certificate could not be verified for unknown reasons).
- 'Vertrauen bearbeiten' ('Edit trust'). The radio button for '... nicht vertrauen' ('... not trust') was set.
- 'OK'.
- Ansehen ('View') the certificate: "Dieses Zertifikat wurde für die folgenden Anwendungen zertifiziert" (This certificates was certified for the following applications): SSL-Client-Zertifikat | SSL-Client-Zertifikat.
- 'Vertrauen bearbeiten' ('Edit trust'). The radio button for '... vertrauen' ('... trust') was set. This is fatal bug 1.
- Selected radio button "Der Echtheit dieses Zertifikats nicht vertrauen" (".. not trust"). 
- "OK".




Actual results:

Nothing happened. The certificate is still trusted. Trust cannot be removed. This is fatal bug 2.
In one of the entries the identification authorizations for Websites, Mail and Software were even set ('login.live.com' or 'login.skype.com', cannot remember).
After my experiments most of the stolen USERTRUST Network certificates are trusted now and there is nothing I can do but remove the entries and take care myself.


Expected results:

Trust must not be switched from "not trust" to "do trust" just by editing.
It must be possible to edit trust to "nopt trust".
Furthermore the list in tab 'Server' should (I say *must*) display the current trust settings. As it is now, the user cannot tell by looking at the entry whether this is an entry to trust our *not* trust a certificate. This is why I had a look via Edit Trust and damaged my installation.

Comment 1

[Hayo]

Obviously I used a German Firefox. My translations are just guesses.

Comment 2

[Hayo]

I can confirm now that the same problem exists with the Windows version of Firefox 17.0.1.
I edited The USERTRUST Network - login.skype.com by doing nothing and exited with OK. The certificate got changed to trusted, plus the trust configurations are all checked to identify for websites, mail and software.
This may cause the maximum damage to users - automatic trust to fraud, without any user feedback. IMHO for any Firefox versions in development this bug should be set to BLOCKER.
Environment: 'Mozilla Firefox 17.0.1, Mozilla Firefox EU, euballot 1.1' running on Windows 7 Professional 64-bit.

Comment 3

[Matthias Versen [:Matti]]

I can confirm that the "Trust the authenticity of this certificate" checkbox is enabled after opening the "edit trust" setting and hitting "Ok" and it can not be changed back.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0

Comment 4

[:Cykesiopka]

Some poking around in the cert manager code, hope this helps:

In editcerts.js, function doSSLOK():
  var trustssl = sslTrust.value ? nsIX509CertDB.TRUSTED_SSL : 0;

sslTrust.value seems correct (it's either true or false), however, since (typeof sslTrust.value) is 'string' and non-empty, trustssl is always set to nsIX509CertDB.TRUSTED_SSL

Comment 5

[Matthias Versen [:Matti]]

Brian: Would comment #4 explain this bug ?

Comment 6

[:Cykesiopka]

So after digging a bit deeper:
  - sslTrust.value is a string because sslTrust is a XUL radiogroup
    (https://developer.mozilla.org/en-US/docs/XUL/radiogroup)
  - "true" and "false" come from the values of the two <radio> elements

Relevant part from editsslcert.xul:
  <radiogroup id="sslTrustGroup">
    <radio label="&certmgr.editsslcert.dotrust;" value="true"/>
    <radio label="&certmgr.editsslcert.donttrust;" value="false"/>
  </radiogroup>

Changing the line I mentioned earlier to this:
  var trustssl = sslTrust.value == "true" ?
                 nsIX509CertDB.TRUSTED_SSL :
                 nsIX509CertDB.UNTRUSTED;
fixed the issue for me when I tested it.

On a related note, it looks like editemailcert.xul and doEmailOK() suffer from the exact same issue.

Comment 7

[:Cykesiopka]

Attachment: Proposed Patch

This patch also removes a few unused variables.

Comment 8

[:Cykesiopka]

Review ping?

Comment 9

[Hayo]

I must express my disappointment that after six months this critical security bug is neither fixed nor even assigned to anybody.

Furthermore the volunteer work an bugfixing Cykesiopka is not reviewed by anyone for more than three months.

I switched the platform to 'All'/'All', since this is likely the case. Also set importance to 'P1' to get visibility. This should have been done by a reviewer in the Mozilla Foundation for months. 

Shall we also raise it from 'critical' to 'blocker'?

Comment 10

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

First, a workaround: delete the certificate override entry for which the trust has been mis-set. It will be re-populated on restart with the appropriate trust settings because it is a built-in entry. This brings me to the main issue: trust for these certificates should not be editable in the first place. In fact, I doubt their existence should even be exposed in the UI. More generally, our entire certificate management UI is in need of attention. As I understand it, the UX team will eventually come up with a new design. In the meantime, it is difficult to justify working on significant changes without their input. That being said, this fix is not a significant change. Rather, it is a simple bug fix.

Cykesiopka - if you're still interested in working on this, I'll make sure it happens. If not, I'll make sure it happens anyway.

Hayo - we appreciate the time you took to point out this bug. However, the passive-aggressive attitude is not appreciated.

Comment 11

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

Comment on attachment 725856 [details] [diff] [review]
Proposed Patch

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

There may be additional unused variables in this file - if you wouldn't mind having a look, that would be great. Also, there is some commented-out code that can all be removed.
If you're still interested in working on this, flag me for r? with an updated patch.

::: security/manager/pki/resources/content/editcerts.js
@@ +117,5 @@
>  {
>    var sslTrust = document.getElementById("sslTrustGroup");
> +  var trustssl = sslTrust.value == "true" ?
> +                 nsIX509CertDB.TRUSTED_SSL :
> +                 nsIX509CertDB.UNTRUSTED;

I think a more readable style would be to indent these two lines two spaces each.

@@ +171,5 @@
>  {
>    var sslTrust = document.getElementById("sslTrustGroup");
> +  var trustemail = sslTrust.value == "true" ?
> +                   nsIX509CertDB.TRUSTED_EMAIL :
> +                   nsIX509CertDB.UNTRUSTED;

Same as above.

Comment 12

[:Cykesiopka]

Attachment: Proposed Patch v2

+ Additional indentation
+ Removed commented out code

I didn't find any additional unused variables.

Comment 13

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

Comment on attachment 785330 [details] [diff] [review]
Proposed Patch v2

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

Cykesiopka: do you have access to the try server? If so, it would be great if you could do a try run (something like "try: -b do -p linux64 -u all -t none -n") to make sure no existing tests break.

::: security/manager/pki/resources/content/editcerts.js
@@ +49,1 @@
>                            nsIX509CertDB.TRUSTED_OBJSIGN)) {

Tiny nit: this line needs one more space of indentation (same with lines 42 and 35).
Whoever checks this in can take care of it, though (it'll probably be me).

Comment 14

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

-  var trustssl = sslTrust.value ? nsIX509CertDB.TRUSTED_SSL : 0;
+  var trustssl = sslTrust.value == "true" ?
+                   nsIX509CertDB.TRUSTED_SSL :
+                   nsIX509CertDB.UNTRUSTED;

It seems like, out of all the various changes in this patch, the only one that matters is replacing 'sslTrust.value' with 'sslTrust.value == "true"', so that is the only change that should get committed in this bug. The other changes are good, but they should be done in a separate commit. Otherwise, it gets confusing about what you are changing. For example, it is confusing to replace '0' with nsIX509CertDB.UNTRUSTED; it seems like a semantically-important change until you realize that nsIX509CertDB.UNTRUSTED == 0.

Comment 15

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

<briansmith> I actually wonder if even this change is correct.
<briansmith> Because, there are three states: TRUSTED, DISTRUSTED, and INHERITS TRUST
<briansmith> but, there are only two states in the UI.
<briansmith> I wonder if this patch changes the certs from DISTRUSTED to INHERITS TRUST?

Should we instead have three radio buttons?

Comment 16

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

Brian and I talked a bit more on irc:

2013-08-05 14:00:46 briansmith  keeler: here's a potential solution for all those problems:
2013-08-05 14:00:58 briansmith  just don't let the user edit certificate trust in the server tab
2013-08-05 14:01:10 briansmith  i.e. only let them trust/distrust CAs
2013-08-05 14:01:41 briansmith  definitely, it doesn't make sense to explicitly trust an end-entity cert. Then it could never be revoked
2013-08-05 14:03:23 keeler  briansmith: ok - sounds good
2013-08-05 14:04:05 keeler  briansmith: although, that's the tab where permanent ee exceptions get stored. The only way to affect their trust at that point would be to delete them, which is probably best anyway.
2013-08-05 14:04:39 briansmith  keeler: yes, deleting them would be best.
2013-08-05 14:04:55 keeler  briansmith: ok - sounds like a plan

tl;dr: Remove that button/dialog entirely from the "servers" tab. I would say we should remove it from the "people" tab as well, but I don't really know what that one's about, and I think we should remove the "people" tab entirely if it's not being used for anything (that's for another bug, though).
Anyway, sorry to change the bug parameters on you again, Cykesiopka, but I think that's the best approach at this point.
Also, just a heads-up: I'm on PTO for a week starting tomorrow, so I won't be responding to bugmail until I get back.

Comment 17

[:Cykesiopka]

(In reply to David Keeler (:keeler) [PTO 8/7-8/14] from comment #13)
> Cykesiopka: do you have access to the try server?

I don't have access to the try server unfortunately.


(In reply to David Keeler (:keeler) [PTO 8/7-8/14] from comment #16)
> tl;dr: Remove that button/dialog entirely from the "servers" tab. I would
> say we should remove it from the "people" tab as well, but I don't really
> know what that one's about, and I think we should remove the "people" tab
> entirely if it's not being used for anything (that's for another bug,
> though).
> Anyway, sorry to change the bug parameters on you again, Cykesiopka, but I
> think that's the best approach at this point.
> Also, just a heads-up: I'm on PTO for a week starting tomorrow, so I won't
> be responding to bugmail until I get back.

No problem. I'm making steady progress on a patch that removes the trust editing from the server tab.

However with this approach, it looks like the similar issue in editemailcert.xul and doEmailOK() won't be fixed... For another bug?

Comment 18

[:Cykesiopka]

(In reply to Cykesiopka from comment #17)
> However with this approach, it looks like the similar issue in
> editemailcert.xul and doEmailOK() won't be fixed... For another bug?

Sorry, ignore this. I was confused between OthersOverlay.xul (People Tab) with OrphanOverlay.xul (Others Tab)

Comment 19

[:Cykesiopka]

Attachment: Remove Cert Editing in Server Tab v1

Comment 20

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

I will not be able to look at this for at least a week because I am on PTO.

Comment 21

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

Comment on attachment 787370 [details] [diff] [review]
Remove Cert Editing in Server Tab v1

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

Looks good! Just one nit.

::: security/manager/locales/en-US/chrome/pippki/certManager.dtd
@@ -39,4 @@
>  
>  <!ENTITY certmgr.editcert.title               "Edit Security Certificate Settings">
>  <!ENTITY certmgr.editcacert.title             "Edit CA certificate trust settings">
>  <!ENTITY certmgr.editsslcert.title            "Edit website certificate trust settings">

I think you can remove this one too.

Comment 22

[:Cykesiopka]

Attachment: Remove Cert Editing in Server Tab v2

+ Removed certmgr.editsslcert.title as well

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/198caba447af

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/198caba447af