Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

Thanks. I've moved the CSR to Finalized and will now wait for
approval. Hope it is approved soon so I can integrate to baseline.
I'll target JDK-13.

Kind regards,
Martin.-

On Tue, Jan 8, 2019 at 2:11 PM Valerie Peng  wrote:
>
> Hi Martin,
>
> The webrev.15 looks fine to me. The release note is mostly fine, I made
> some minor update to it.
>
> For the CSR, you should finalize it in order for it to be approved. The
> whole process should be explained in the pointer that I sent you
> earlier. I added myself to be the reviewer back in Dec already, so all
> you need to do now is to move the CSR to the "Finalized" state to signal
> that there is no more changes.
>
> In the mean time, which release are you targeting this to? You should
> update the "Fix Version" field of JDK-6913047 to keep various teams
> informed. Also, if this is for JDK 12, you need to watch out for the
> release schedule as RPD2 is fast approaching.
>
> After the CSR is approved, you can proceed with integration. However,
> given the extent of this change, please be really careful with potential
> code conflicts when merging. Double check everything to make sure you
> don't accidentally "undo" others' changes.
>
> Regards,
>
> Valerie
>
> On 1/4/2019 1:25 PM, Martin Balao wrote:
> > Hi Valerie,
> >
> > Is webrev.15 ready for approval? (CSR, patch)
> >
> > I've re-worked release notes a bit [0].
> >
> > Thanks,
> > Martin.-
> >
> > --
> > [0] - https://bugs.openjdk.java.net/browse/JDK-8215018
> >
> > On Fri, Dec 7, 2018 at 11:17 AM Martin Balao  wrote:
> >> Hi Valerie,
> >>
> >> On Thu, Dec 6, 2018 at 11:27 PM Valerie Peng 
> >> wrote:
> >>> I suppose the changes in this update is just the system property
> >>> renaming? I looked at the relevant files and they look fine. If you made
> >>> more changes than this, please let me know and I will take a closer look
> >>> at them.
> >> Yes, that's right. No changes other that those related to renaming and
> >> documenting the property as requested in the CSR.
> >>
> >>> Don't forget to add a release note subtask for JDK-6913047 as it has a
> >>> "release-note=yes" label.
> >> Will do.
> >>
> >>> I will re-run mach5 with this webrev.15 just to be safe.
> >> Thanks,
> >> Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

The webrev.15 looks fine to me. The release note is mostly fine, I made 
some minor update to it.


For the CSR, you should finalize it in order for it to be approved. The 
whole process should be explained in the pointer that I sent you 
earlier. I added myself to be the reviewer back in Dec already, so all 
you need to do now is to move the CSR to the "Finalized" state to signal 
that there is no more changes.


In the mean time, which release are you targeting this to? You should 
update the "Fix Version" field of JDK-6913047 to keep various teams 
informed. Also, if this is for JDK 12, you need to watch out for the 
release schedule as RPD2 is fast approaching.


After the CSR is approved, you can proceed with integration. However, 
given the extent of this change, please be really careful with potential 
code conflicts when merging. Double check everything to make sure you 
don't accidentally "undo" others' changes.


Regards,

Valerie

On 1/4/2019 1:25 PM, Martin Balao wrote:

Hi Valerie,

Is webrev.15 ready for approval? (CSR, patch)

I've re-worked release notes a bit [0].

Thanks,
Martin.-

--
[0] - https://bugs.openjdk.java.net/browse/JDK-8215018

On Fri, Dec 7, 2018 at 11:17 AM Martin Balao  wrote:

Hi Valerie,

On Thu, Dec 6, 2018 at 11:27 PM Valerie Peng 
wrote:

I suppose the changes in this update is just the system property
renaming? I looked at the relevant files and they look fine. If you made
more changes than this, please let me know and I will take a closer look
at them.

Yes, that's right. No changes other that those related to renaming and
documenting the property as requested in the CSR.


Don't forget to add a release note subtask for JDK-6913047 as it has a
"release-note=yes" label.

Will do.


I will re-run mach5 with this webrev.15 just to be safe.

Thanks,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

Is webrev.15 ready for approval? (CSR, patch)

I've re-worked release notes a bit [0].

Thanks,
Martin.-

--
[0] - https://bugs.openjdk.java.net/browse/JDK-8215018

On Fri, Dec 7, 2018 at 11:17 AM Martin Balao  wrote:
>
> Hi Valerie,
>
> On Thu, Dec 6, 2018 at 11:27 PM Valerie Peng 
> wrote:
> >
> > I suppose the changes in this update is just the system property
> > renaming? I looked at the relevant files and they look fine. If you made
> > more changes than this, please let me know and I will take a closer look
> > at them.
>
> Yes, that's right. No changes other that those related to renaming and
> documenting the property as requested in the CSR.
>
> >
> > Don't forget to add a release note subtask for JDK-6913047 as it has a
> > "release-note=yes" label.
>
> Will do.
>
> >
> > I will re-run mach5 with this webrev.15 just to be safe.
>
> Thanks,
> Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

On Thu, Dec 6, 2018 at 11:27 PM Valerie Peng 
wrote:
>
> I suppose the changes in this update is just the system property
> renaming? I looked at the relevant files and they look fine. If you made
> more changes than this, please let me know and I will take a closer look
> at them.

Yes, that's right. No changes other that those related to renaming and
documenting the property as requested in the CSR.

>
> Don't forget to add a release note subtask for JDK-6913047 as it has a
> "release-note=yes" label.

Will do.

>
> I will re-run mach5 with this webrev.15 just to be safe.

Thanks,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

I suppose the changes in this update is just the system property 
renaming? I looked at the relevant files and they look fine. If you made 
more changes than this, please let me know and I will take a closer look 
at them.


Don't forget to add a release note subtask for JDK-6913047 as it has a 
"release-note=yes" label.


I will re-run mach5 with this webrev.15 just to be safe.

Regards,
Valerie

On 11/29/2018 8:45 AM, Martin Balao wrote:


Hi Valerie,

Webrev.15

  * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.15/
  * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.15.zip

Property to disable this fix has been renamed to
"sun.security.pkcs11.disableKeyExtraction", as requested in the CSR [1].

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8213430

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

Webrev.15

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.15/
 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.15.zip

Property to disable this fix has been renamed to
"sun.security.pkcs11.disableKeyExtraction", as requested in the CSR [1].

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8213430

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

Thanks for having a look at this.

Here it's Webrev.14 with that fixed:

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.14/
 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.14.zip

I'll start the CSR process now.

Kind regards,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

I fixed all previously discussed issues in Webrev.13:

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.13/
 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.13.zip

In addition to that, I fixed a couple of bugs introduced in p11_keymgmt.c.

In Java_sun_security_pkcs11_wrapper_PKCS11_getNativeKeyInfo function, the
first call to C_GetAttributeValue (to get CKA_CLASS, CKA_KEY_TYPE,
CKA_SENSITIVE and CKA_NETSCAPE_DB attributes) may fail because the key may
not have a CKA_NETSCAPE_DB attribute. That is fine. It just means that we
are not going to get that attribute -which does not exist- and it won't be
needed for key unwrapping.

Later in Java_sun_security_pkcs11_wrapper_PKCS11_getNativeKeyInfo function,
a similar issue happened. The call to get buffer lengths may return an
error if one of the attributes does not exist. This is fine because length
values are obtained anyways and based on that we were not going to query
for non-existent attributes later.

These bugs were silently making all keys not to be extracted.

Thanks,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

Please find my reply below:

On 10/18/2018 9:26 AM, Martin Balao wrote:

Hi Valerie,

Thanks for your feedback.

I suggest to keep only one hierarchy of webrevs, so I'll continue in 
Webrev.12 using your Webrev.02 as a base.
Sure, I agree and prefer to use your hierarchy for the sake of complete 
history.


In general, I'm happy with your P11Key refactorings and with keeping 
the previous reference inc/dec scheme on P11key clients side.


I'll go file-by-file reviewing changes between your Webrev.02 and my 
Webrev.11:


 * P11Key.java
  * Why is P11Key now public?

Hmm, this change may not be intentional. Please discard it.

  * I have some concerns regarding how you calculate "extractKeyInfo" 
value:

   * Why is !sensitive a requirement?
    * We can extract them wrapped, and the code already handles that.
   * !tokenObject should be a requirement
    * I know it's checked in NativeKeyHolder constructor but it could 
be part of extractKeyInfo too for clarity

   * Why is !type.equals(SECRET) a requirement?
We can change the setting of extractKeyInfo to include !tokenObject and 
remove the !senstive and !type.equals(SECRET) I suppose. I used a 
stricter setting in my webrev.02 to avoid triggering the key wrapping 
functionality for ease of observation and verification. I didn't get to 
testing the prototype with key wrapping functionality before leaving for 
vacation, so I leave the stricter setting of extractKeyInfo in webrev.02 
in place.


  * In case of createNativeKey failure, I think we should decrement 
the previously-incremented reference counter

True, but since an exception is thrown, it may not matter much.

  * Why do we need refCount to be of AtomicInteger type? All the 
modifications to this instance variable are done inside instance 
synchronized methods (getKeyID and releaseKeyID)
This is done before I changed those methods to be synchronized. We don't 
need to use AtomicInteger now.


  * The benefit of the previous incNativeKeyRef/decNativeKeyRef 
methods is that they don't pay a sync cost for all cases in which the 
feature is disabled. I believe we can keep that performance 
improvement here too.
Sure, it's good to not pay sync cost when feature is disabled. I didn't 
get to polish my webrev.02 due to the time crunch. Just want to show you 
the model that I have in mind.


   * nativeKeyInfo variable is written at creation time and all other 
usages are read-only, so we can have multiple readers at the same time 
and synchronize on it if not null (instead of synchronizing the whole 
method). What do you think?

Sure, sounds good.


 * P11KeyStore.java
  * Copyright updated
  * The reason why a call to "makeNativeKeyPersistent" was made in 
P11KeyStore.updateP11Pkey method was because "P11Key key" could have 
been extracted and the change not persisted in the keystore. This call 
was disabling the feature for such keys. However, we now decided -for 
a good reason- that keys from keystores (token objects) will not be 
extracted. Thus, this call is no longer needed. I'm okay with this change.

  * "long newKeyID" variable not used. Fixed on some other files too.

Ok.


 * P11Mac.java
  * Copyright updated
  * Split ensureInitialized and initialize functions again for the 
reasons we've previously discussed.
Line 195: For engineInit(...) calls, we should always call initialize() 
instead of ensureInitialized(). Although ensureInitialized() here will 
always call initialize() due to the reset(true) call on line 192, but 
conceptually, it should be initialize().



 * P11SecretKeyFactory.java
  * Copyright updated
  * The inc/dec pattern was broken in case of "token.p11.C_CopyObject" 
failure. Fixed.

Good.


 * P11TlsKeyMaterialGenerator.java
  * Removed dead & debug code

Sure.



 * P11TlsPrfGenerator.java
  * Removed dead & debug code

Sure.


 * p11_keymgmt.c
  * Removed dead code
  * I've seen that you replaced some variable assignments with memcpy 
and memset calls because of compiler errors. I wonder what these 
errors are, as we should be able to cast and do assignments.
Some are due to compiler errors, some are due to SIGBUS errors when 
testing with your changes. Casting also makes long lines which may 
hinder readability. This may due to my machine being solaris sparc.


I agree with having a property to completely disable this feature. 
I've not seen, so far, unfixed cases in which extraction succeeds but 
re-creation fails.
Right, I think the chance of extraction succeeds but re-creation fails 
may be slim. The worse problem is that the re-creation succeeds but the 
re-created key is different from the original one. As this change does 
depend on the list of attributes, I wonder if the current list of 
attributes defined in the hardcoded template may become incomplete in 
the future (as newer algorithms are added or due to some other 
enhancements) and whether this may lead to the re-created keys being 
different. There could be other possible problems which we 

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

Thanks for your feedback.

I suggest to keep only one hierarchy of webrevs, so I'll continue in
Webrev.12 using your Webrev.02 as a base.

In general, I'm happy with your P11Key refactorings and with keeping the
previous reference inc/dec scheme on P11key clients side.

I'll go file-by-file reviewing changes between your Webrev.02 and my
Webrev.11:

 * P11Cipher.java
  * Ok
   * I used to set "initialized" variable to false in all "initialize"
function error flows but it's not necessary as it shouldn't be true when
entering.

 * P11DHKeyFactory.java
  * Copyright updated

 * P11DSAKeyFactory.java
  * Ok

 * P11Digest.java
  * Ok

 * P11ECDHKeyAgreement.java
  * Copyright updated

 * P11ECKeyFactory.java
  * Copyright updated

 * P11Key.java
  * Why is P11Key now public?
  * I have some concerns regarding how you calculate "extractKeyInfo" value:
   * Why is !sensitive a requirement?
* We can extract them wrapped, and the code already handles that.
   * !tokenObject should be a requirement
* I know it's checked in NativeKeyHolder constructor but it could be
part of extractKeyInfo too for clarity
   * Why is !type.equals(SECRET) a requirement?
  * It's correct that we can not destroy the key after creation (under the
assumption that will be used soon), and that this should be a performance
benefit for most cases. My approach here was more memory conservative, but
it should be fine anyways. I'm okay with this change.
  * In case of createNativeKey failure, I think we should decrement the
previously-incremented reference counter
  * Why do we need refCount to be of AtomicInteger type? All the
modifications to this instance variable are done inside instance
synchronized methods (getKeyID and releaseKeyID)
  * The benefit of the previous incNativeKeyRef/decNativeKeyRef methods is
that they don't pay a sync cost for all cases in which the feature is
disabled. I believe we can keep that performance improvement here too.
   * nativeKeyInfo variable is written at creation time and all other
usages are read-only, so we can have multiple readers at the same time and
synchronize on it if not null (instead of synchronizing the whole method).
What do you think?

 * P11KeyAgreement.java
  * Ok

 * P11KeyGenerator.java
  * Ok

 * P11KeyPairGenerator.java
  * Ok

 * P11KeyStore.java
  * Copyright updated
  * The reason why a call to "makeNativeKeyPersistent" was made in
P11KeyStore.updateP11Pkey method was because "P11Key key" could have been
extracted and the change not persisted in the keystore. This call was
disabling the feature for such keys. However, we now decided -for a good
reason- that keys from keystores (token objects) will not be extracted.
Thus, this call is no longer needed. I'm okay with this change.
  * "long newKeyID" variable not used. Fixed on some other files too.

 * P11Mac.java
  * Copyright updated
  * Split ensureInitialized and initialize functions again for the reasons
we've previously discussed.

 * P11RSACipher.java
  * Ok

 * P11RSAKeyFactory.java
  * Ok

 * P11SecretKeyFactory.java
  * Copyright updated
  * The inc/dec pattern was broken in case of "token.p11.C_CopyObject"
failure. Fixed.

 * P11Signature.java
  * Ok

 * P11TlsKeyMaterialGenerator.java
  * Removed dead & debug code

 * P11TlsMasterSecretGenerator.java
  * Ok

 * P11TlsPrfGenerator.java
  * Removed dead & debug code

 * P11TlsRsaPremasterSecretGenerator.java
  * Ok

 * PKCS11.java
  * Copyright updated

 * p11_keymgmt.c
  * Removed dead code
  * I've seen that you replaced some variable assignments with memcpy and
memset calls because of compiler errors. I wonder what these errors are, as
we should be able to cast and do assignments.

 * pkcs11t.h
  * Ok

 * pkcs11wrapper.h
  * Ok

In addition, I removed trimming white spaces in several files.

I agree with having a property to completely disable this feature. I've not
seen, so far, unfixed cases in which extraction succeeds but re-creation
fails.

Webrev.12 is based on your Webrev.02, with modifications previously
described:

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12/
 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12.zip

NOTE: Webrev.12 does not address neither the P11Key.java issues above nor
the property to completely disable this feature. Those 2 things are the
only pending on my side for now.

Kind regards,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Tomas Gustavsson]

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:
>
> 
> - 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?
>
> 
> - 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 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

I found the problem causing the one regression test failure and fixed 
it. Now Mach5 run is clean.


http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.02

I also made various changes hoping to improve things. You can compare 
the files in above webrev with yours for differences. General principal 
is to minimize the changes as all new code may introduce regressions 
especially with changes of this scale.
One key difference is that in your code, you destroy the native key 
handle after extracting native key info in the key constructor, and then 
re-creating the key handle when increase the reference count. I use a 
different approach, I keep the key handle in the key constructor which 
will then be destroyed when the reference count goes down to 0. From the 
regression test output that I observed, most keys are created and used 
once. Keeping the keyID in constructor seems more efficient. Besides, I 
also disable native key info extraction for all token keys.


One thing that I am debating is whether we should add some property to 
disable this. I am aware that this will only be enabled if the key info 
extraction succeeds. However, would there be cases where the extraction 
succeeds but the re-creation fails? P11 Key objects are quite widely 
used, if something goes wrong, the impact may be significant.


Thanks,
Valerie

On 10/1/2018 6:48 PM, Valerie Peng wrote:


Hi Martin,

For the KeyStore case, they are mostly token objects which the extract 
key info approach does not apply?


For your changes in p11_keymgmt.c, I ran into compiler error and 
SIGBUS errors on two OS (mac and solaris sparc), I ended up changing 
variable initializations as well as memset(...). With the updated 
native changes, I adapted and re-tested my prototype changes. For 
reference, you can find the updated prototype changes at: 
http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.01/


Besides making changes in the keymgmet.c for getting rid of 
platform-specific compilation error and SIGBUS error, I noticed that 
you hardcoded the key wrapping mechanism in native code for both 
getNativeKeyInfo(...)/createNativeKey() methods, it seems better to 
storing the mechanism object at java side, i.e. P11Key and its 
associated classes, and then pass the object to JNI code (please also 
see my webrev.01)


In addition, I switched the reference counting to your model, i.e. 
increase in init() and decrease in reset(), instead of the 
try-n-finally model in prototype webrev.00. My earlier comment on 
P11Cipher class which you should not replace the initialize() call 
with ensureInitialized() call applies to all other PKCS11 classes as well.


With this approach, the KeyID field of P11Key should not be freely 
accessible and directly referenced outside of P11Key class. Also, the 
increase and decrease of reference counting must be paired up. 
Supposedly, the reference count should not go negative, right? If the 
reference counting isn't correct, the key may be freed pre-maturely? 
Lastly, the reference counting is an implementation detail and I think 
it's better to keep it inside the P11Key class/file and not exposing 
it, i.e. through method names.


I have spent time verifying my updated prototype changes and trace the 
reference counting. All look fine, except there is one regression test 
failure (sun/security/tools/keytool/NssTest.java) on linux-x64 which I 
am still troubleshooting. However, I will be on vacation from 10/4 to 
10/21, so I want to update you on what I have so you can continue 
during my vacation.


Thanks,

Valerie



On 9/18/2018 4:48 PM, Valerie Peng wrote:


Hi Martin,

I am ok with your conservation choice of only applying this when 
using NSS. If we are only applying this for NSS, we should really 
refactor the code to minimize the impact on callers and P11Key class. 
My prototype code may be on the extreme end of minimizing changes. 
But the current webrev can use some refactoring also. With your 
explanation, I now understand your model better. How about the 
refactoring in P11Key class? Is there a reason for not doing this? I 
did test my prototype code against existing regression tests (except 
the KeyStore ones as more API changes are needed for persistent keys 
which I have not covered in prototype) but I ran into some strange 
errors in some native p11 calls which I did not touch so I commented 
them out and just checked the part of reference count, etc.


I will take a closer look at the KeyStore case and let you know.
Thanks,
Valerie

On 9/18/2018 7:29 AM, Martin Balao wrote:

Hi Valerie,

Thanks for your comments.

Here it is Webrev.11:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11/ 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip 




L397: That's right. I was trying to simplify 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

For the KeyStore case, they are mostly token objects which the extract 
key info approach does not apply?


For your changes in p11_keymgmt.c, I ran into compiler error and SIGBUS 
errors on two OS (mac and solaris sparc), I ended up changing variable 
initializations as well as memset(...). With the updated native changes, 
I adapted and re-tested my prototype changes. For reference, you can 
find the updated prototype changes at: 
http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.01/


Besides making changes in the keymgmet.c for getting rid of 
platform-specific compilation error and SIGBUS error, I noticed that you 
hardcoded the key wrapping mechanism in native code for both 
getNativeKeyInfo(...)/createNativeKey() methods, it seems better to 
storing the mechanism object at java side, i.e. P11Key and its 
associated classes, and then pass the object to JNI code (please also 
see my webrev.01)


In addition, I switched the reference counting to your model, i.e. 
increase in init() and decrease in reset(), instead of the try-n-finally 
model in prototype webrev.00. My earlier comment on P11Cipher class 
which you should not replace the initialize() call with 
ensureInitialized() call applies to all other PKCS11 classes as well.


With this approach, the KeyID field of P11Key should not be freely 
accessible and directly referenced outside of P11Key class. Also, the 
increase and decrease of reference counting must be paired up. 
Supposedly, the reference count should not go negative, right? If the 
reference counting isn't correct, the key may be freed pre-maturely? 
Lastly, the reference counting is an implementation detail and I think 
it's better to keep it inside the P11Key class/file and not exposing it, 
i.e. through method names.


I have spent time verifying my updated prototype changes and trace the 
reference counting. All look fine, except there is one regression test 
failure (sun/security/tools/keytool/NssTest.java) on linux-x64 which I 
am still troubleshooting. However, I will be on vacation from 10/4 to 
10/21, so I want to update you on what I have so you can continue during 
my vacation.


Thanks,

Valerie



On 9/18/2018 4:48 PM, Valerie Peng wrote:


Hi Martin,

I am ok with your conservation choice of only applying this when using 
NSS. If we are only applying this for NSS, we should really refactor 
the code to minimize the impact on callers and P11Key class. My 
prototype code may be on the extreme end of minimizing changes. But 
the current webrev can use some refactoring also. With your 
explanation, I now understand your model better. How about the 
refactoring in P11Key class? Is there a reason for not doing this? I 
did test my prototype code against existing regression tests (except 
the KeyStore ones as more API changes are needed for persistent keys 
which I have not covered in prototype) but I ran into some strange 
errors in some native p11 calls which I did not touch so I commented 
them out and just checked the part of reference count, etc.


I will take a closer look at the KeyStore case and let you know.
Thanks,
Valerie

On 9/18/2018 7:29 AM, Martin Balao wrote:

Hi Valerie,

Thanks for your comments.

Here it is Webrev.11:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11/ 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip 




L397: That's right. I was trying to simplify the code but missed 
this. Thanks.
L471: The key reference counter has to be decremented under any 
exception (P11Key.decNativeKeyRef method call). But, yes, no 
exception different than PKCS11Exception should be thrown. Reverted 
this change.



L99: Comment changed. It should be better now.
L148-L149: In fact, I'd enforce this and disable the feature for all 
token keys. Token keys are permanent and extracting them is risky. 
This criteria was already applied when dealing with key stores 
(P11Keystore class).


Yes, this feature is enabled for NSS only because it's the only 
backend we currently know that is affected by this memory "leak" 
issue. If there were any other software-token backend affected, we 
can try this feature there too. HSMs shouldn't have any problem. I 
prefer to take a more conservative approach and enable the feature 
only in those cases in which it's really necessary. All other cases, 
default to the previous mechanism for freeing memory.


This does not replace the PhantomReference approach; both work 
together and are complementary. In cases where temporary keys feature 
is disabled or when a temporary key client is not behaving correctly 
(i.e.: leaking stateful operations like "cipher" or "signature" in an 
intermediate state with the native key initialized), PhantomReference 
approach will be the last chance to free memory. The native key 
object can 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

I am ok with your conservation choice of only applying this when using 
NSS. If we are only applying this for NSS, we should really refactor the 
code to minimize the impact on callers and P11Key class. My prototype 
code may be on the extreme end of minimizing changes. But the current 
webrev can use some refactoring also. With your explanation, I now 
understand your model better. How about the refactoring in P11Key class? 
Is there a reason for not doing this? I did test my prototype code 
against existing regression tests (except the KeyStore ones as more API 
changes are needed for persistent keys which I have not covered in 
prototype) but I ran into some strange errors in some native p11 calls 
which I did not touch so I commented them out and just checked the part 
of reference count, etc.


I will take a closer look at the KeyStore case and let you know.
Thanks,
Valerie

On 9/18/2018 7:29 AM, Martin Balao wrote:

Hi Valerie,

Thanks for your comments.

Here it is Webrev.11:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11/ 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip 




L397: That's right. I was trying to simplify the code but missed this. 
Thanks.
L471: The key reference counter has to be decremented under any 
exception (P11Key.decNativeKeyRef method call). But, yes, no exception 
different than PKCS11Exception should be thrown. Reverted this change.



L99: Comment changed. It should be better now.
L148-L149: In fact, I'd enforce this and disable the feature for all 
token keys. Token keys are permanent and extracting them is risky. 
This criteria was already applied when dealing with key stores 
(P11Keystore class).


Yes, this feature is enabled for NSS only because it's the only 
backend we currently know that is affected by this memory "leak" 
issue. If there were any other software-token backend affected, we can 
try this feature there too. HSMs shouldn't have any problem. I prefer 
to take a more conservative approach and enable the feature only in 
those cases in which it's really necessary. All other cases, default 
to the previous mechanism for freeing memory.


This does not replace the PhantomReference approach; both work 
together and are complementary. In cases where temporary keys feature 
is disabled or when a temporary key client is not behaving correctly 
(i.e.: leaking stateful operations like "cipher" or "signature" in an 
intermediate state with the native key initialized), PhantomReference 
approach will be the last chance to free memory. The native key object 
can be destroyed (C_DestroyObject call) either from the 
PhantomReference mechanism or from the temporary keys mechanism. There 
shouldn't be any conflict between them. If it's destroyed through 
temporary keys mechanism, then we know that the P11Key object is alive 
(refereced) and thus PhantomReference destruction won't be taking 
place at the same time. Once the key is deleted, keyID is set to 0 and 
session to null. Thus, PhantomReference destruction won't have any 
effect when executed later. If we think of the other case (when the 
key is freed by PhantomReference), we have a P11Key object with a 
native key initialized but with no references to it. Thus, 
destroyNativeKey method won't be called and 
SessionKeyRef.disposeNative is the only method that will delete the key.


L157: that's right, synchronization has to be at class level. Fixed.

L1343: It's not the same session: this.session was assigned a new 
value (this.session = session;) before calling addObject.


L1363: removeObject is called for the session, inside 
setKeyIDAndSession: "this.session.removeObject();". Null is set to 
this.session instance variable after this call.


In regards to the refactorings you proposed, the problem I see with 
moving key reference incrementing/decrementing to PKCS11.java is that 
some operations are stateful. I.e.: encryption. When we initialize the 
operation with C_EncryptInit, the key id is the 3rd parameter. 
Destroying the key id and then doing C_EncryptUpdate sounds incorrect 
to me. Have you tried the regression testing suite after this 
refactoring? (I see some parts commented). In regards to removing the 
tmpNativeKey parameter (used to explicitly disable the feature for new 
P11Key objects), how do you handle the P11KeyStore case? We don't want 
temporary keys there.


Kind regards,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

Thanks for your comments.

Here it is Webrev.11:

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11/
 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip


L397: That's right. I was trying to simplify the code but missed this.
Thanks.
L471: The key reference counter has to be decremented under any exception
(P11Key.decNativeKeyRef method call). But, yes, no exception different than
PKCS11Exception should be thrown. Reverted this change.


L99: Comment changed. It should be better now.
L148-L149: In fact, I'd enforce this and disable the feature for all token
keys. Token keys are permanent and extracting them is risky. This criteria
was already applied when dealing with key stores (P11Keystore class).

Yes, this feature is enabled for NSS only because it's the only backend we
currently know that is affected by this memory "leak" issue. If there were
any other software-token backend affected, we can try this feature there
too. HSMs shouldn't have any problem. I prefer to take a more conservative
approach and enable the feature only in those cases in which it's really
necessary. All other cases, default to the previous mechanism for freeing
memory.

This does not replace the PhantomReference approach; both work together and
are complementary. In cases where temporary keys feature is disabled or
when a temporary key client is not behaving correctly (i.e.: leaking
stateful operations like "cipher" or "signature" in an intermediate state
with the native key initialized), PhantomReference approach will be the
last chance to free memory. The native key object can be destroyed
(C_DestroyObject call) either from the PhantomReference mechanism or from
the temporary keys mechanism. There shouldn't be any conflict between them.
If it's destroyed through temporary keys mechanism, then we know that the
P11Key object is alive (refereced) and thus PhantomReference destruction
won't be taking place at the same time. Once the key is deleted, keyID is
set to 0 and session to null. Thus, PhantomReference destruction won't have
any effect when executed later. If we think of the other case (when the key
is freed by PhantomReference), we have a P11Key object with a native key
initialized but with no references to it. Thus, destroyNativeKey method
won't be called and SessionKeyRef.disposeNative is the only method that
will delete the key.

L157: that's right, synchronization has to be at class level. Fixed.

L1343: It's not the same session: this.session was assigned a new value
(this.session = session;) before calling addObject.

L1363: removeObject is called for the session, inside setKeyIDAndSession:
"this.session.removeObject();". Null is set to this.session instance
variable after this call.

In regards to the refactorings you proposed, the problem I see with moving
key reference incrementing/decrementing to PKCS11.java is that some
operations are stateful. I.e.: encryption. When we initialize the operation
with C_EncryptInit, the key id is the 3rd parameter. Destroying the key id
and then doing C_EncryptUpdate sounds incorrect to me. Have you tried the
regression testing suite after this refactoring? (I see some parts
commented). In regards to removing the tmpNativeKey parameter (used to
explicitly disable the feature for new P11Key objects), how do you handle
the P11KeyStore case? We don't want temporary keys there.

Kind regards,
Martin.-

Re: RFR 6913047: SunPKCS11 memory leak

[Michael StJohns]

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:



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


- 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 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

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:



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


- 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/~mbalao/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 

Re: RFR 6913047: SunPKCS11 memory leak

[Michael StJohns]

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:



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


- 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/~mbalao/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 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

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:



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


- 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/~mbalao/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] - 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

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:



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


- 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/~mbalao/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 

[2] - 
https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml 

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

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/~mbalao/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 

[2] - 
https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml 

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10/
 * http://cr.openjdk.java.net/~mbalao/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
[2] - https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/
test/sun/security/pkcs11/fips/fips.cfg.shtml

Re: RFR 6913047: SunPKCS11 memory leak

[Valerie Peng]

Hi Martin,

Thanks for the update. I will try the updated webrev out and see if the 
failed regression tests now pass.


Sorry that my cycles for reviewing your patches for JDK-6913047 and 
JDK-8029661 are somewhat limited due to current project priorities.


I will return to work on them as soon as I have time.

Regards,
Valerie

On 6/4/2018 3:39 PM, Martin Balao wrote:

Hi Valerie,

4 bugs have been fixed in Webrev 09:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.09/ 



 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.09.zip 



Bug #1
--

Bug happened when re-building sensitive private DSA keys. A wrapping 
key -that lives in the token- is used for an encrypted extraction of 
the private DSA key when a token is operated in FIPS mode. NSS + 
legacy DB, because of internal storage constraints, uses a special key 
attribute called CKA_NETSCAPE_DB to store the public key associated to 
the private key. Previous to webrev.09, we were not reading and 
encoding this attribute when extracting the key so it couldn't be 
re-built. This problem could have potentially affected EC keys.


Bug #2
--

There was a misshandling of object sessions reference counting, which 
was leading to leaked open sessions and closure of in-use sessions.


Suppose the following case:
 * A key "A" was re-built in session M (P11Key.incNativeKeyRef). 
Session M has a key object associated, and is returned to the pool.

 * A session number N was obtained to perform a derivation of key "A"
 * Before derivation finishes, session M was closed (session N 
remained alive). Thus, sourceKey in "sourceKey = 
sftk_ObjectFromHandle(hBaseKey, session)" is NULL and 
CKR_KEY_HANDLE_INVALID returned. See here [1]. This was happening 
because when re-building key "A", the session objects number was not 
incremented with addObject method. Only the session for the original 
key had its reference counter incremented -and was, in fact, leaked-.


Sessions reference counting has been simplified. When a key is 
destroyed (because key info is extracted out of the token), reference 
counter is decremented. When a key is re-built in the token, reference 
counter is incremented. This applies to both original and re-built keys.


Bug #3
--

A session was released in P11Cipher at the end of the operation. 
However, session instance variable was not updated to null. Thus, 
session could be killed by any other object and when tried to be 
reused with the same P11Cipher instance, an error occurred.


Bug #4
--

A P11Digest session was killed without verifying if session had objects.

In addition to fixing these bugs, I did some further refactoring to 
align how P11Signature, P11Cipher, P11Mac and P11RSACipher classes 
handle initialization and finalization.


Kind regards,
Martin.-

--
[1] - 
https://github.com/nss-dev/nss/blob/a66859469920b7d619f3efab7ce993579c4085fd/lib/softoken/pkcs11c.c#L6502

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi Valerie,

4 bugs have been fixed in Webrev 09:

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.09/

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.09.zip

Bug #1
--

Bug happened when re-building sensitive private DSA keys. A wrapping key
-that lives in the token- is used for an encrypted extraction of the
private DSA key when a token is operated in FIPS mode. NSS + legacy DB,
because of internal storage constraints, uses a special key attribute
called CKA_NETSCAPE_DB to store the public key associated to the private
key. Previous to webrev.09, we were not reading and encoding this attribute
when extracting the key so it couldn't be re-built. This problem could have
potentially affected EC keys.

Bug #2
--

There was a misshandling of object sessions reference counting, which was
leading to leaked open sessions and closure of in-use sessions.

Suppose the following case:
 * A key "A" was re-built in session M (P11Key.incNativeKeyRef). Session M
has a key object associated, and is returned to the pool.
 * A session number N was obtained to perform a derivation of key "A"
 * Before derivation finishes, session M was closed (session N remained
alive). Thus, sourceKey in "sourceKey = sftk_ObjectFromHandle(hBaseKey,
session)" is NULL and CKR_KEY_HANDLE_INVALID returned. See here [1]. This
was happening because when re-building key "A", the session objects number
was not incremented with addObject method. Only the session for the
original key had its reference counter incremented -and was, in fact,
leaked-.

Sessions reference counting has been simplified. When a key is destroyed
(because key info is extracted out of the token), reference counter is
decremented. When a key is re-built in the token, reference counter is
incremented. This applies to both original and re-built keys.

Bug #3
--

A session was released in P11Cipher at the end of the operation. However,
session instance variable was not updated to null. Thus, session could be
killed by any other object and when tried to be reused with the same
P11Cipher instance, an error occurred.

Bug #4
--

A P11Digest session was killed without verifying if session had objects.

In addition to fixing these bugs, I did some further refactoring to align
how P11Signature, P11Cipher, P11Mac and P11RSACipher classes handle
initialization and finalization.

Kind regards,
Martin.-

--
[1] -
https://github.com/nss-dev/nss/blob/a66859469920b7d619f3efab7ce993579c4085fd/lib/softoken/pkcs11c.c#L6502

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Webrev 04 uploaded to cr.openjdk.java.net:

 * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-
6913047/webrev.04/ (browse online)
 * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-
6913047/webrev.04/6913047.webrev.04.zip (download)

On Wed, Oct 11, 2017 at 10:31 AM, Martin Balao  wrote:

> Hi,
>
> I'd like to propose a fix for bug JDK-6913047: "Long term memory leak when
> using PKCS11 and JCE exceeds 32 bit process address space" [1]. This fix
> does not contain changes in the GC and is SunPKCS11 internal only.
>
> PROBLEM
> 
>
> When using the SunPKCS11 crypto provider (for cipher, signature, mac, key
> generation or any other operation), multiple key objects may be created.
> I.e.: every time a TLS session is established, a unique master key (derived
> from the pre-master-key) has to be created and then used for encryption and
> decryption operations. This is a legitimate use case in which key caching
> does not make sense as each key is unique per session. These keys are of
> P11Key type and have a corresponding native key object created. In the case
> of NSS SunPKCS11 backend (PKCS11 software token), this native key object is
> temporarily stored in the process native heap. The interface is simple: a
> JNI call is done to create a native key object (C_CreateObject,
> C_CopyObject, C_DeriveKey, C_GenerateKeys, etc., according to the PKCS11
> interface) and an integer handler is kept in the Java side (P11Key). When
> the P11Key object is destroyed, a finalizer code is executed to free the
> native key object (through C_DestroyObject JNI call). The problem is that
> finalizer code execution happens only if the JVM garbage-collector cleans
> up the P11Key object. That may be delayed or not done at all, depending on
> different GC algorithms, parameters and environment conditions. As a
> result, the native heap may be exhausted with not freed native key objects,
> and the JVM will then crash -this is particularly true for 32 bits VMs
> where the virtual address space can be exhausted-.
>
>
> SCOPE
> 
>
> The fix is proposed for SunPKCS11 with NSS backend only. Other PKCS11
> backends are not currently under scope. It's likely that hardware PKCS11
> backends store native key objects in their own memory, preventing a native
> heap exhaustion and a JVM crash. However, it might be possible to cause an
> exhaustion on their own memory blocking key objects creation at some point.
> In any case, this is speculative as no tests were done on our side with
> real hardware.
>
>
> SOLUTION
> 
>
> Assuming that native keys are extractable, the idea is to hold native key
> data in the Java heap while keys are not in use. When a P11Key is created,
> every CK_ATTRIBUTE (PKCS11) value for the native key is queried, data
> stored in an opaque Java byte[] (inside the P11Key object) and native key
> destroyed. Every time the P11Key is about to be used, the native key is
> created with the stored data. After usage, the native key is again
> destroyed. Thus, it's not necessary to wait for a finalizer execution to
> cleanup native resources: cleanup is done at deterministic and
> previously-known points. This comes with a resposibility for key users
> -which are all SunPKCS11 internal services like P11Signature, P11Cipher,
> P11KeyGenerator, etc.-: create and destroy native keys through a reference
> counting scheme exposed by P11Key class. There are two kind of usages:
>
>  1) stateless: the native key is "atomically" created, used and destroyed.
> I.e.: MAC calculation, getEncodedInternal operation (on P11Key objects),
> signature operations, TLS key derivation, etc.
>
>  2) statefull: the native key is created, one or multiple intermediate
> actions are performed by the key user, a final action is performed and
> finally the native key is destroyed. I.e.: cipher operations.
>
> For keys that are extractable but sensitive (CKA_SENSITIVE attribute is
> true), as the case when operating in FIPS mode, wrapping/unwrapping is used
> as a workaround to extract session keys. Wrapper key is global and lives
> forever.
>
> There are no interface changes for SunPKCS11 external users.
>
> If keys are not extractable or the feature cannot be enabled for any other
> reason, the previous finalizer scheme is used as a fallback.
>
>
> ADDITIONAL IMPLEMENTATION NOTES
> 
>
> When a P11Key is created, a constructor parameter exists to indicate if
> the feature is enabled for that specific key. For this feature to be
> enabled, 2 additional conditions apply: 1) SunPKCS11 backend has to be NSS,
> and 2) key has to be extractable. If the feature is not enabled for a key,
> behavior is as previous to this patch (native key destruction relies on
> finalizer execution).
>
> The only P11Key 

Re: RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

SCOPE clarification:

 * The proposed patch should be solving the issue in HSMs also. Real
hardware testing would be needed to check that, but it should be working.

On Wed, Oct 11, 2017 at 10:31 AM, Martin Balao  wrote:

> Hi,
>
> I'd like to propose a fix for bug JDK-6913047: "Long term memory leak when
> using PKCS11 and JCE exceeds 32 bit process address space" [1]. This fix
> does not contain changes in the GC and is SunPKCS11 internal only.
>
> PROBLEM
> 
>
> When using the SunPKCS11 crypto provider (for cipher, signature, mac, key
> generation or any other operation), multiple key objects may be created.
> I.e.: every time a TLS session is established, a unique master key (derived
> from the pre-master-key) has to be created and then used for encryption and
> decryption operations. This is a legitimate use case in which key caching
> does not make sense as each key is unique per session. These keys are of
> P11Key type and have a corresponding native key object created. In the case
> of NSS SunPKCS11 backend (PKCS11 software token), this native key object is
> temporarily stored in the process native heap. The interface is simple: a
> JNI call is done to create a native key object (C_CreateObject,
> C_CopyObject, C_DeriveKey, C_GenerateKeys, etc., according to the PKCS11
> interface) and an integer handler is kept in the Java side (P11Key). When
> the P11Key object is destroyed, a finalizer code is executed to free the
> native key object (through C_DestroyObject JNI call). The problem is that
> finalizer code execution happens only if the JVM garbage-collector cleans
> up the P11Key object. That may be delayed or not done at all, depending on
> different GC algorithms, parameters and environment conditions. As a
> result, the native heap may be exhausted with not freed native key objects,
> and the JVM will then crash -this is particularly true for 32 bits VMs
> where the virtual address space can be exhausted-.
>
>
> SCOPE
> 
>
> The fix is proposed for SunPKCS11 with NSS backend only. Other PKCS11
> backends are not currently under scope. It's likely that hardware PKCS11
> backends store native key objects in their own memory, preventing a native
> heap exhaustion and a JVM crash. However, it might be possible to cause an
> exhaustion on their own memory blocking key objects creation at some point.
> In any case, this is speculative as no tests were done on our side with
> real hardware.
>
>
> SOLUTION
> 
>
> Assuming that native keys are extractable, the idea is to hold native key
> data in the Java heap while keys are not in use. When a P11Key is created,
> every CK_ATTRIBUTE (PKCS11) value for the native key is queried, data
> stored in an opaque Java byte[] (inside the P11Key object) and native key
> destroyed. Every time the P11Key is about to be used, the native key is
> created with the stored data. After usage, the native key is again
> destroyed. Thus, it's not necessary to wait for a finalizer execution to
> cleanup native resources: cleanup is done at deterministic and
> previously-known points. This comes with a resposibility for key users
> -which are all SunPKCS11 internal services like P11Signature, P11Cipher,
> P11KeyGenerator, etc.-: create and destroy native keys through a reference
> counting scheme exposed by P11Key class. There are two kind of usages:
>
>  1) stateless: the native key is "atomically" created, used and destroyed.
> I.e.: MAC calculation, getEncodedInternal operation (on P11Key objects),
> signature operations, TLS key derivation, etc.
>
>  2) statefull: the native key is created, one or multiple intermediate
> actions are performed by the key user, a final action is performed and
> finally the native key is destroyed. I.e.: cipher operations.
>
> For keys that are extractable but sensitive (CKA_SENSITIVE attribute is
> true), as the case when operating in FIPS mode, wrapping/unwrapping is used
> as a workaround to extract session keys. Wrapper key is global and lives
> forever.
>
> There are no interface changes for SunPKCS11 external users.
>
> If keys are not extractable or the feature cannot be enabled for any other
> reason, the previous finalizer scheme is used as a fallback.
>
>
> ADDITIONAL IMPLEMENTATION NOTES
> 
>
> When a P11Key is created, a constructor parameter exists to indicate if
> the feature is enabled for that specific key. For this feature to be
> enabled, 2 additional conditions apply: 1) SunPKCS11 backend has to be NSS,
> and 2) key has to be extractable. If the feature is not enabled for a key,
> behavior is as previous to this patch (native key destruction relies on
> finalizer execution).
>
> The only P11Key user that explicitly does not use this feature is
> P11KeyStore. This is because these 

RFR 6913047: SunPKCS11 memory leak

[Martin Balao]

Hi,

I'd like to propose a fix for bug JDK-6913047: "Long term memory leak when
using PKCS11 and JCE exceeds 32 bit process address space" [1]. This fix
does not contain changes in the GC and is SunPKCS11 internal only.

PROBLEM


When using the SunPKCS11 crypto provider (for cipher, signature, mac, key
generation or any other operation), multiple key objects may be created.
I.e.: every time a TLS session is established, a unique master key (derived
from the pre-master-key) has to be created and then used for encryption and
decryption operations. This is a legitimate use case in which key caching
does not make sense as each key is unique per session. These keys are of
P11Key type and have a corresponding native key object created. In the case
of NSS SunPKCS11 backend (PKCS11 software token), this native key object is
temporarily stored in the process native heap. The interface is simple: a
JNI call is done to create a native key object (C_CreateObject,
C_CopyObject, C_DeriveKey, C_GenerateKeys, etc., according to the PKCS11
interface) and an integer handler is kept in the Java side (P11Key). When
the P11Key object is destroyed, a finalizer code is executed to free the
native key object (through C_DestroyObject JNI call). The problem is that
finalizer code execution happens only if the JVM garbage-collector cleans
up the P11Key object. That may be delayed or not done at all, depending on
different GC algorithms, parameters and environment conditions. As a
result, the native heap may be exhausted with not freed native key objects,
and the JVM will then crash -this is particularly true for 32 bits VMs
where the virtual address space can be exhausted-.


SCOPE


The fix is proposed for SunPKCS11 with NSS backend only. Other PKCS11
backends are not currently under scope. It's likely that hardware PKCS11
backends store native key objects in their own memory, preventing a native
heap exhaustion and a JVM crash. However, it might be possible to cause an
exhaustion on their own memory blocking key objects creation at some point.
In any case, this is speculative as no tests were done on our side with
real hardware.


SOLUTION


Assuming that native keys are extractable, the idea is to hold native key
data in the Java heap while keys are not in use. When a P11Key is created,
every CK_ATTRIBUTE (PKCS11) value for the native key is queried, data
stored in an opaque Java byte[] (inside the P11Key object) and native key
destroyed. Every time the P11Key is about to be used, the native key is
created with the stored data. After usage, the native key is again
destroyed. Thus, it's not necessary to wait for a finalizer execution to
cleanup native resources: cleanup is done at deterministic and
previously-known points. This comes with a resposibility for key users
-which are all SunPKCS11 internal services like P11Signature, P11Cipher,
P11KeyGenerator, etc.-: create and destroy native keys through a reference
counting scheme exposed by P11Key class. There are two kind of usages:

 1) stateless: the native key is "atomically" created, used and destroyed.
I.e.: MAC calculation, getEncodedInternal operation (on P11Key objects),
signature operations, TLS key derivation, etc.

 2) statefull: the native key is created, one or multiple intermediate
actions are performed by the key user, a final action is performed and
finally the native key is destroyed. I.e.: cipher operations.

For keys that are extractable but sensitive (CKA_SENSITIVE attribute is
true), as the case when operating in FIPS mode, wrapping/unwrapping is used
as a workaround to extract session keys. Wrapper key is global and lives
forever.

There are no interface changes for SunPKCS11 external users.

If keys are not extractable or the feature cannot be enabled for any other
reason, the previous finalizer scheme is used as a fallback.


ADDITIONAL IMPLEMENTATION NOTES


When a P11Key is created, a constructor parameter exists to indicate if the
feature is enabled for that specific key. For this feature to be enabled, 2
additional conditions apply: 1) SunPKCS11 backend has to be NSS, and 2) key
has to be extractable. If the feature is not enabled for a key, behavior is
as previous to this patch (native key destruction relies on finalizer
execution).

The only P11Key user that explicitly does not use this feature is
P11KeyStore. This is because these keys (token keys) are managed by alias
names and makes no sense to remove them from the key store (they need to be
accessible by an alias at any time).

Because P11Key objects can be used by multiple threads at a time, there is
a thread-safe reference counting scheme in order to decide when a native
key object has to be created or destroyed. The SunPKCS11 internal API to
use a P11Key is as follows: 1)