1126027 - Segfault at nsNSSCertificateDB::SetCertTrust() after changing trust settings of an e-mail cert in the cert manager

Status: RESOLVED DUPLICATE of bug 1168048

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

STR:
 - Go to the "Authorities" tab of the Cert Manager and import the CA cert I will attach to the bug
   - Trust for e-mail
 - Go to the "People" tab and import the e-mail cert
 - Trust the e-mail cert via the "Edit Trust..." dialogue
 - Press OK to dismiss the Cert Manager, then reopen it
 - Find the e-mail cert that should now be under the "Servers" tab and "Delete..." it
 - Press OK to dismiss the Cert Manager, then reopen it
 - Go to the "People" tab, select the e-mail cert, and open and close the "Edit Trust..." dialogue until the top section that is usually filled with text is empty
   - This shouldn't take more than 10 tries
 - Press OK
-> Firefox crashes due to a null pointer deref induced segfault

Marking as s-s for now just in case.

Comment 1

[:Cykesiopka]

Attachment: exampleRootCert0.der

Comment 2

[:Cykesiopka]

Attachment: exampleEmailCert0.0.der

Comment 3

[:Cykesiopka]

The only interesting part of a gdb backtrace (the rest is all JS and XPConnect stuff):
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007f453ff463ee in nsNSSCertificateDB::SetCertTrust (this=0x7f45169026a0, cert=0x0, type=4, trusted=2) at /home/m-c_drive/mozilla-inbound/security/manager/ssl/src/nsNSSCertificateDB.cpp:992
> 992	  ScopedCERTCertificate nsscert(cert->GetCert());

Comment 4

[:Cykesiopka]

Notes on my brief investigation:
- Pressing "Edit Trust..." opens editemailcert.xul
- editcerts.js::doLoadForEmailCert() is called as the onload for editemailcert.xul

- doLoadForEmailCert() calls nsNSSCertificateDB::FindCertByDBKey():
> function doLoadForEmailCert()
> {
>  var dbkey = self.name;
> 
>  //  Get the cert from the cert database
>  certdb = Components.classes[nsX509CertDB].getService(nsIX509CertDB);
>  cert = certdb.findCertByDBKey(dbkey, null);
>  ...
> }

- nsNSSCertificateDB::FindCertByDBKey() calls CERT_FindCertByIssuerAndSN(), which sometimes returns null:
  => The global var |cert| in editcerts.js gets assigned to null
> nsNSSCertificateDB::FindCertByDBKey(const char *aDBkey, nsISupports *aToken,
>                                    nsIX509Cert **_cert)
> {
> ...
>   cert = CERT_FindCertByIssuerAndSN(CERT_GetDefaultCertDB(), &issuerSN);
>   PR_FREEIF(keyItem.data);
>   if (cert) {
>     nsCOMPtr<nsIX509Cert> nssCert = nsNSSCertificate::Create(cert.get());
>     if (!nssCert)
>       return NS_ERROR_OUT_OF_MEMORY;
>     nssCert.forget(_cert);
>   }
>   return NS_OK;
> }

- User presses "OK" and editcerts.js::doEmailOK() is called as the ondialogaccept handler
- doEmailOK() calls nsNSSCertificateDB::SetCertTrust(), passing in a null pointer for the |cert| argument
> function doEmailOK()
> {
>   ...
>   certdb.setCertTrust(cert, nsIX509Cert.EMAIL_CERT, trustemail);
>   return true;
> }

- Null pointer deref occurs:
> nsNSSCertificateDB::SetCertTrust(nsIX509Cert *cert, 
>                                  uint32_t type,
>                                  uint32_t trusted)
> {
> ...
>   ScopedCERTCertificate nsscert(cert->GetCert());
> ...
> }

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

Null ptr deref after a fair amount of user interaction. We're not sure if it's even a sec bug but rating sec-low for now. Others feel free to weigh in.

Comment 6

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

This might be fixed by bug 1139205 (although it would still be a good idea for PSM code to null-check its arguments in general).

Comment 7

[:Cykesiopka]

The UI for this was removed in Bug 1130418, and Bug 1168048 adds null checks.
I guess this can be resolved/opened up etc?

Comment 8

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

Yep - I think we're good here.