129709 - Illegal free and memory leak of cert->nickname in __CERT_AddTempCertToPerm

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

In NSS 3.4, we exported the function __CERT_AddTempCertToPerm for PSM.

In stanpcertdb.c, rev. 1.38, this function does the following:

    if (stanNick && nickname && strcmp(nickname, stanNick) != 0) {
        /* take the new nickname */
        PORT_Free(cert->nickname);
        stanNick = NULL;
    }
    if (!stanNick && nickname) {
        stanNick = nssUTF8_Duplicate((NSSUTF8 *)nickname, c->object.arena);
        cert->nickname = PORT_Strdup(nickname);
    }

However, cert->nickname should be allocated in cert->arena (see pki3hack.c,
fill_CERTCertificateFields).  So, there are two scenarios in which the
above code will be wrong.

1. If someone calls CERT_NewTempCertificate with one nickname, then
calls CERT_AddTempCertToPerm with a *different* nickname, we will make an
incorect call to PORT_Free, and then call PORT_Strdup to allocate
cert->nickname, which will be leaked.  The incorrect call to PORT_Free
is especially serious because it frees a buffer allocated in an arena.

The good news is that this scenario is unlikely to happen.  In particular,
PSM doesn't do this.

2. If someone calls CERT_NewTempCertificate with a null nickname, then
calls CERT_AddTempCertToPerm with a *non-null* nickname, we will call
PORT_Strdup to allocated cert->nickname, which will be leaked.

This is what PSM does, so PSM will hit the leak.

But certs are rarely imported permanently.  The leak is just the size of
a nickname buffer, and it will occur once per downloaded cert per session.
That number should be extremely low.

With the above analysis and the safe assumption that PSM is the only user
of __CERT_AddTempCertToPerm, I recommend that we not fix this bug before
NSS 3.4 RTM and we fix it in NSS 3.4.1.

Comment 1

[Wan-Teh Chang]

Attachment: Ian's proposed patch

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I'm not entirely certain that PSM will remain the only user of 
__CERT_AddTempCertToPerm in the 3.4 timeframe (before 3.4.1 is released).  
If we have a low risk patch now ...

Comment 3

[Ian McGreer]

patch checked in per wtc's request.

Comment 4

[Wan-Teh Chang]

Marked the bug fixed.

Comment 5

[Wan-Teh Chang]

Attachment: Patch to fix a memory leak in nssList_Create on error

I found out about this memory leak when I reviewed the nssList
code.  We leak the arena if nss_ZNEW fails.  r=mcgreer.  I've
checked in the patch.

Comment 6

[Wan-Teh Chang]

Comment on attachment 74417 [details] [diff] [review]
Patch to fix a memory leak in nssList_Create on error

Sorry, attached to the wrong bug.

Comment 7

[Wan-Teh Chang]

Comment on attachment 74417 [details] [diff] [review]
Patch to fix a memory leak in nssList_Create on error

I meant to attach this patch to bug 129778, although
this patch is not really related to that bug either.