RFR 8220513: Wrapper Key may get deleted when closing sessions in SunPKCS11 crypto provider

[Martin Balao]

Hi Valerie,

Thanks for having a look at this proposal.

On 3/25/19 9:03 PM, Valerie Peng wrote:
> 
> Thanks for trying to address this "free-wrapper-key" issue. However,
> changes in the key clean up/free area is difficult to verify and check
> for issues and I am a little concerned that this new model may be hard
> to trace when the key id is mutable and potentially fragile/error prone
> for future changes/fixes.

I agree in that there is some complexity involved, but it is not much
different than the one we have dealt with before in this whole "memory
leak" fix.

> 
> My questions/feedbacks on current changes are:
> 
> 1) line 1283: ref may be null (see line 1220) and this will lead to NPE?

For line 1283 to be executed, nativeKeyInfo has to be non-null (see line
1264). When nativeKeyInfo is non-null, a SessionKeyRef instance is
always assigned to a "ref" variable, and no execution path can set "ref"
back to null -we removed the previous "this.ref = this.ref.dispose()" line-.

This is not by-chance, this is because we now keep SessionKeyRef
instances always alive when there is native key info extracted. Previous
to this patch, a SessionKeyRef instance was alive only while there was a
native key created. The reason for this change, as I said, is that
SessionKeyRef instances now protect not only a created native key but a
wrapper key that may have been used to encrypt the extracted native key
information.

> 
> 2) It looks like you don't really need to store the "wrapperKeyUsed"
> value in both NativeKeyHolder and SessionKeyRef classes. It can be a
> local variable for NativeKeyHolder class and the increment of
> nativeKeyWrapperRefCount can be moved to SessionKeyRef constructor.  The
> counter nativeKeyWrapperRefCount is incremented inside NativeKeyHolder
> constructor

Yes, I thought about that when coming to the proposed patch but was not
convinced for clarity and safety reasons. When a wrapper key does not
exist and has to be created, we must exit the creation block with the
reference counter incremented to protect the key. Otherwise, a race
condition is possible: while one thread creates the native key but still
does not increment the reference counter, a short-living object may
destroy the wrapper key. Moving the whole creation block to the
SessionKeyRef constructor is less clear to me -see below-.

> 
> 3) I have not spent as much time as you on tracing this. However, I
> think we should be able to keep the "final keyID" approach and handle
> the NativeKeyWrapperRefCount inc/dec in SessionKeyRef
> constructor/dispose methods. Have you tried this?

Keeping the "final keyID" approach is creating different SessionKeyRef
instances every time a native key is created. However, this approach
does not have a SessionKeyRef instance when there is extracted native
key information but there isn't a native key created. Now suppose that
this extracted native key information is protected by a wrapper key and
that the P11Key gets garbage-collected. There is no one to decrement the
reference counter and the wrapper key will be always alive.

> 
> 4) It may not be enough to just run tests under sun/security/pkcs11, as
> SunPKCS11 provider is used by many other JDK security components such as
> TLS. To be safe, you should submit the test against jdk-tier1,jdk-tier2
> to check for possible regressions. If you can't, please let me know.
> 

I'm not exactly sure how can I do that but sounds good. If I cannot find
public information, I'll ask you for help.

Thanks,
Martin.-