On 5/16/18 10:02 PM, Weijun Wang wrote:



On May 17, 2018, at 2:42 AM, Sean Mullan <sean.mul...@oracle.com> 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.

Ok, I didn't realize this pattern was used before.


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

Seems more complicated and harder to understand that code. So I'm ok with the current changes.

--Sean


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.

Reply via email to