RFR: JDK-8209129 :Further improvements to cipher buffer management

[Seán Coffey]

Ping. I'd like to push on with this review.

Regards,
Sean.

On 16/08/18 17:53, Seán Coffey wrote:
> Find new webrev here Max :
>
> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/
>
> regards :
>
>> MD4.java:
>>
>>   110     void implReset() {
>>
>>   Add @Override. 
> There's a whole bunch of annotations missing in these files, I'd 
> prefer if it was fixed up with different bug ID.
>
> another thing we're to be cautious about is the nulling out of values 
> returned from some Key's such as PBEKey subclasses. We're assuming 
> that getPassword() returns cloned copies. That should be the case for 
> any production fit code.
>
> Edit to test/jdk/com/sun/crypto/provider/Cipher/PBE/PKCS12Cipher.java 
> was necessary as a result.
>
> regards,
> Sean.
>
>
> On 10/08/2018 15:18, Seán Coffey wrote:
>> Thanks for the review Max - comments inline..
>>
>> Regards,
>> Sean.
>>
>> On 10/08/18 11:53, Weijun Wang wrote:
>>> HmacPKCS12PBESHA1.java:
>>>
>>>    76             byte[] passwdBytes = key.getEncoded();
>>>    77             if ((passwdBytes == null) ||
>>>    78                 !(key.getAlgorithm().regionMatches(true, 0, 
>>> "PBE", 0, 3))) {
>>>    79                 throw new InvalidKeyException("Missing 
>>> password");
>>>    80             }
>>>
>>>   Please also cleanup passwdBytes. Maybe divide the if check above 
>>> into 2 to avoid passwdBytes being assigned but algorithm is not PBE.
>> Yes - I had thought about refactoring this code during edits. Will 
>> make that change.
>>>
>>>   132         Arrays.fill(passwdChars, '0');
>>>
>>>   In a finally block?
>>>
>>> PBES1Core.java:
>>>
>>>   250         Arrays.fill(passwdBytes, (byte) 0x00);
>>>
>>>   In a finally block?
>>>
>>> PBES2Core.java:
>>>
>>>   273         Arrays.fill(passwdChars, ' ');
>>>   274         Arrays.fill(passwdBytes, (byte)0x00);
>>>
>>>   In a finally block?
>> Yes - makes sense to capture above in finally blocks. Will re-factor.
>>>
>>> PBKDF2KeyImpl.java:
>>>
>>>    88         if (passwd == null) {
>>>    89             // Should allow an empty password.
>>>    90             this.passwd = new char[0];
>>>    91         } else {
>>>    92             this.passwd = passwd.clone();
>>>    93         }
>>>    94         // Convert the password from char[] to byte[]
>>>    95         byte[] passwdBytes = getPasswordBytes(this.passwd);
>>>    96         // remove local copy
>>>    97         Arrays.fill(passwd, '0');
>>>
>>>   Strange, why passwd.clone()?
>> Not sure - it's original behaviour. I'll check if it makes sense to 
>> change.
>>>
>>>   129         Arrays.fill(passwdBytes, (byte)0x00);
>>>
>>>   In a finally block?
>>>
>>> PBMAC1Core.java:
>>>
>>>   111             byte[] passwdBytes = key.getEncoded();
>>>   112             if ((passwdBytes == null) ||
>>>   113                 !(key.getAlgorithm().regionMatches(true, 0, 
>>> "PBE", 0, 3))) {
>>>   114                 throw new InvalidKeyException("Missing 
>>> password");
>>>   115             }
>>>
>>>   170         Arrays.fill(passwdChars, ' ');
>>>
>>>   Same comments as for HmacPKCS12PBESHA1.java.
>>>
>>> PKCS12PBECipherCore.java:
>>>
>>>   262         char[] passwdChars = null;
>>>
>>>   Deal with the same way as in PBMAC1Core.java?
>>>
>>> MD4.java:
>>>
>>>   110     void implReset() {
>>>
>>>   Add @Override.
>> All above comments make sense. I've come across one other possible 
>> issue in MessageDigest functionality. I'm debugging that to be sure. 
>> Will get a new webrev out once that's done.
>>
>> regards,
>> Sean.
>>>
>>> Thanks
>>> Max
>>>
>>>
>>>> On Aug 9, 2018, at 6:42 PM, Seán Coffey <sean.coffey at oracle.com> 
>>>> wrote:
>>>>
>>>> Adding RFR to title..
>>>>
>>>> On 09/08/2018 11:37, Seán Coffey wrote:
>>>>> I've been looking further at how private/temporary buffers are 
>>>>> used in cipher/keystore management and identified some more areas 
>>>>> that could benefit with a more aggressive nulling out of contents.
>>>>>
>>>>> I've been testing through use of stepping through debugging 
>>>>> sessions while setting/getting keys and capturing process memory 
>>>>> via tooling like gcore.
>>>>>
>>>>> JBS report : https://bugs.openjdk.java.net/browse/JDK-8209129
>>>>>
>>>>> webrev : 
>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v1/webrev/index.html 
>>>>>
>>>>>
>>>>> TCK and regression tests are green.
>>>>>
>>>>> regards,
>>>>> Sean.
>>>>>
>>
>