RFR 6913047: SunPKCS11 memory leak

Tomas Gustavsson tomas at primekey.se
Wed Oct 10 05:12:03 UTC 2018 

Hi,

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

I just though I'd second this, albeit late. We're seing the current
PKCS#11 Provider model break down with some new HSMs out there. I.e. on
AWS CloudHSM/Cavium, it is only possible to set CKA_ID at time of key
creation, whereas the P11 Provider sets it afterwards as a setAttribute
call. This effectively makes the old Sun P11 Provider not usable at all
wiht CloudHSM/Cavium.
With todays PKCS#11 landscape I think we need more control, or the
provider will essentially be defunct.

Regards,
Tomas


On 2018-09-05 22:49, Michael StJohns wrote:
> 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>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the security-dev mailing list