Edit Trust in Certificate Manager - Server sets dangerous wrong values

RESOLVED FIXED in mozilla26

Status

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: bugzilla, Assigned: Cykesiopka)

Tracking

({dev-doc-needed})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Comment 1

7 years ago
Obviously I used a German Firefox. My translations are just guesses.
Severity: normal → critical
Reporter

Comment 2

7 years ago
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.
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
Status: UNCONFIRMED → NEW
Component: Untriaged → Security: UI
Ever confirmed: true
Product: Firefox → Core
Assignee

Comment 4

6 years ago
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
Brian: Would comment #4 explain this bug ?
Assignee

Comment 6

6 years ago
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.
Assignee

Comment 7

6 years ago
Posted patch Proposed Patch (obsolete) — Splinter Review
This patch also removes a few unused variables.
Attachment #725856 - Flags: review?(bsmith)
Assignee

Comment 8

6 years ago
Review ping?
Reporter

Comment 9

6 years ago
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'?
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
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.
Severity: critical → normal
Priority: P1 → --
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.
Attachment #725856 - Flags: review?(brian) → feedback+
Assignee

Comment 12

6 years ago
Posted patch Proposed Patch v2 (obsolete) — Splinter Review
+ Additional indentation
+ Removed commented out code

I didn't find any additional unused variables.
Assignee: nobody → cykesiopka.bmo
Attachment #725856 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #785330 - Flags: review?(dkeeler)
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).
Attachment #785330 - Flags: review?(dkeeler) → review+
-  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.
<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?
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.
Severity: normal → critical
Status: ASSIGNED → NEW
OS: All → Linux
Hardware: All → x86_64
Severity: critical → normal
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee

Comment 17

6 years ago
(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?
Assignee

Comment 18

6 years ago
(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)
Assignee

Comment 19

6 years ago
Attachment #785330 - Attachment is obsolete: true
Attachment #787370 - Flags: review?(brian)
I will not be able to look at this for at least a week because I am on PTO.
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.
Attachment #787370 - Flags: review?(brian) → review+
Assignee

Comment 22

6 years ago
+ Removed certmgr.editsslcert.title as well
Attachment #787370 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/198caba447af
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla26
Assignee

Updated

4 years ago
Duplicate of this bug: 907531
Product: Core → Core Graveyard

Updated

3 years ago
Depends on: 1305385
You need to log in before you can comment on or make changes to this bug.