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

Seán Coffey sean.coffey at oracle.com
Fri Aug 10 14:18:07 UTC 2018


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.
>>>




More information about the security-dev mailing list