RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

Thanks for the update. I will try the updated webrev out and see if the 
failed regression tests now pass.

Sorry that my cycles for reviewing your patches for JDK-6913047 and 
JDK-8029661 are somewhat limited due to current project priorities.

I will return to work on them as soon as I have time.

Regards,
Valerie

On 6/4/2018 3:39 PM, Martin Balao wrote:
> Hi Valerie,
>
> 4 bugs have been fixed in Webrev 09:
>
>  * 
> http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.09/ 
> <http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.09/>
>
>  * 
> http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.09.zip 
> <http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.09.zip>
>
> Bug #1
> ----------------------
>
> Bug happened when re-building sensitive private DSA keys. A wrapping 
> key -that lives in the token- is used for an encrypted extraction of 
> the private DSA key when a token is operated in FIPS mode. NSS + 
> legacy DB, because of internal storage constraints, uses a special key 
> attribute called CKA_NETSCAPE_DB to store the public key associated to 
> the private key. Previous to webrev.09, we were not reading and 
> encoding this attribute when extracting the key so it couldn't be 
> re-built. This problem could have potentially affected EC keys.
>
> Bug #2
> ----------------------
>
> There was a misshandling of object sessions reference counting, which 
> was leading to leaked open sessions and closure of in-use sessions.
>
> Suppose the following case:
>  * A key "A" was re-built in session M (P11Key.incNativeKeyRef). 
> Session M has a key object associated, and is returned to the pool.
>  * A session number N was obtained to perform a derivation of key "A"
>  * Before derivation finishes, session M was closed (session N 
> remained alive). Thus, sourceKey in "sourceKey = 
> sftk_ObjectFromHandle(hBaseKey, session)" is NULL and 
> CKR_KEY_HANDLE_INVALID returned. See here [1]. This was happening 
> because when re-building key "A", the session objects number was not 
> incremented with addObject method. Only the session for the original 
> key had its reference counter incremented -and was, in fact, leaked-.
>
> Sessions reference counting has been simplified. When a key is 
> destroyed (because key info is extracted out of the token), reference 
> counter is decremented. When a key is re-built in the token, reference 
> counter is incremented. This applies to both original and re-built keys.
>
> Bug #3
> ----------------------
>
> A session was released in P11Cipher at the end of the operation. 
> However, session instance variable was not updated to null. Thus, 
> session could be killed by any other object and when tried to be 
> reused with the same P11Cipher instance, an error occurred.
>
> Bug #4
> ----------------------
>
> A P11Digest session was killed without verifying if session had objects.
>
> In addition to fixing these bugs, I did some further refactoring to 
> align how P11Signature, P11Cipher, P11Mac and P11RSACipher classes 
> handle initialization and finalization.
>
> Kind regards,
> Martin.-
>
> --
> [1] - 
> https://github.com/nss-dev/nss/blob/a66859469920b7d619f3efab7ce993579c4085fd/lib/softoken/pkcs11c.c#L6502