RFR 6913047: SunPKCS11 memory leak

Sat Sep 1 18:18:11 UTC 2018

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/20180901/624dd190/attachment.htm>