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

[Valerie Peng]

Hi Martin,

Alright, as long as this mutable-key-id approach works as expected, I 
can live with it.

The refList field inside the SessionKeyRef is changed to use LinkedList. 
When there is a large number of P11Key objects, the refList.remove(this) 
call (line1415) may takes a long long time. Have you considered some 
other data structure like HashSet?

Lastly, on a somewhat relevant note, I noticed that line 311 in 
p11_keymgmt.c, the if-check of jWrappingKeyHandle is against -1, it 
should be against value 0? Also, the else method of this if-check  
should throw an exception? Would be good to address them as well.

Thanks,
Valerie
On 3/29/2019 9:05 AM, Martin Balao wrote:
> 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.-