RFR 6913047: SunPKCS11 memory leak

Wed Sep 5 01:59:40 UTC 2018

These sun.security.pkcs11.wrapper classes are internal and subject to 
changes without notice.

For the time being, maybe we can leave these method intact and add a 
comment about calling the new methods which use P11Key argument instead 
of the key handle argument.

Regards,
Valerie

On 9/1/2018 11:18 AM, Michael StJohns wrote:
> Hi Valerie -
>
> I may be speaking only for myself, but there are probably a number of 
> folk using sun.security.pkcs11.wrapper.* in lieu of the PKCS11 
> provider for a number of reasons including an ability to manage the 
> key storage and wrapping efficiently.   Looking at this I'm thinking 
> there will be a large number of things breaking because of the way you 
> refactored things.
>
> While I understand that none of the sun.security.* classes are 
> "public" - these were mostly taken directly from the IAIK contribution 
> way back when and the folks I worked with used them in lieu of 
> external libraries to have a consistent PKCS11 interface java-to-java.
>
> Perhaps instead you'd consider doing something like leaving the Java 
> to Native public methods intact?
>
> Mike
>
>
>
>
> On 8/31/2018 7:53 PM, Valerie Peng wrote:
>>
>> Hi Martin,
>>
>> With the new model of "creating the key handle as needed", I think we 
>> should not allow the direct access of keyID field outside of P11Key 
>> class. This field should be made private and accessed through 
>> methods. In addition, existing PKCS11.C_XXX() methods with P11 keyID 
>> arguments can be made private and we can add wrapper methods with 
>> P11Key object argument to ensure proper usage of the P11Key object 
>> and its keyID field.
>>
>> I have also refactored the keyID management code into a separate 
>> holder class. The prototype code can be found at: 
>> http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.00/
>>
>> The main changes that I made are in P11Key.java and PKCS11.java. The 
>> other java classes are updated to call the wrapper methods in 
>> PKCS11.java.
>>
>> Thanks & have a great Labor day weekend,
>>
>> Valerie
>>
>>
>> On 8/16/2018 5:28 PM, Valerie Peng wrote:
>>>
>>> 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/20180904/96b806c7/attachment.htm>