910986 - Cert trust editing broken in People tab of Cert Manager

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[:Cykesiopka]

Similar to the Server tab from Bug 825583, editing the trust of a cert listed in the People tab of the Cert Manager does not work properly.

The reasoning for the brokenness described in Bug 825583 Comment 4 and 6 most likely also applies for the People tab.

Comment 1

[:Cykesiopka]

Attachment: Proposed Patch v1

Remove the ability to edit cert trust in the People tab, similar to what was done in Bug 825583.

Comment 2

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

I think this is good. However, this will remove the ability to trust email certificates in Thunderbird that are either self-signed or signed by an unknown CA. The solution there would be to import and trust the appropriate CA, but since there isn't an easy "add certificate override" button like with servers in Firefox (as far as I know), this may have detrimental effects on usability. I think Mark would know if this is a concern.

Comment 3

[Mark Banner (:standard8)]

Sorry for not responding earlier, I don't actually know to be honest. You could try posting on tb-planning. An alternative is if it was removed, could the UI be moved to Thunderbird instead with it keeping working?

Comment 4

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

Comment on attachment 798401 [details] [diff] [review]
Proposed Patch v1

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

The consensus on the mailing lists seems to be to remove the entire People tab from Firefox but add it back in Thunderbird (I guess with an overlay? I'm not really sure of the best way to do that.) As part of that, we should probably actually fix the trust setting issue (in Thunderbird), rather than removing that button.

Comment 5

[:Cykesiopka]

Attachment: bug910986_v2.patch

Same approach as Bug 825583 Attachment 725856 [details] [diff].

I've checked that this patch fixes the issue at the UI level.

However:
Bug 825583 Comment 15:
> <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?

I don't know the answer to this sadly, so just asking for feedback until I or someone else finds out...

Comment 6

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

Comment on attachment 8344271 [details] [diff] [review]
bug910986_v2.patch

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

I've done a bit of investigation with these trust bits, and I believe this will toggle a certificate between DISTRUSTED and TRUSTED, but not INHERITS TRUST. I'm not sure this is entirely what we want in the end, but:

a) this seems to be a failure of the nsIX509CertDB::setCertTrust interface in general
b) this whole UI needs a lot of attention

Neither of which should hold up this patch, which at least makes the implementation more correct than it was.

r=me with the change noted below. Thanks for the patch.

::: security/manager/pki/resources/content/editcerts.js
@@ +107,5 @@
>  {
>    var sslTrust = document.getElementById("sslTrustGroup");
> +  var trustemail = sslTrust.value == "true" ?
> +                     nsIX509CertDB.TRUSTED_EMAIL :
> +                     0;

nsIX509CertDB.UNTRUSTED would be more clear instead of "0" here.
Also, the arrangement of ? and : should look more like this:
... = sslTrust.value == "true"
      ? nsIX509CertDB.TRUSTED_EMAIL
      : nsIX509CertDB.UNTRUSTED;

(where the ? and : are lined up with the first "s")

Comment 7

[:Cykesiopka]

(In reply to David Keeler (:keeler) from comment #6)
> I've done a bit of investigation with these trust bits, and I believe this
> will toggle a certificate between DISTRUSTED and TRUSTED, but not INHERITS
> TRUST. I'm not sure this is entirely what we want in the end, but:
> 
> a) this seems to be a failure of the nsIX509CertDB::setCertTrust interface
> in general
> b) this whole UI needs a lot of attention
> 
> Neither of which should hold up this patch, which at least makes the
> implementation more correct than it was.
> 
> r=me with the change noted below. Thanks for the patch.

Thanks for the investigation and review!

Comment 8

[:Cykesiopka]

Attachment: bug910986_v3.patch

For check in.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/418110dd6003

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/418110dd6003

Comment 11

[Andrei Vaida [:avaida]]

Hi Cykesiopka, I'm not sure if it's feasible or not, but please bare with me: I was wondering if there's a sample security certificate that I could import in order to verify this fix. At this point, I don't have a profile with a certificate that falls in the "People" category and while the list from that tab is empty, the "Edit Trust..." button is still visible (although disabled with the rest of the buttons) when compared to the "Servers" tab.

Also, if I understood correctly, this fix should have removed the "Edit Trust..." button completely from the "People" tab, please correct me if I'm wrong.

Comment 12

[:Cykesiopka]

Hi Andrei,

I'm going to attach two sample certs that should let you verify the fix.

I created these certs by following section 2.3 of http://www.howtoforge.com/how-to-encrypt-mails-with-ssl-certificates-s-mime .

Steps (just in case):
- Import ca.crt
  - Trust for e-mail
- Import cert.crt
- If all goes well, an entry should show up with the following info:
    Certificate Name: b910986 Person
    E-Mail Address: [e-mail]
    Expires On: <2015-04-01>

Comment 13

[:Cykesiopka]

(In reply to Andrei Vaida, QA [:AndreiVaida] from comment #11)
> Also, if I understood correctly, this fix should have removed the "Edit
> Trust..." button completely from the "People" tab, please correct me if I'm
> wrong.

That was my original intention. However David said that doing that "may have detrimental effects on usability" (See Comments 2 and 4), so the fix just corrected the bug instead.

Comment 14

[:Cykesiopka]

Attachment: ca.crt

Comment 15

[:Cykesiopka]

Attachment: cert.crt

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Cykesiopka from comment #13)
> That was my original intention. However David said that doing that 
> "may have detrimental effects on usability" (See Comments 2 and 4), 
> so the fix just corrected the bug instead.
Thank you for clarifying this.

(In reply to Cykesiopka from comment #12)
> I created these certs by following section 2.3 of 
> http://www.howtoforge.com/how-to-encrypt-mails-with-ssl-certificates-s-mime .
This might be quite helpful on future bugs from the same category, thanks for sharing.

(In reply to Cykesiopka from comment #12)
> Steps (just in case):
> - Import ca.crt
>  - Trust for e-mail
> - Import cert.crt
> - If all goes well, an entry should show up with the following info:
>    Certificate Name: b910986 Person
>    E-Mail Address: [e-mail]
>    Expires On: <2015-04-01>
I was only able to import these two certificates from the "Servers" tab, while my attempts of importing them from the "People" tab resulted in the following error: "This certificate can't be verified and will not be imported. The certificate issuer might be unknown or untrusted, the certificate might have expired or been revoked, or the certificate might not have been approved.".

I also tried exporting certificates from Thunderbird's "People" tab and importing them into Firefox's "People" tab, but with no success - I assume this is expected in terms of compatibility or certificate types (?). 

I will research myself further, but if you come up with any other ideas on how to verify this, please let me know.

Comment 17

[:Cykesiopka]

(In reply to Andrei Vaida, QA [:avaida] from comment #16)
> I was only able to import these two certificates from the "Servers" tab,
> while my attempts of importing them from the "People" tab resulted in the
> following error: "This certificate can't be verified and will not be
> imported. The certificate issuer might be unknown or untrusted, the
> certificate might have expired or been revoked, or the certificate might not
> have been approved.".

Sorry, I neglected to mention that ca.crt should be imported under the Authorities tab, and that cert.crt should be imported under the People tab.

> I also tried exporting certificates from Thunderbird's "People" tab and
> importing them into Firefox's "People" tab, but with no success - I assume
> this is expected in terms of compatibility or certificate types (?). 

Unfortunately I don't know why that doesn't work, sorry!

> I will research myself further, but if you come up with any other ideas on
> how to verify this, please let me know.

I'm not sure if there are other ways to verify this, but this is how I did it:
- Highlight the "b910986 Person" entry
- Press the Edit Trust button
- Make sure that "Do not trust [...]" is selected
- Press the OK button
- Press the Edit Trust button again, and make sure the cert stays distrusted (and trusted stays trusted as well I suppose)

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Cykesiopka from comment #17)
> Sorry, I neglected to mention that ca.crt should be imported under the
> Authorities tab, and that cert.crt should be imported under the People tab.
It worked, thanks Cykesiopka!

This issue is verified fixed on Windows 7 64-bit [1], using:
- the latest Beta (Build ID: 20140331125246),
- the latest Aurora (Build ID: 20140402004007),
- the latest Nightly (Build ID: 20140402030201).


1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0