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.cof...@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.
