RFR: 8343232: PKCS#12 KeyStore support for RFC 9879: Use of Password-Based Message Authentication Code 1 (PBMAC1) [v14]

Fri Oct 24 19:48:27 UTC 2025

On Thu, 23 Oct 2025 14:20:47 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Mark Powers has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 31 commits:
>> 
>>  - merge
>>  - stragglers
>>  - checkpoint
>>  - remaining comments
>>  - more review comments from Sean and Weijun
>>  - more review comments from Weijun and Sean
>>  - another day another iteration
>>  - move algorithm-specific code into MacData and no change to SunJCE
>>  - fix behavior with keytool
>>  - default salt length and one other comment from Weijun
>>  - ... and 21 more: https://git.openjdk.org/jdk/compare/d8ebe387...8f0b0d02
>
> src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 309:
> 
>> 307:         pBES2_params.write(DerValue.tag_Sequence,
>> 308:                 // keysize encoded as octets
>> 309:                 PBKDF2Parameters.encode(salt, iCount, keysize/8, kdfAlgo_OID));
> 
> If you make the change I suggested in `PBKDF2Parameters.encode`, the line above should be
> 
> pBES2_params.writeBytes(PBKDF2Parameters.encode(salt, iCount, keysize/8, kdfAlgo_OID));

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 1:
> 
>> 1: /*
> 
> In this class, there are several places where `startsWith("pbewith")` is called. Add a comment to the method that the algorithm names are guaranteed to be using lowercase letters (at least for the prefix).

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 160:
> 
>> 158:             keySpec = new PBEKeySpec(password);
>> 159:         }
>> 160:         pbeKey = skf.generateSecret(keySpec);
> 
> If the line above fails, there is no chance to clean `keySpec`. Create a big try-finally block.

I assume you mean to put line 160 in the existing try-finally block rather than create another try-finally block just for this.

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 170:
> 
>> 168:         } finally {
>> 169:             keySpec.clearPassword();
>> 170:             sun.security.util.KeyUtil.destroySecretKeys(pbeKey);
> 
> Here, `pbeKey` is cleared before `m.update` and `m.doFinal` are called. This might be unsafe because `m` could be still using `pbeKey`.

Putting `m.update` and `m.doFinal` into the existing try-finally block.

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 248:
> 
>> 246:     }
>> 247: 
>> 248:     byte[] getSalt() {
> 
> This method is not used anywhere.

removed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 257:
> 
>> 255: 
>> 256:     /**
>> 257:      * Returns the ASN.1 encoding of this object.
> 
> This is a `static` method. There is no "this object".

Duh! Fixed.

> src/java.base/share/classes/sun/security/pkcs12/PBMAC1Parameters.java line 136:
> 
>> 134:         // keyDerivationFunc AlgorithmIdentifier {{PBMAC1-KDFs}}
>> 135:         tmp3.write(DerValue.tag_Sequence, PBKDF2Parameters.encode(salt,
>> 136:                 iterationCount, keyLength, kdfHmac));
> 
> If you make the change I suggested in `PBKDF2Parameters.encode`, the line above should be
> 
> tmp3.writeBytes(PBKDF2Parameters.encode(salt,
>                  iterationCount, keyLength, kdfHmac));

fixed

> src/java.base/share/classes/sun/security/pkcs12/PBMAC1Parameters.java line 137:
> 
>> 135:         tmp3.write(DerValue.tag_Sequence, PBKDF2Parameters.encode(salt,
>> 136:                 iterationCount, keyLength, kdfHmac));
>> 137:         tmp3.write(DerValue.tag_Sequence, tmp4);
> 
> Now that Koushik's JDK-8367008 has been integrated, there is no need to construct `tmp4`. Just call `tmp3.write(AlgorithmId.get(hmac))`. It's OK for this method to throw a NSAE.

Good idea. Fixed.

> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 353:
> 
>> 351:                     cipher.init(Cipher.DECRYPT_MODE, skey, algParams);
>> 352:                 } finally {
>> 353:                     sun.security.util.KeyUtil.destroySecretKeys(skey);
> 
> No need for the package name `sun.security.util.KeyUtil`. All classes have already been imported. The are several such cases in this file and one in `MacData`.

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 162:
> 
>> 160:         tmp0.putInteger(iterationCount);
>> 161:         tmp0.putInteger(keyLength);
>> 162:         tmp0.write(DerValue.tag_Sequence, tmp1);
> 
> After Koushik's JDK-8367008, no need for `tmp1`. This can be just `tmp0.write(new AlgorithmId(prf))`.

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 168:
> 
>> 166:         out.write(DerValue.tag_Sequence, tmp0);
>> 167: 
>> 168:         return out.toByteArray();
> 
> `out` is just a concatenation of OID and `tmp0`, which needs to be wrapped inside an ASN.1 SEQUENCE. The return value should be
> 
> new DerOutputStream().write(DerValue.tag_Sequence, out).toByteArray();
> 
> 
> This makes sure the output of `encode` is the same as the input of the `new PBKDF2Parameters()` constructor.
> 
> Once this is updated, the other places that call `encode` do not need to add the ASN.1 SEQUENCE wrapper.

fixed

> src/java.base/share/conf/security/java.security line 1344:
> 
>> 1342: 
>> 1343: # The algorithm used to calculate the optional MacData at the end of a PKCS12
>> 1344: # file. This can be any HmacPBE or PBEWith<mac> algorithm defined in the
> 
> I know `HmacPBE` has been like this from the beginning, but to be consistent and precise, suggest changing it to `HmacPBE<digest>`.

Good Idea. Fixed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461790972
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461793840
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461788435
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461788610
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461788819
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461793450
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461791388
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461788952
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461789192
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461789643
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461790340
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2461794211