RFR 6913047: SunPKCS11 memory leak

Michael StJohns mstjohns at comcast.net
Wed Sep 5 20:49:02 UTC 2018 

On 9/4/2018 9:59 PM, Valerie Peng wrote:
> These sun.security.pkcs11.wrapper classes are internal and subject to 
> changes without notice.
No arguments there.  But that interface has been stable since the 
initial contribution and to be blunt - the PKCS11 provider only works 
well if you use the keys you created through the provider. There's a set 
of idiosyncratic choices for how to map keys to certs to keys that make 
keys created by non-provider calls (e.g. C code or other non-java libs) 
to the token difficult to use through the provider and vice versa.  That 
led to us using the wrapper classes directly.

Maybe its time to provide a PKCS11AttributeSpec  of some sort for key 
creation and for looking things up?  The current model is literally 
12-15 years old AFAICT.

>
> 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.

That should work.  You may want to think about deprecating those methods 
and target removing them for a later release in a couple of years.

Thanks - Mike


>
> 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/20180905/63e796ae/attachment.htm>

More information about the security-dev mailing list