RFR: 8343232: PKCS#12 KeyStore support for RFC 9879: Use of Password-Based Message Authentication Code 1 (PBMAC1) [v14]
Weijun Wang
weijun at openjdk.org
Thu Oct 23 14:36:46 UTC 2025
On Wed, 22 Oct 2025 22:02:50 GMT, Mark Powers <mpowers at openjdk.org> wrote:
>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232)
>
> 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
Some comments.
src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 244:
> 242: }
> 243: DerValue pBKDF2_params = kdf.data.getDerValue();
> 244: if (pBKDF2_params.tag != DerValue.tag_Sequence) {
This check seems like it should belong to the `PBKDF2Parameters` constructor.
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));
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).
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 77:
> 75: private final int keyLength;
> 76: private final String kdfHmac;
> 77: private final String hmac;
Add a comment that the 3 fields above are only for PBMAC1.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 135:
> 133: }
> 134:
> 135: private static byte[] getMac(String macAlgorithm, char[] password,
Why the difference in the names `getMac` and `calculateMac`?
This method is the basic one for calculation, and `processMacData` is verification and `calculateMac` is generation. Add more comments.
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.
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`.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 248:
> 246: }
> 247:
> 248: byte[] getSalt() {
This method is not used anywhere.
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".
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));
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.
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`.
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))`.
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.
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>`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24429#pullrequestreview-3370108106
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455246107
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455321557
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455341868
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455250023
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455260394
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455262798
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455266129
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455267245
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455333333
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455324888
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455284291
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455287363
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455295271
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455313609
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2455352943
More information about the security-dev
mailing list