Closed Bug 1130418 Opened 7 years ago Closed 7 years ago

Remove e-mail cert trust editing UI

Categories

(Core Graveyard :: Security: UI, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
OS: Windows 7 → All
Hardware: x86 → All
Attachment #8565411 - Flags: review?(dkeeler)
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.
Attachment #8565411 - Flags: review?(dkeeler)
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
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.
Summary: Fix editcerts.js getCaCertForEntityCert() logic → Remove e-mail cert trust editing UI
Target Milestone: --- → mozilla39
Attachment #8565411 - Attachment is obsolete: true
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.
Attachment #8570108 - Flags: review?(dkeeler) → review?(VYV03354)
> 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?
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.
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?
(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.
(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 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.
Attachment #8570108 - Flags: review?(VYV03354) → review+
+ Remove some nearby trailing whitespace as well
Attachment #8570108 - Attachment is obsolete: true
Attachment #8571738 - Flags: review+
Blocks: 423025
Product: Core → Core Graveyard
Depends on: 1305385
You need to log in before you can comment on or make changes to this bug.