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