1520216 - Crash in nssCertificate_Destroy because NSSCertificate->decoding is null (or invalid)

Status: RESOLVED INVALID

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

Related: bug 1453518, bug 1386601

The above is the current topcrash in Thunderbird.
Nobody has a reliable way to reproduce it.

In all scenarios, the call stack indicates that we originate from NSS_CMSSignedData_ImportCerts, related to verifying the signature inside S/MIME messages.

At the time of the crash, in most cases I've looked at, there were at least two threads active in NSS_CMSSignedData_ImportCerts. (This could mean the user has clicked on another email message, while the signature verification of the previous message has not yet finished.)

We crash here:

nssCertificate_Destroy(
NSSCertificate *c)
{
if (c) {
nssDecodedCert *dc = c->decoding;

This functions assumes that c->decoding MUST have been set to a valid pointer.
But is this really guaranteed in all circumstances?

I found two places, where a new NSSCertificate structure is created, and where c->decoding could remain NULL.

(1) nssCertificate_Create

It calls nss_ZNEW(arena, NSSCertificate), but I don't see and code that sets the decoding member.

(2) STAN_GetNSSCertificate

It also uses nss_ZNEW(arena, NSSCertificate).
It attempts to set
c->decoding = create_decoded_pkix_cert_from_nss3cert(NULL, cc);

However, create_decoded_pkix_cert_from_nss3cert may return NULL in an out-of-memory scenario.

I don't know if NSS guarantees that nssCertificate_Destroy will never be called on a NSSCertificate*. If this is uncertain, then I suggest we change nssCertificate_Destroy to be failsafe.

Comment 1

[Kai Engert [:KaiE:]]

Sigh, I just realize I made a thought mistake.

It's not about member decoding. If c != NULL, but we crash dereferencing c->decoding, it means the pointer c is pointing to invalid memory.