633002 - SECKEY_ImportDERPublicKey causes memory leaks

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Elio Maldonado]

Reprted by Miloslav Trmač:

Description of problem:
SECKEY_DestroyPublicKey(SECKEY_ImportDERPublicKey(...)) leaks memory.

Version-Release number of selected component (if applicable):
nss-3.12.7-6.fc13.x86_64, verified also on RHEL6 beta

Additional info:
AFAICS, other functions initialize an arena, allocate a SECKEYPublicKey out of
the arena, and set key key->arena to the arena.  SECKEY_DestroyPublicKey then
frees key->arena, which frees "key" implicitly.

SECKEY_ImportDERPublicKey uses PORT_ZNew to allocate the SECKEYPublicKey, and
it is never freed.

Comment 1

[Robert Relyea]

This behavior is definitely incorrect.

The function is an old function that has little use. If we can determine that
all users have been assuming SECKEY_DestroyPublicKey() will free the key, then
we can just fix the bug.

If we find there are users that then also call PORT_Free() on the result, then
we'll have to deprecate SECKEY_ImportDERPublicKey, create a new function with a
different name that has the correct semantics, and keep the old function around
for those older applications (per NSS upstream rules for binary compatibility).

It believe Dogtag/CMS/JSS is the most likely user of this function (given the
original patch was from an CMS/JSS developer).

Comment 2

[Robert Relyea]

I found one caller in JSS.

PK11Pub.c

This function wraps NSS public keys from 2 sources, one importing from and
imported DER, and one from the extracted public key. Both are freed by the same
free routine. This means that a corrected SECKEY_ImportDERPublicKey will not
crash JSS, and will fix and existing leak in that package.

Comment 3

[Robert Relyea]

Alexei, could you check that no oracle servers call this function or if they do, free's the result only with SECKEY_DestroyPublicKey()?

Thanks,

bob

Comment 4

[Elio Maldonado]

Attachment: Allocate arena then allocate SECKEYPublicKey off of this arena

Comment 5

[Wan-Teh Chang]

Comment on attachment 517156 [details] [diff] [review]
Allocate arena then allocate SECKEYPublicKey off of this arena

Elio: thanks for the patch.  We should fix this bug
in NSS 3.12.10.

>+    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>+    if (arena == NULL) {
>+	PORT_SetError(SEC_ERROR_NO_MEMORY);
>         goto finish;
>     }

Nit: since the original code in this file uses tabs,
it would be nice to follow suit to make the diffs more
pleasant to review.

>+    pubk = PORT_ArenaZAlloc(arena, sizeof(SECKEYPublicKey));
>+    if (pubk == NULL) {
>+	PORT_SetError(SEC_ERROR_NO_MEMORY);
>+	PORT_FreeArena(arena, PR_TRUE);
>         goto finish;
>     }

Nit: PORT_ArenaZAlloc sets the error code already:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/secport.c&rev=1.29&mark=264,299#263

so you don't need to set it again here.

Do not free the arena here.  Free it under the 'finish'
label instead.

IMPORTANT (the reason for review-): the code under 'finish'
must not call PORT_Free(pubk).  I think it can be rewritten
as follows:

    if (rv != SECSuccess) {
        if (arena != NULL) {
            PORT_FreeArena(arena, PR_FALSE);
        }
        pubk = NULL;
    }

Comment 6

[Elio Maldonado]

Attachment: V2 - Allocate arena then the SECKEYPublicKey off of this arena

Comment 7

[Elio Maldonado]

Attachment: The version for the NSS_3_12_BRANCH

Comment 8

[Wan-Teh Chang]

Comment on attachment 517225 [details] [diff] [review]
V2 - Allocate arena then the SECKEYPublicKey off of this arena

r=wtc.

Comment 9

[Wan-Teh Chang]

Comment on attachment 517226 [details] [diff] [review]
The version for the NSS_3_12_BRANCH

r=wtc.  Bob, please doublecheck this patch.

>+    pubk = PORT_ArenaZAlloc(arena, sizeof(SECKEYPublicKey));

Consider using PORT_ArenaZNew(arena, SECKEYPublicKey) here.
(The original code uses PORT_ZNew(SECKEYPublicKey).)

Comment 10

[Robert Relyea]

Comment on attachment 517226 [details] [diff] [review]
The version for the NSS_3_12_BRANCH

r+ 

I also prefer WTC's suggestion of using the Arena_ZNew() macro. Feel free to make that change when you check in.

bob

Comment 11

[Elio Maldonado]

Patch checked in to trunk:
Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.60; previous revision: 1.59

Comment 12

[Elio Maldonado]

Attachment: Actual patch applied to trunk

Comment 13

[Elio Maldonado]

Patch checked in to NSS_3_12_BRANCH:
Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.54.2.2; previous revision: 1.54.2.1

Comment 14

[Elio Maldonado]

Attachment: Actual patch applied to branch