1130418 - Remove e-mail cert trust editing UI

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[:Cykesiopka]

getCaCertForEntityCert() in editcerts.js has some logic issues:
> if((nextCertInChain.type == nsIX509Cert.CA_CERT) ||
This is not a valid attribute of a nsIX509Cert, and I suspect has never been, even on nsIX509Cert2 or nsIX509Cert3.

> (nextCertInChain.subjectName == lastSubjectName))
I guess this is supposed to find self signed certs where the issuer name is equal to the subject name? I don't think this actually works though - using .issuer on a root cert just returns null, not the cert itself.

> nextCertInChain = cert;
I played around a bit with the cert manager, and it doesn't look like it's possible to get to this function and also have the cert in question be its own issuer... I think it's safe to just change this to |nextCertInChain = cert.issuer|, but maybe I'm misunderstanding.

Comment 1

[:Cykesiopka]

Attachment: bug1130418_fix-editcerts.js-getCaCertForEntityCert()-logic_v1.patch

Comment 2

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

Comment on attachment 8565411 [details] [diff] [review]
bug1130418_fix-editcerts.js-getCaCertForEntityCert()-logic_v1.patch

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

Honestly, if people haven't noticed how broken this is already, we should just remove the feature entirely (because clearly no one is using it). I think that would involve removing the "Edit Trust" button/dialog from the "People" tab and any code/UI that's only reachable from there.

Comment 3

[:Cykesiopka]

IRC conversation expanding on Comment 2:

> Cykesiopka: keeler: Just to confirm, for Bug 1130418, were you suggesting removing the entire e-mail cert trust editing dialogue, or just the "Edit CA Trust"/getCaCertForEntityCert() bits?
> keeler: Cykesiopka: everything after and including the "Edit Trust" button, so the former
> Cykesiopka: keeler: mm, ok. Is there no value for explicit distrust for S/MIME certs?
> keeler: well, like I said, it doesn't work and nobody appears to have complained yet, so I would say no
> keeler: Cykesiopka: in general, I think the current implementation of the certificate manager doesn't serve our users' needs well. I believe there's something better in the works
> Cykesiopka: oh, does the entire distrust thing not work? I thought it was just the Edit CA Trust part being broken
> keeler: it didn't work for me - if I double-click on an email cert, it shows that it validates fine as an email cert, but the "Edit Trust" dialog says it's untrusted (and the "Edit CA Trust" says it can't find an issuer, as we've seen)
> Cykesiopka: keeler: oh, that's because the string shown there also depends on getCaCertForEntityCert()
> Cykesiopka: https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/editcerts.js#87
> Cykesiopka: (unless I'm thinking of the wrong string)
> keeler: Cykesiopka: I think that's part of it, yes. The other part is I don't think that dialog correctly implements setting the distrust bit (I could be wrong, though). It could also be the case that NSS isn't correctly keeping the distrust bit (see e.g. bug 1045907)
> Cykesiopka: keeler: I see. If that's the case, I'll just morph and get the removal patch landed after the nightly branch point

Comment 4

[:Cykesiopka]

Per comment above, I'm going to morph this bug, and get the removal landed after the Nightly -> Aurora merge.

This code has quite a few issues, so if there's some functionality that's still needed, it should be reimplemented in a more sane manner.

Comment 5

[:Cykesiopka]

Attachment: bug1130418_rm-email-cert-trust-editing-ui_v1.patch

Try push looks OK so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd51981d38cb

Comment 6

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

Comment on attachment 8570108 [details] [diff] [review]
bug1130418_rm-email-cert-trust-editing-ui_v1.patch

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

:emk, would you be interested in reviewing this? If not, I'll find another reviewer.

Comment 7

[Masatoshi Kimura [:emk]]

> keeler: it didn't work for me - if I double-click on an email cert, it shows that it validates fine as an email cert, but the "Edit Trust" dialog says it's untrusted (and the "Edit CA Trust" says it can't find an issuer, as we've seen)

Will the email certificate be trusted even if the CA is not trusted for email users? If so, isn't it a critical security bug?

Comment 8

[Masatoshi Kimura [:emk]]

1. I exported a email certificate from operating system's certificate store with full path.
2. I imported the email certificate from Advanced-Certificates-View Certificates-People-Import.
3. The following Alert was displayed twice.
   "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."
4. The certificate was imported despite that the message said it would not.
5. Another certificate was added under the Authorities tab which is the CA of the imported certificate. All trusted bits were off.
6. If I double-click the imported certificate, Certificate Viewer said "This certificate has been verified" despite that the CA is not trusted.

Comment 9

[Masatoshi Kimura [:emk]]

The imported CA was an intermediate CA. The root CA was "Verisign Class 2 Public Primary Certification Authority - G3". If I turned off the email bit of the latter, the imported email certificate was no longer verified. Are trusted bits of intermediates meaningless?

Comment 10

[:Cykesiopka]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> The imported CA was an intermediate CA. The root CA was "Verisign Class 2
> Public Primary Certification Authority - G3". If I turned off the email bit
> of the latter, the imported email certificate was no longer verified. Are
> trusted bits of intermediates meaningless?

I believe removing the trust bits just removes *explicit* trust - the intermediates can still inherit trust from any trusted roots, but I'm no NSS expert. Bug 585352 is about the misleading UI about this.

Comment 11

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

(In reply to Masatoshi Kimura [:emk] from comment #7)
> > keeler: it didn't work for me - if I double-click on an email cert, it shows that it validates fine as an email cert, but the "Edit Trust" dialog says it's untrusted (and the "Edit CA Trust" says it can't find an issuer, as we've seen)
> 
> Will the email certificate be trusted even if the CA is not trusted for
> email users? If so, isn't it a critical security bug?

The "Edit Trust" dialog was displaying incorrect information. The certificate viewer was showing the correct information (i.e. there was a trusted path from the email certificate to a root that was trusted to issue email certificates, etc.).

Cykesiopka is right - removing the trust bits from an intermediate is misleading and basically useless since that certificate can inherit trust from its issuer. (Also note that active distrust isn't really a thing - see e.g. bug 1045907.) That's basically why we're removing this dialog.

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 8570108 [details] [diff] [review]
bug1130418_rm-email-cert-trust-editing-ui_v1.patch

Oh, then this patch will remove the misleading UI.
Looks good to me.

Comment 13

[:Cykesiopka]

Attachment: bug1130418_rm-email-cert-trust-editing-ui_v2.patch

+ Remove some nearby trailing whitespace as well

Comment 14

[:Cykesiopka]

Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e80498c60d0

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7b4c25de7d

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/fb7b4c25de7d