RFR JDK-8134364: Add defensive copies to get/set methods for OCSPNonceExtension
Jamil Nimeh
jamil.j.nimeh at oracle.com
Tue Sep 1 21:12:13 UTC 2015
Hi Sean, thanks for the comments, they all sound very reasonable to me.
I'll get on fixing those now. WRT the cloning, I figured that since the
class had public visibility and the methods in question were public
methods that it was better to err on the side of safety, even with the
class being in a "sun" package. Thanks also for the link
[1]...interesting reading.
--Jamil
On 09/01/2015 11:29 AM, Sean Mullan wrote:
> On 08/28/2015 09:25 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> I've removed the CertAttrSet interface from OCSPNonceExtension and
>> trimmed away a few unneeded methods. As a result the class is immutable
>> now.
>
> Looks a lot cleaner. Strictly speaking, the cloning is not necessary
> since this is an internal class -- unless the array can leak through
> some code path to untrusted code, but I think in this case it is good
> practice and probably won't affect performance much as nonces are a
> rarely used feature (I think). A few comments (some stylistic based on
> [1]) on OCSPNonceExtension:
>
> 48: make class final
>
> 49: constants are generally named upper case and with "_" as a word
> separator, so EXTENSION_NAME is better.
>
> 58: move this private method after the constructors (or better yet,
> embed it inside the ctor since it is only called from there)
>
> 84: not necessarily the SUN provider, depends on the configured
> provider order
>
> 174: toString should probably use StringBuilder.append instead of "+".
>
> --Sean
>
> [1] review ongoing at
> http://cr.openjdk.java.net/~alundblad/styleguide/feedback.html
>
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8134364
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.01
>>
>> Thanks,
>> --Jamil
>>
>> On 08/25/2015 08:11 AM, Jamil Nimeh wrote:
>>> Hi Sean,
>>>
>>> Yes, I was just following Extension convention in terms of
>>> implementing CertAttrSet. Within sun.security.x509, X509CertInfo uses
>>> the set methods from other objects implementing CertAttrSet. But
>>> OCSPNonceExtension really isn't a certificate attribute so it probably
>>> doesn't need to bring this interface in.
>>>
>>> I'll refactor this and alter the tests to match.
>>>
>>> --Jamil
>>>
>>> On 08/25/2015 06:40 AM, Sean Mullan wrote:
>>>> Is it necessary for the class to implement CertAttrSet? I realize all
>>>> of the internal X.509 extensions implement that, but that was done a
>>>> long time ago and not something we really want to carry forward if
>>>> not needed. I think it would be cleaner not to do that as it would
>>>> more easily allow you to make the class immutable, and would allow
>>>> you to remove the set method.
>>>>
>>>> Thanks,
>>>> Sean
>>>>
>>>> On 08/25/2015 04:14 AM, Xuelei Fan wrote:
>>>>> OCSPNonceExtension.java
>>>>> =======================
>>>>> - nonceData = (byte[])obj;
>>>>> + nonceData = ((byte[])obj).clone();
>>>>>
>>>>> Do you want to check null obj?
>>>>>
>>>>> - return nonceData;
>>>>> + return (nonceData != null ? nonceData.clone() : null);
>>>>>
>>>>> I think you may want to enclose the "!=" operator as:
>>>>>
>>>>> + return (nonceData != null) ? nonceData.clone() : null;
>>>>>
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 8/25/2015 12:55 PM, Jamil Nimeh wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This is a quick fix to the OCSPNonceExtension class to add a couple
>>>>>> defensive copies to public get/set methods.
>>>>>>
>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8134364
>>>>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.00
>>>>>>
>>>>>> Thanks,
>>>>>> --Jamil
>>>>>
>>>
>>
More information about the security-dev
mailing list