[15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
Xuelei Fan
xuelei.fan at oracle.com
Mon Jun 8 21:55:21 UTC 2020
On 6/8/2020 1:42 PM, Valerie Peng wrote:
> "md instanceof Cloneable && md is not from PKCS11" is not entirely
> precise. What I mean is that for MessageDigestSpi impls from PKCS11
> provider, we will need to call the clone() to know for sure whether
> cloning is supported. If we decide to employ these extra logic for
> saving clone() call, it's better to do it inside the
> MessageDigest.of(...) so the callers don't have to repeat the same
> logic. Comments?
>
It sounds like a good idea to me if the repeatedly checking could be avoid.
Xuelei
> Valerie
>
> On 6/8/2020 1:34 PM, Valerie Peng wrote:
>> Hmm, I checked all MessageDigestSpi impls of JDK providers... The
>> story is more complicated than we expect.
>>
>> For SUN provider which implement the digest spi, implementing
>> Cloneable means it supports clone functionality.
>>
>> However, for SunPKCS11 provider which wraps native PKCS11 library, it
>> always implements Cloneable interface, but when clone() is called, it
>> will then perform the equivalent PKCS11 calls and throw CNSE if any
>> PKCS11 error is observed.
>>
>> So, there is a possibility that the instanceof check and the clone()
>> check leads to different result in this particular scenario.
>>
>> The chance of 3rd non-PKCS11 party provider whose
>> MessageDigest/MessageDigestSpi impl implements Cloneable but throws
>> CNSE when clone() is called should be very low? So, I think it should
>> be sufficient to use "md instanceof Cloneable && md is not from PKCS11"?
>>
>> Valerie
>>
>> On 6/6/2020 9:10 AM, Xuelei Fan wrote:
>>> As the the Delegate class takes care of the Cloneable SPI
>>> implementation, it should be sufficient to use "md instanceof
>>> Cloneable" only. It is not a expected behavior that a provider
>>> implements Cloneable but does not really support it.
>>>
>>> Xuelei
>>>
>>> On 6/5/2020 10:54 PM, Weijun Wang wrote:
>>>> Is it possible to try "md instanceof Cloneable || md.clone()
>>>> returns"? Hopefully this is fast enough in most cases and also has
>>>> the chance to recognize more actually-cloneable ones.
>>>>
>>>> I'm also OK with simply using "md instanceof Cloneable".
>>>>
>>>> --Max
>>>>
>>>>> On Jun 6, 2020, at 12:02 PM, Valerie Peng <valerie.peng at oracle.com>
>>>>> wrote:
>>>>>
>>>>>
>>>>> I am merely following the spec's recommendation of trying the
>>>>> clone() for cloneability check.
>>>>>
>>>>> If you both are ok with it and prefer the instanceof check, I can
>>>>> sure reverting back the changes in HmacCore and HandshakeHash classes.
>>>>>
>>>>> Valerie
>>>>>
>>>>> On 6/5/2020 2:04 AM, Seán Coffey wrote:
>>>>>> I share the same concern. clone() is a heavy weight operation in
>>>>>> constructors that can be called alot during intensive crypto
>>>>>> operations.
>>>>>>
>>>>>> Now that you've done work on Delegate class - why not also keep
>>>>>> the (instanceof Cloneable) test ? That can serve as your fastpath
>>>>>> for the default JDK configuration AFAIK.
>>>>>>
>>>>>> regards,
>>>>>> Sean.
>>>>>>
>>>>>> On 05/06/2020 00:16, Weijun Wang wrote:
>>>>>>>
>>>>>>>> 在 2020年6月5日,03:19,Valerie Peng <valerie.peng at oracle.com>
>>>>>>>> 写道:
>>>>>>>>
>>>>>>>>> Can you give an example when these 2 rules have different
>>>>>>>>> results? Is this only true for those implementation that
>>>>>>>>> directly subclass MessageDigest?
>>>>>>>> Before this fix, even the Spi impl implements Cloneable the
>>>>>>>> instanceof check will always fail because the wrapper class,
>>>>>>>> i.e. MessageDigest.Delegate does not. However, if you call the
>>>>>>>> clone() (made public by the MessageDigest class), it will
>>>>>>>> succeed because Delegate.clone() checks to see if the spi object
>>>>>>>> implements the Cloneable interface, if yes, it will proceed to
>>>>>>>> call the spi clone(). So, for this scenario, the results are
>>>>>>>> different, e.g. instanceof returns false, but clone() succeeds.
>>>>>>>> This is just one example. Is this what you are asking?
>>>>>>> No.
>>>>>>>
>>>>>>> I understand this case, but this has already been fixed. Is there
>>>>>>> any other example? Or are you only follow the words in the spec?
>>>>>>> i.e. try clone() to see if it’s cloneable.
>>>>>>>
>>>>>>> I am worried that try clone() is much heavier than just check
>>>>>>> instanof Cloneable.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Max
>>>>
More information about the security-dev
mailing list