RFR 8202299: Java Keystore fails to load PKCS12/PFX certificates created in WindowsServer2016

[Weijun Wang]

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

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.