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