On Wed, 17 May 2023 03:11:54 GMT, Martin Balao <mba...@openjdk.org> wrote:
>> We would like to propose an implementation for the [JDK-8301553: Support >> Password-Based Cryptography in >> SunPKCS11](https://bugs.openjdk.org/browse/JDK-8301553) enhancement >> requirement. >> >> In addition to pursuing the requirement goals and guidelines of >> [JDK-8301553](https://bugs.openjdk.org/browse/JDK-8301553), we want to share >> the following implementation notes (grouped per altered file): >> >> * >> ```src/java.base/share/classes/com/sun/crypto/provider/HmacPKCS12PBECore.java``` >> (modified) >> * This file contains the ```SunJCE``` implementation for the [PKCS #12 >> General Method for Password >> Integrity](https://datatracker.ietf.org/doc/html/rfc7292#appendix-B) >> algorithms. It has been modified with the intent of consolidating all >> parameter checks in a common file >> (```src/java.base/share/classes/sun/security/util/PBEUtil.java```), that can >> be used both by ```SunJCE``` and ```SunPKCS11```. This change does not only >> serve the purpose of avoiding duplicated code but also ensuring alignment >> and compatibility between different implementations of the same algorithms. >> No changes have been made to parameter checks themselves. >> * The new ```PBEUtil::getPBAKeySpec``` method introduced for parameters >> checking takes both a ```Key``` and a ```AlgorithmParameterSpec``` instance >> (same as the ```HmacPKCS12PBECore::engineInit``` method), and returns a >> ```PBEKeySpec``` instance which consolidates all the data later required to >> proceed with the computation (password, salt and iteration count). >> >> * ```src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java``` >> (modified) >> * This file contains the ```SunJCE``` implementation for the [PKCS #5 >> Password-Based Encryption >> Scheme](https://datatracker.ietf.org/doc/html/rfc8018#section-6.2) >> algorithms, which use PBKD2 algorithms underneath for key derivation. In the >> same spirit than for the ```HmacPKCS12PBECore``` case, we decided to >> consolidate common code for parameters validation and default values in a >> single file >> (```src/java.base/share/classes/sun/security/util/PBEUtil.java```), that can >> serve both ```SunJCE``` and ```SunPKCS11``` and ensure compatibility. >> However, instead of a single static method at the implementation level (see >> ```PBEUtil::getPBAKeySpec```), we create an instance of an auxiliary class >> and invoke an instance method (```PBEUtil.PBES2Params::getPBEKeySpec```). >> The reason is to persist parameters data that has to be consistent between >> calls to ```PBES2Core::engineInit``` (in its multiple ... > > Martin Balao has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Rebase fix after JDK-8306033. Replace called functions with their new > names. > - 8301553: Support Password-Based Cryptography in SunPKCS11 (iteration #1) > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - 8301553: Support Password-Based Cryptography in SunPKCS11 > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> src/java.base/share/classes/com/sun/crypto/provider/HmacPKCS12PBECore.java line 115: > 113: try { > 114: derivedKey = PKCS12PBECipherCore.derive( > 115: keySpec.getPassword(), keySpec.getSalt(), Comparing to the original impl, this new call of keySpec.getPassword() produces extra copy of password which needs to be cleared as well. src/java.base/share/classes/com/sun/crypto/provider/HmacPKCS12PBECore.java line 121: > 119: keySpec.clearPassword(); > 120: } > 121: SecretKey cipherKey = new SecretKeySpec(derivedKey, "HmacSHA1"); Can clear out the "derivedKey" bytes if no longer needed. src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 165: > 163: byte[] derivedKey = s.getEncoded(); > 164: s.clearPassword(); > 165: SecretKeySpec cipherKey = new SecretKeySpec(derivedKey, > cipherAlgo); Clear out the "derivedKey" bytes if no longer needed. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java line 345: > 343: throw new InvalidKeyException("Encoded format must be > RAW"); > 344: } > 345: byte[] encoded = key.getEncoded(); Would be nice to clear out "encoded" bytes afterwards. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java line 362: > 360: session = token.getObjSession(); > 361: CK_MECHANISM ckMech; > 362: char[] password = keySpec.getPassword(); Should clear out "password" afterwards. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java line 391: > 389: } > 390: > 391: char[] encPassword; Same, clear out "encPassword" afterwards. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java line 444: > 442: int keyLength = 0; > 443: if ("RAW".equalsIgnoreCase(pbeKey.getFormat())) { > 444: byte[] encoded = pbeKey.getEncoded(); Should clear out "encoded" afterwards. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java line 450: > 448: } > 449: int ic = pbeKey.getIterationCount(); > 450: char[] pwd = pbeKey.getPassword(); Should clear out "pwd" afterwards. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java line 562: > 560: } else if (algorithm.equalsIgnoreCase("DES")) { > 561: if (keySpec instanceof DESKeySpec desKeySpec) { > 562: byte[] keyBytes = desKeySpec.getKey(); Would be nice to clear out "keyBytes" afterwards. Same goes for the other "keyBytes" in the same method. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196925886 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196926732 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196936604 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196940708 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196942089 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196943128 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196944774 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196946761 PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1196951831
