585066 - Memory leak in PK11_PubWrapSymKey: imported public key is never destroyed

Status: NEW

(NSS :: Libraries, defect, P4)

Description

[bob.e.foss]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8
Build Identifier: NSS 3.12.6

A public key object is created with PK11_ImportPublicKey, but
it is never destroyed.

I found this with the help of Valgrind.

I unfortunately do not have a simple test case as it involves copying
public keys from an HSM to the NSS soft token.

Reproducible: Always

Comment 1

[bob.e.foss]

Attachment: Simple patch to destroy the imported public key

Comment 2

[Wan-Teh Chang]

bob.e.foss: thanks for the bug report.  (Your patch isn't correct
in general.)

Bob (Relyea), I have a question for you below.

This leak was also reported in Chromium bug 32170:
http://code.google.com/p/chromium/issues/detail?id=32170#c5
I reproduced the relevant info here for convenience.

valgrind reports this leak in NSS 3.12.3.1 when an DHE_RSA SSL
cipher suite is used:

==5542== 5,006 (4,174 direct, 832 indirect) bytes in 2 blocks are definitely lost in 
loss record 384 of 386
==5542==    at 0x4C248C4: malloc 
/tmp/vg/coregrind/m_replacemalloc/vg_replace_malloc.c:196
==5542==    by 0x761C2B3: PL_ArenaAllocate /build/buildd/nspr-
4.7.5/mozilla/nsprpub/lib/ds/plarena.c:229
==5542==    by 0x71E3AFF: PORT_ArenaAlloc_Util /tmp/nss.g21230/nss-
3.12.3.1/mozilla/security/nss/lib/util/secport.c:246
==5542==    by 0xC53161E: ???
==5542==    by 0xC53224A: ???
==5542==    by 0xC532DBE: ???
==5542==    by 0x6EE5B4F: PK11_CreateNewObject /tmp/nss.g21230/nss-
3.12.3.1/mozilla/security/nss/lib/pk11wrap/pk11obj.c:412
==5542==    by 0x6ECE4FE: PK11_ImportPublicKey /tmp/nss.g21230/nss-
3.12.3.1/mozilla/security/nss/lib/pk11wrap/pk11akey.c:236
==5542==    by 0x6EF08C2: PK11_PubWrapSymKey /tmp/nss.g21230/nss-
3.12.3.1/mozilla/security/nss/lib/pk11wrap/pk11skey.c:1160
==5542==    by 0x85692F: sendRSAClientKeyExchange 
/usr/local/google/chromium/src/net/third_party/nss/ssl/ssl3con.c:4515
==5542==    by 0x856DA2: ssl3_SendClientKeyExchange 
/usr/local/google/chromium/src/net/third_party/nss/ssl/ssl3con.c:4695
==5542==    by 0x858987: ssl3_HandleServerHelloDone 
/usr/local/google/chromium/src/net/third_party/nss/ssl/ssl3con.c:5617
==5542== 

The PK11_CreateNewObject call in PK11_ImportPublicKey is:

        rv = PK11_CreateNewObject(slot, CK_INVALID_SESSION, theTemplate,
                                        templateCount, isToken, &objectID);
        if (ckaId) {
            SECITEM_FreeItem(ckaId,PR_TRUE);
        }
        if (pubValue) {
            SECITEM_FreeItem(pubValue,PR_TRUE);
        }
        if ( rv != SECSuccess) {
            return CK_INVALID_HANDLE;
        }
    }

    pubKey->pkcs11ID = objectID;
    pubKey->pkcs11Slot = PK11_ReferenceSlot(slot);

    return objectID;
}

Note that the objectID and slot are stored in pubKey's pkcs11ID
and pkcs11Slot fields.

But SECKEY_DestroyPublicKey doesn't destroy the PKCS #11 object
if it is a permanent/token object:

void
SECKEY_DestroyPublicKey(SECKEYPublicKey *pubk)
{
    if (pubk) {
        if (pubk->pkcs11Slot) {
            if (!PK11_IsPermObject(pubk->pkcs11Slot,pubk->pkcs11ID)) {
                PK11_DestroyObject(pubk->pkcs11Slot,pubk->pkcs11ID);
            }
            PK11_FreeSlot(pubk->pkcs11Slot);
        }
        if (pubk->arena) {
            PORT_FreeArena(pubk->arena, PR_FALSE);
        }
    }
}

Bob added that !PK11_IsPermObject() test in rev. 1.17 of seckey.c.
Bob, do you remember why?  Your checkin comment simply says:
Bug 117978: accessor functions to all JCE keystore API to be implemented.

Here is the diff:

Index: seckey.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v
retrieving revision 1.16
retrieving revision 1.17
diff -p -u -6 -w -r1.16 -r1.17
--- seckey.c    10 Dec 2001 21:11:14 -0000      1.16
+++ seckey.c    3 Feb 2002 03:37:18 -0000       1.17
@@ -206,13 +206,15 @@ SECKEY_DestroyPrivateKey(SECKEYPrivateKe

 void
 SECKEY_DestroyPublicKey(SECKEYPublicKey *pubk)
 {
     if (pubk) {
        if (pubk->pkcs11Slot) {
+           if (!PK11_IsPermObject(pubk->pkcs11Slot,pubk->pkcs11ID)) {
            PK11_DestroyObject(pubk->pkcs11Slot,pubk->pkcs11ID);
+           }
            PK11_FreeSlot(pubk->pkcs11Slot);
        }
        if (pubk->arena) {
            PORT_FreeArena(pubk->arena, PR_FALSE);
        }
     }

Comment 3

[Robert Relyea]

> (Your patch isn't correct in general.)

Clearly the patch is wrong. You don't want to delete the public key simply because you used it to wrap an object.

> Bob added that !PK11_IsPermObject() test in rev. 1.17 of seckey.c.
> Bob, do you remember why?

If they are permanent keys, they aren't supposed to be destroy simply because you have freed the low level key structure. That is because they are permanent keys;).

If isToken is true, your are asking to create a permanent, in-token key. If that is not what you want, you should pass isToken as false.

If you want to delete a permanent key, you can use 'PK11_DeleteTokenPublicKey' to do this.

--------------------------------------------------------------------

I could wish for more information in the bug, but with what is in here, I'm guessing the code is trying to move a symetric key from the HSM to softoken. This means the ??? is actually in the HSM PKCS #11 module. I'm guessing that the HSM is allocating some memory used to access permanent keys and not freeing them on shutdown. I'm guessing that for 2 reasons:

1) If it were softoken, we should have had symbols for it..
 and
2) comment 1 says the copy is from the HSM to softoken. This means the wrap will happen in the HSM.

NOTE: that fact the HSM is calling NSPR is a bit confusing. There is nothing that prevents HSMs from using NSPR, It's just a bit surprising, thus the identification of the leak coming from the HSM is tentative.

If these are both the case, then I believe there are likely 2 bugs here: one in the HSM and one in the application:

Minor issue:

The HSM may not be properly freeing it's 'scratch' memory when it is shutdown. HSMs often need to keep memory around when it accesses permanent (token) objects to provide proper PKCS #11 semantics. This memory should be freed on shutdown, but it does not constitute a true memory leak for an application that holds NSS (and thus the HSM) open for it's lifetime (which is typical for an NSS application). It could also me the token isn't being shutdown because of a reference leak in the application, or the application never calling NSS_Shutdown. In this case, the leak is spurious.

Major issue:

The application is trying to use a permanent public key when it wanted to use a temporary one (istoken is false). I'm pretty sure this is the real issue, particularly if the code wtc pointed out. Unless what is being copied is a public key (which doesn't seem to be the case), there public key should not be a permanent key.

Hopefully this will prompt some more input that will help validate my assumptions.

bob

Comment 4

[bob.e.foss]

Sorry, this has been a really long time since I worked on this.

I believe the public key came from the HSM and is imported into the soft token.
The key was then used to wrap either a TLS session key or a pre-master secret
and then unwrap it in the HSM.

Comment 5

[bob.e.foss]

I found these valgrind notes in my records.
Line numbers are likely off as I have several patches.

I believe this code was being called because I was copying
the master secret from the soft token into the HSM to perform
the HMAC-SHA1 calculations.

==11605== 176 bytes in 4 blocks are possibly lost in loss record 209 of 287
==11605==    at 0x4005EB7: calloc (vg_replace_malloc.c:418)
==11605==    by 0x42079BC: PR_Calloc (prmem.c:474)
==11605==    by 0x41DE4EC: PORT_ZAlloc_Util (secport.c:139)
==11605==    by 0x41DE63F: PORT_NewArena_Util (secport.c:246)
==11605==    by 0x4641C90: sftk_GetPubKey (pkcs11.c:1568)
==11605==    by 0x4640A42: sftk_handlePublicKeyObject (pkcs11.c:947)
==11605==    by 0x4641554: sftk_handleKeyObject (pkcs11.c:1290)
==11605==    by 0x4641BBD: sftk_handleObject (pkcs11.c:1510)
==11605==    by 0x46460D5: NSC_CreateObject (pkcs11.c:3815)
==11605==    by 0x40D53D9: PK11_CreateNewObject (pk11obj.c:413)
==11605==    by 0x40B8A82: PK11_ImportPublicKey (pk11akey.c:236)
==11605==    by 0x40E24C5: PK11_PubWrapSymKey (pk11skey.c:1200)

==11605== 352 bytes in 4 blocks are possibly lost in loss record 223 of 287
==11605==    at 0x4005EB7: calloc (vg_replace_malloc.c:418)
==11605==    by 0x42079BC: PR_Calloc (prmem.c:474)
==11605==    by 0x4219426: PR_NewLock (ptsynch.c:174)
==11605==    by 0x41DE666: PORT_NewArena_Util (secport.c:251)
==11605==    by 0x4641C90: sftk_GetPubKey (pkcs11.c:1568)
==11605==    by 0x4640A42: sftk_handlePublicKeyObject (pkcs11.c:947)
==11605==    by 0x4641554: sftk_handleKeyObject (pkcs11.c:1290)
==11605==    by 0x4641BBD: sftk_handleObject (pkcs11.c:1510)
==11605==    by 0x46460D5: NSC_CreateObject (pkcs11.c:3815)
==11605==    by 0x40D53D9: PK11_CreateNewObject (pk11obj.c:413)
==11605==    by 0x40B8A82: PK11_ImportPublicKey (pk11akey.c:236)
==11605==    by 0x40E24C5: PK11_PubWrapSymKey (pk11skey.c:1200)
--
==11605== 352 bytes in 4 blocks are possibly lost in loss record 224 of 287
==11605==    at 0x4005EB7: calloc (vg_replace_malloc.c:418)
==11605==    by 0x42079BC: PR_Calloc (prmem.c:474)
==11605==    by 0x4219426: PR_NewLock (ptsynch.c:174)
==11605==    by 0x465596E: sftk_NewObject (pkcs11u.c:1032)
==11605==    by 0x4645F77: NSC_CreateObject (pkcs11.c:3778)
==11605==    by 0x40D53D9: PK11_CreateNewObject (pk11obj.c:413)
==11605==    by 0x40B8A82: PK11_ImportPublicKey (pk11akey.c:236)
==11605==    by 0x40E24C5: PK11_PubWrapSymKey (pk11skey.c:1200)
==11605==    by 0x40C456C: pk11_KeyExchange (pk11kea.c:163)
==11605==    by 0x40E1B03: pk11_CopyToSlotPerm (pk11skey.c:847)
==11605==    by 0x40E1B42: pk11_CopyToSlot (pk11skey.c:856)
--
==11605== 352 bytes in 4 blocks are possibly lost in loss record 225 of 287
==11605==    at 0x4005EB7: calloc (vg_replace_malloc.c:418)
==11605==    by 0x42079BC: PR_Calloc (prmem.c:474)
==11605==    by 0x4219426: PR_NewLock (ptsynch.c:174)
==11605==    by 0x46559A2: sftk_NewObject (pkcs11u.c:1037)
==11605==    by 0x4645F77: NSC_CreateObject (pkcs11.c:3778)
==11605==    by 0x40D53D9: PK11_CreateNewObject (pk11obj.c:413)
==11605==    by 0x40B8A82: PK11_ImportPublicKey (pk11akey.c:236)
==11605==    by 0x40E24C5: PK11_PubWrapSymKey (pk11skey.c:1200)
==11605==    by 0x40C456C: pk11_KeyExchange (pk11kea.c:163)
==11605==    by 0x40E1B03: pk11_CopyToSlotPerm (pk11skey.c:847)
==11605==    by 0x40E1B42: pk11_CopyToSlot (pk11skey.c:856)

==11605== 1,024 bytes in 4 blocks are possibly lost in loss record 248 of 287
==11605==    at 0x4004A16: malloc (vg_replace_malloc.c:195)
==11605==    by 0x420795E: PR_Malloc (prmem.c:467)
==11605==    by 0x41DE440: PORT_Alloc_Util (secport.c:112)
==11605==    by 0x4653EEA: sftk_NewAttribute (pkcs11u.c:92)
==11605==    by 0x465532F: sftk_AddAttributeType (pkcs11u.c:823)
==11605==    by 0x4645FEE: NSC_CreateObject (pkcs11.c:3787)
==11605==    by 0x40D53D9: PK11_CreateNewObject (pk11obj.c:413)
==11605==    by 0x40B8A82: PK11_ImportPublicKey (pk11akey.c:236)
==11605==    by 0x40E24C5: PK11_PubWrapSymKey (pk11skey.c:1200)
==11605==    by 0x40C456C: pk11_KeyExchange (pk11kea.c:163)
==11605==    by 0x40E1B03: pk11_CopyToSlotPerm (pk11skey.c:847)
==11605==    by 0x40E1B42: pk11_CopyToSlot (pk11skey.c:856)

Comment 6

[bob.e.foss]

I also have this patch which may or may not be related to this.


diff -rubN mozilla.orig/security/nss/lib/pk11wrap/pk11kea.c mozilla/security/nss/lib/pk11wrap/pk11kea.c
--- mozilla.orig/security/nss/lib/pk11wrap/pk11kea.c	2010-08-02 19:35:10.000000000 +0000
+++ mozilla/security/nss/lib/pk11wrap/pk11kea.c	2010-08-04 14:31:07.000000000 +0000
@@ -152,8 +172,28 @@
 	}
 rsa_failed:
 	if (wrapData.data != NULL) PORT_Free(wrapData.data);
-	if (privKey != NULL) SECKEY_DestroyPrivateKey(privKey);
-	if (pubKey != NULL) SECKEY_DestroyPublicKey(pubKey);
+        /* only destroy private key if we created it */
+        if (privKey != NULL) {
+          if (privKeyHandle == CK_INVALID_HANDLE) {
+            SECKEY_DestroyPrivateKey(privKey);
+          } else {
+            PK11_FreeSlot(privKey->pkcs11Slot);
+            if (privKey->arena) {
+              PORT_FreeArena(privKey->arena, PR_TRUE);
+            }
+          }
+        }
+        /* only destroy public key if we created it */
+        if (pubKey != NULL) {
+          if (pubKeyHandle == CK_INVALID_HANDLE) {
+            SECKEY_DestroyPublicKey(pubKey);
+          } else {
+            PK11_FreeSlot(pubKey->pkcs11Slot);
+            if (pubKey->arena) {
+              PORT_FreeArena(pubKey->arena, PR_FALSE);
+            }
+          }
+        }
 
 	return  newSymKey;
     
}

Comment 7

[Robert Relyea]

OK importing from the hsm into nss makes more sense that we are calling NSPR.

The above patch looks wrong. The public key side would definitely leak. The private key side looks like it's trying to avoid deleting the token's private rsa key. If this is happening, then I think we have another bug in SECKEY_DestroyPrivateKey(). For now try restoring the the public key portion of your patch and see if you still leak.

bob

Comment 8

[Wan-Teh Chang]

(In reply to comment #3)

Bob: thank you for looking at this bug and sorry about the
late reply.

Just to clarify -- the stack trace in my comment 2 was an
independent report of this memory leak by a Google Chrome
developer.  (Note: I made a mistake in my comment 2.  The
SSL key exchange algorithm is RSA, not DHE_RSA.)

So no hardware token was involved in the memory leak
reported in comment 2.  It's not clear to me why the
server's RSA public key became a token/permanent object
after the PK11_ImportPublicKey call.

Comment 9

[Nicholas Nethercote [inactive]]

Wan-teh, can you estimate how important this is?  Is the leak common and/or large?