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.cof...@oracle.com> wrote:
On 22 August 2018 19:22:49 IST, Ivan Gerasimov <ivan.gerasi...@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.cof...@oracle.com>
wrote:
Find new webrev here Max :
http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/
regards :
