RFR: 8301553: Support Password-Based Cryptography in SunPKCS11 [v3]

Wed May 17 19:12:08 UTC 2023

On Wed, 17 May 2023 03:11:54 GMT, Martin Balao <mbalao at 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 <fferrari at redhat.com>
>    Co-authored-by: Martin Balao <mbalao at redhat.com>
>  - 8301553: Support Password-Based Cryptography in SunPKCS11
>    
>    Co-authored-by: Francisco Ferrari <fferrari at redhat.com>
>    Co-authored-by: Martin Balao <mbalao at 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