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

[Xuelei Fan]

No idea.  I may not use a printable character to zero a password.

Xuelei

On 8/23/2018 3:31 AM, Weijun Wang wrote:
> I have no more comment.
> 
> As for fill(passwd, '\0'), maybe JVM can translate it to ZeroMemory() or memset(0). In fact, I don't know why originally it was fill(passwd, '0'). I can only guess that it can still be printed out as a normal string and if someone misuse it there won't be a NPE. Who knows?
> 
> Thanks
> Max
> 
>> On Aug 23, 2018, at 6:01 AM, Seán Coffey <sean.coffey at oracle.com> wrote:
>>
>>
>>
>> On 22 August 2018 19:22:49 IST, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>>> Hi Seán!
>>>
>>> Just a minor comment.
>>>
>>> I don't know if it's even measurable in this context, but I was under
>>> impression that filling memory with zero *bytes* might be a slightly
>>> more efficient then filling with any other constant.
>>>
>>> Maybe it is better to use Arrays.fill(passwd, '\0') instead of
>>> Arrays.fill(passwd, '0') to give the JVM a chance to optimize filling
>>> if
>>> it's possible?
>>
>> Interesting comment Ivan. I was not aware of such an effect! If you've further references on that, I'd appreciate it. '0' is used in other, similar, fill operations IIRC. Perhaps we can optimize such code across all security libs code via another JBS issue.
>>
>> Regards,
>> Sean.
>>
>>>
>>> With kind regards,
>>>
>>> Ivan
>>>
>>>
>>> On 8/22/18 9:25 AM, Seán Coffey wrote:
>>>> Thanks for reviewing. comments inline..
>>>>
>>>> On 22/08/18 15:50, Weijun Wang wrote:
>>>>> PBES2Core.java:
>>>>>
>>>>>   181         byte[] passwdBytes = key.getEncoded();
>>>>>   182         char[] passwdChars = null;
>>>>>   183         PBEKeySpec pbeSpec;
>>>>>   184         try {
>>>>>   185             if ((passwdBytes == null) ||
>>>>>   186 !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
>>>>>   187                 throw new InvalidKeyException("Missing
>>> password");
>>>>>   188             }
>>>>>   ....
>>>>>   272         } finally {
>>>>>   273             if (passwdChars != null) Arrays.fill(passwdChars,
>>> '
>>>>> ');
>>>>>   274             Arrays.fill(passwdBytes, (byte)0x00);
>>>>>   275         }
>>>>>
>>>>> If passwdBytes == null, line 274 would throw an NPE.
>>>> Good catch. Corrected.
>>>>>
>>>>> PBKDF2KeyImpl.java:
>>>>>
>>>>>    87         char[] passwd = keySpec.getPassword();
>>>>>    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');
>>>>>
>>>>> If passwd == null, line 97 would throw an NPE.
>>>> Another good catch!
>>>>
>>>> updated webrev :
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v3/webrev/
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>>>
>>>>> Otherwise fine.
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>
>>>>>> On Aug 17, 2018, at 12:53 AM, Seán Coffey <sean.coffey at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> Find new webrev here Max :
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/
>>>>>>
>>>>>> regards :
>>>>>>
>>>>
>>>>
>