RFR 6913047: SunPKCS11 memory leak

Valerie Peng valerie.peng at oracle.com
Fri Aug 17 00:28:09 UTC 2018


Hi Martin,


Since there is no regression test for this test, you will need to add a 
"noreg-xxx" label to the bug record. For this issue, it'll probably be 
"noreg-hard".

To understand the changes better, please find my questions and some 
review comments below:

<src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java>
- line 397: It's incorrect to change initialize() to 
ensureInitialized(). If the cipher object were to be initialized twice 
with two different keys, you need to re-initialize again.
- line 471: Why do you change it to Throwable from PKCS11Exception?

<src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java>
- line 99: comment is somewhat unclear. typo: "temporal" should be 
"temporary".
- line 148-149: JDK-8165996 has been fixed. This does not apply now, 
does it?
- You changed the constructors of all the P11Key-related classes to take 
an additional boolean argument "tmpNativeKey". However, this boolean 
argument is only used when the underlying token is "NSS". Why limiting 
to NSS only? It seems that all callers except for PKCS11 KeyStore pass 
true for "tmpNativeKey". When "tmpNativeKey" is true, the keyID handle 
first destroyed in constructor and later created again (ref count == 1) 
and destroyed (when ref count drops to 0). This replaced the 
PhantomReference approach in SessionKeyRef class, right? Now both 
approaches seem to be used and key destruction is taking places at 
different methods. I think we should clarify the cleanup model and 
perhaps refactor the code so it's easier which approach is in use. Or 
grouping all these cleanup-related fields/variables into a separate 
class for readability/clarity.
- line 157-175: nativeKeyWrapperKeyID is a static variable, shouldn't it 
be synchronized on a class level object?
- line 1343: the impl doesn't look right. Why do you call both 
removeObject() and addObject() here with the same check?
- line 1363: the change seems incorrect, you should still call 
session.removeObject(). the call setKeyIDAndSession(0, null) does not 
lower the session object count.

Thanks,
Valerie

On 8/7/2018 5:41 PM, Valerie Peng wrote:
>
> Hi Martin,
>
> Thanks for the update, I will resume the review on this one with your 
> latest webrev.
>
> BTW, there is no webrev.07 for your other fix, i.e. JDK-8029661, 
> right? Just checking.
>
> Regards,
> Valerie
>
> On 8/3/2018 2:13 PM, Martin Balao wrote:
>> Hi Valerie,
>>
>>  * 
>> http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10/ 
>> <http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.10/>
>>  * 
>> http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10.zip 
>> <http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.10.zip>
>>
>> New in webrev 10:
>>
>>  * Bug fix when private DSA/EC keys are sensitive
>>   * I found this bug removing "attributes = compatibility" entry in 
>> NSS configuration file so keys were CKA_SENSITIVE.
>>   * This is really an NSS bug when unwrapping ephemeral keys 
>> (NSC_UnwrapKey function), because CKA_NETSCAPE_DB attribute is 
>> required but not used/needed. There was a similar bug when creating 
>> objects (NSC_CreateObject function), fixed many years ago [1].
>>   * In those cases in which the bug occurs (private keys, of DSA/EC 
>> type, sensitive and without CKA_NETSCAPE_DB attribute set), I store 
>> an empty CKA_NETSCAPE_DB attribute in the buffer that will later be 
>> used for key unwrapping. I'm not doing a C_SetAttributeValue call 
>> because keys are read-only. We can let users set CKA_NETSCAPE_DB 
>> attribute in NSS configuration file [2] but this would be a 
>> workaround on users side.
>>   * Changes in:
>>    * p11_keymgmt.c
>>     * L175-187, L212-214 and L275-278
>>
>>  * Bug fix when storing sensitive RSA keys in a keystore
>>   * CKA_NETSCAPE_DB attribute is not needed so we return it as null 
>> (instead of failing with an "Invalid algorithm" message)
>>   * Changes in:
>>    * P11KeyStore.java
>>     * L1909-1914
>>
>>  * Lines length was cut to improve code readability
>>
>> Regression tests on jdk/sun/security/pkcs11 category passed. I ran my 
>> internal test suite too, that covers the following services (with 
>> SunPKCS11 provider): Cipher, Signature, KeyAgreement, Digest, Mac, 
>> KeyGenerator, KeyPairGenerator and Keystore.
>>
>> Kind regards,
>> Martin.-
>>
>> --
>> [1] - https://bugzilla.mozilla.org/show_bug.cgi?id=309701#c6 
>> <https://bugzilla.mozilla.org/show_bug.cgi?id=309701#c6>
>> [2] - 
>> https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml 
>> <https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180816/d3b004dc/attachment.htm>


More information about the security-dev mailing list