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