RFR: JDK-8209129 :Further improvements to cipher buffer management
Seán Coffey
sean.coffey at oracle.com
Thu Aug 16 16:53:43 UTC 2018
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.
>>>>
>
More information about the security-dev
mailing list