> On May 17, 2018, at 2:42 AM, Sean Mullan <[email protected]> wrote:
>
> The while(true) in PKCS12KeyStore.java seems like it isn't really necessary,
> since you are calling the code inside it at most twice. I think a better
> approach would be to move lines 2134-2146 into a utility method and call it
> again if you get an Exception and the password is empty.
The while block looks a little strange, but there are too many local variables
here and extracting the lines into a method means we need to pass all of them
as arguments. This is also just a single step in a long process and taking it
out as a separate method breaks the code reading. Finally, such while blocks
appear 3 times and we will have to create multiple methods like
tryDecryptKey/tryDecryptCert/tryVerifyMac.
It will be nice if Java allows a method defined inside a method. What do you
think if I extract the lines into a lambda?
interface RetryWithZero<T> {
T tryOnce(char[] password) throws Exception;
static <S> S run(RetryWithZero<S> f, char[] password) throws Exception {
try {
return f.tryOnce(password);
} catch (Exception e) {
if (password.length == 0) {
return f.tryOnce(new char[1]);
}
throw e;
}
}
}
byte[] keyInfo = RetryWithZero.run(pass -> {
SecretKey skey = getPBEKey(pass);
Cipher cipher = Cipher.getInstance(
mapPBEParamsToAlgorithm(algOid, algParams));
cipher.init(Cipher.DECRYPT_MODE, skey, algParams);
return cipher.doFinal(encryptedKey);
}, password);
byte[] rawData = safeContentsData;
try {
safeContentsData = RetryWithZero.run(pass -> {
SecretKey skey = getPBEKey(pass);
Cipher cipher = Cipher.getInstance(algOid.toString());
cipher.init(Cipher.DECRYPT_MODE, skey, algParams);
return cipher.doFinal(rawData);
}, password);
} catch (Exception e) {
throw new IOException("keystore password was incorrect",
new UnrecoverableKeyException(
"failed to decrypt safe contents entry: " + e));
}
RetryWithZero.run(pass -> {
SecretKey key = getPBEKey(pass);
m.init(key, params);
m.update(authSafeData);
byte[] macResult = m.doFinal();
if (debug != null) {
debug.println("Checking keystore integrity " +
"(" + m.getAlgorithm() + " iterations: " + ic + ")");
}
if (!MessageDigest.isEqual(macData.getDigest(), macResult)) {
throw new UnrecoverableKeyException("Failed PKCS12" +
" integrity checking");
}
return (Void)null;
}, password);
If we code this way, password will NOT be updated permanently (see p.p.s of my
previous mail).
Thanks
Max
>
> Looks fine otherwise.
>
> --Sean
>
> On 4/27/18 12:56 PM, Weijun Wang wrote:
>> Please take a look at
>> http://cr.openjdk.java.net/~weijun/8202299/webrev.00/
>> Turns out we have to retry [0] other than [] in all 3 locations: decrypting
>> keys, decrypting certs, and verifying the mac.
>> Thanks
>> Max
>> p.s. You might wonder why suddenly in Windows Server 2016, Microsoft starts
>> using [0] to generate the Mac. In fact, they have been doing this all the
>> time. However, before 2016, they also encrypted the certificates, and to
>> decrypt them, Java has already changed password from [] to [0].
>> p.p.s. But is this correct? Should the certificate decryption code only
>> temporarily retries [0] but not changing password itself? Well, maybe. But
>> unless a weird software sometimes uses [] and sometimes [0], this will not
>> be a problem, and changing password itself saves us some cycles from always
>> trying twice.
