RFR 6913047: SunPKCS11 memory leak

Tue Oct 2 01:48:00 UTC 2018

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/%7Embalao/webrevs/6913047/6913047.webrev.11/>
>>  * 
>> http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip 
>> <http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.11.zip>
>>
>> <src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java>
>> 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.
>>
>> <src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java>
>> 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.-
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181001/c9729302/attachment.htm>