On Tue, 30 May 2023 23:42:24 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8301553: Support Password-Based Cryptography in SunPKCS11 (iteration #4) >> >> Co-authored-by: Francisco Ferrari <fferr...@redhat.com> >> Co-authored-by: Martin Balao <mba...@redhat.com> > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java > line 221: > >> 219: // for the underlying cipher is equal to the PBE service key >> length. >> 220: // Otherwise, initialization fails. >> 221: return svcPbeKi.keyLen; > > This method "Returns the key size of the given key object in bits. " > How do you ensure that this key is the one used in the initialization? This > method may also throw InvalidKeyException though, With this impl, even if > passing a null key, this would return an int value and not detecting the key > is invalid. @valeriepeng: the rationale behind this decision is based on the only usage of `engineGetKeySize()`, which corresponds to a [`crypto.policy=limited` _Cryptographic Jurisdiction Policy_](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/java.base/share/conf/security/java.security#L906-L942). Here, `engineGetKeySize()` is employed to check the key during the _Cipher_ initialization (see [`Cipher.java:1463`](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/java.base/share/classes/javax/crypto/Cipher.java#L1463), [`Cipher.java:1110`](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/java.base/share/classes/javax/crypto/Cipher.java#L1110), [`Cipher.java:1139`](https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/java.base/share/classes/javax/crypto/Cipher.java#L1139)). So, what we can assert is that if the key size isn't going to be `svcPbeKi.keyLen`, then that very sa me _Cipher_ initialization will later fail anyway. There also is a previous precedent set by _SunJCE_: https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java#L214-L216 https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e/src/java.base/share/classes/com/sun/crypto/provider/PBEWithMD5AndTripleDESCipher.java#L377-L379 https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e//src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java#L355-L358 https://github.com/openjdk/jdk/blob/1c596e147dac5102b31a946c905843b9b19b428e//src/java.base/share/classes/com/sun/crypto/provider/PBEWithMD5AndDESCipher.java#L367-L371 NOTE: by passing `-Djava.security.properties=<(echo crypto.policy=limited)` as a JVM argument to [`test/jdk/sun/security/pkcs11/Cipher/PBECipher.java`](https://github.com/openjdk/jdk/blob/80575ccb06f8f1aee26f9cd18b8504211d2f62c7/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java#L191), I could reproduce a failure of the [older implementation using the underlying cipher](https://github.com/openjdk/jdk/blob/80575ccb06f8f1aee26f9cd18b8504211d2f62c7/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java#L210-L215): if we pass a `com.sun.crypto.provider.PBEKey` of the [`PBE` → `PBEWithMD5AndDES`](https://github.com/openjdk/jdk/blob/80575ccb06f8f1aee26f9cd18b8504211d2f62c7/src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java#L279) algorithm, the [underlying `AES` cipher's `P11SecretKeyFactory.convertKey()` call](https://github.com/openjdk/jdk/blob/80575ccb06f8f1aee26f9cd18b8504211d2f62c7/src/jdk.crypto.cryptoki/share/classes/sun/security/pk cs11/P11Cipher.java#L1021-L1022) fails because this algorithm is not registered in the `P11SecretKeyFactory.keyInfo` map, so [`ki` is `null` here](https://github.com/openjdk/jdk/blob/80575ccb06f8f1aee26f9cd18b8504211d2f62c7/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java#L292-L295), resulting in `InvalidKeyException: Cannot use a PBEWithMD5AndDES key for a AES service`. I've also found a failure for the current implementation when using `crypto.policy=limited`, I will investigate this further tomorrow. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1213781875
