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