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

Thu Oct 16 02:41:25 UTC 2025

On Wed, 15 Oct 2025 14:21:43 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Mark Powers has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   remaining comments
>
> src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 251:
> 
>> 249:         keysize = kdfParams.getKeyLength();
>> 250: 
>> 251:         if (pBES2_params.tag != DerValue.tag_Sequence) {
> 
> You can probably move this check before line 228. Note that the same check has been performed on line 214 and the only reason to do it again is if `pBES2_params` has been reassigned on line 227.

fixed

> src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 308:
> 
>> 306:         DerOutputStream pBES2_params = new DerOutputStream();
>> 307:         pBES2_params.write(DerValue.tag_Sequence,
>> 308:                 PBKDF2Parameters.encode(salt, iCount, keysize, kdfAlgo_OID));
> 
> `keySize` here is number of bits, but the `PBKDF2Parameters.encode` requires number of bytes. This means a newly created PKCS12 keystore is invalid. When Java loads a PKCS12 keystore, this field is read but not validated. On the other hand, openssl validates it and will report an error.
> 
> Suggestion: either always use the same style in all fields and method arguments, or name them precisely.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 65:
> 
>> 63:  * }
>> 64:  *
>> 65:  * </pre>
> 
> Please move the `@author` line to the end of the javadoc comment.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 135:
> 
>> 133:     }
>> 134: 
>> 135:     private static Mac getMac(String macAlgorithm, char[] password,
> 
> I would rather let this method return `mac.doFinal(data)`. The method cleans up `pbeKey` in a finally block and I somehow cannot guarantee it would not have any negative impact when the `Mac` object is still alive.
> 
> There are two places this method is called and the `Mac` object is used for other purposes. For one, its `getAlgorithm` is called and used in a debug log. I think the `macAlgorithm` can be used too. The other called `getMacLength` but that is the same of the length of the `doFinal` output.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 147:
> 
>> 145:         if (macAlgorithm.startsWith("pbewith")) {
>> 146:             m = Mac.getInstance(hmac);
>> 147:             int len = keyLength == 0 ? m.getMacLength()*8 : keyLength;
> 
> Somehow I prefer to use `-1` for the default case. This is consistent with `PBKDF2Parameters`.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 152:
> 
>> 150:                     params.getIterationCount(), len);
>> 151:             pbeKey = skf.generateSecret(new PBEKeySpec(password,
>> 152:                     params.getSalt(), params.getIterationCount(), len));
> 
> You already created a variable named `keySpec`. Do not create another exact same `PBEKeySpec` object.

It also caused the leak of uncleaned memory that was seen during testing. Fixed.

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 155:
> 
>> 153:         } else {
>> 154:             hmac = macAlgorithm;
>> 155:             m = Mac.getInstance(hmac);
> 
> Don't assign to `hmac` and just call `Mac.getInstance(macAlgorithm)`. Mutating `hmac` makes people wondering if it has a purpose.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 208:
> 
>> 206:             String macAlgorithm, int macIterationCount, byte[] salt)
>> 207:             throws IOException, NoSuchAlgorithmException {
>> 208:         final byte[] mData;
> 
> No need for `mData`. Just `return bytes.toByteArray()` at the end.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 210:
> 
>> 208:         final byte[] mData;
>> 209:         final PBEParameterSpec params;
>> 210:         String algName = "PBMAC1";
> 
> No need to assign a value now. Assign it in the `if (macAlgorithm.startsWith("pbewith"))` block.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 214:
> 
>> 212:         String hmac;
>> 213:         Mac m;
>> 214:         int keyLength;
> 
> Useless.

removed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 225:
> 
>> 223:         } else if (macAlgorithm.startsWith("hmacpbe")) {
>> 224:             algName = macAlgorithm.substring(7);
>> 225:             kdfHmac = macAlgorithm;
> 
> Assign null to `kdfHmac`. People might wonder it's useful in this case.

fixed

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 240:
> 
>> 238:             DerOutputStream bytes = new DerOutputStream();
>> 239:             bytes.write(encode(algName, macResult, params, kdfHmac, hmac,
>> 240:                     m.getMacLength()));
> 
> `m.getLength()` is the same as `macResult`.

fixed

> src/java.base/share/classes/sun/security/pkcs12/PBMAC1Parameters.java line 99:
> 
>> 97:         this.hmacAlgo = o.stdName();
>> 98: 
>> 99:         DerValue kdf = pBMAC1_params.data.getDerValue();
> 
> This is just `info[0]`. You have already read all sub-components on line 77.

fixed

> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 2074:
> 
>> 2072:                 MacData macData = new MacData(s);
>> 2073:                 int ic = macData.getIterations();
>> 2074:                 byte[] salt = macData.getSalt();
> 
> This is useless.

removed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 80:
> 
>> 78:     private final int keyLength;
>> 79: 
>> 80:     private String prfAlgo;
> 
> This can be final.

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 86:
> 
>> 84:      * parameter block.
>> 85:      * 
>> 86:      * @param keyDerivationFunc the DER encoding of the parameter block
> 
> The `@param` name should be `pBKDF2_params`.

changed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 152:
> 
>> 150:                         .putOctetString(salt)
>> 151:                         .putInteger(iterationCount)
>> 152:                         .putInteger(keyLength)
> 
> Currently, when this method is called, `keyLength` can never be -1. Please add an `assert` statement so it will not be accidentally called later with a length being -1.

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434402411
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434402543
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434402294
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434402161
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434402031
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434402657
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401928
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401499
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401788
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401352
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401623
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401266
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401136
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434401047
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434400938
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434400777
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2434400633