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

[Thomas Fitzsimmons]

On Tue, 7 Oct 2025 20:33:24 GMT, Mark Powers <mpowers at openjdk.org> wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 173:
>> 
>>> 171:         if (keyLength > 0) {
>>> 172:             pBKDF2_params.putInteger(keyLength / 8); // derived key length (in octets)
>>> 173:         }
>> 
>> I think `keyLength` is a MUST here.  Maybe this should instead check if `keyLength` is `<= 0`, and if so, throw an exception.  Then proceed to encode `keyLength` unconditionally.
>
> After innumerable iterations, this encoding method will be removed from `PBMAC1Parameters`.
> That said, keyLength now comes from the Mac before being encoded in `MacData`:
> 
> 
> Mac m = Mac.getInstance(hmac);
> int keyLength = m.getMacLength()*8;
> 
> If this doesn't work, we have some other serious issues.

The `> 0` check is still there in MacData.getEncoded:


        if (this.pbmac1Keystore) {
        [...]
            // encode derived key length
            if (this.keyLength > 0) {
                pBKDF2_params.putInteger(this.keyLength / 8); // derived key length (in octets)
            }


How about making sure it is greater than zero in the constructor instead, something like:


--- a/src/java.base/share/classes/sun/security/pkcs12/MacData.java
+++ b/src/java.base/share/classes/sun/security/pkcs12/MacData.java
@@ -122,6 +122,10 @@ class MacData {
         }
         if (algName.equals("PBMAC1")) {
             this.pbmac1Keystore = true;
+            if (keyLength <= 0) {
+                throw new IllegalArgumentException("the keyLength " +
+                        "parameter for PBMAC1 must be greater than 0");
+            }
         }
         algid = AlgorithmId.get(algName);
 
@@ -333,9 +337,7 @@ byte[] getEncoded() throws NoSuchAlgorithmException, IOException {
             pBKDF2_params.putInteger(this.iterations);
 
             // encode derived key length
-            if (this.keyLength > 0) {
-                pBKDF2_params.putInteger(this.keyLength / 8); // derived key length (in octets)
-            }
+            pBKDF2_params.putInteger(this.keyLength / 8); // derived key length (in octets)
             pBKDF2_params.write(DerValue.tag_Sequence, kdfHmac);
             tmp3.putOID(pkcs5PBKDF2_OID);
             tmp3.write(DerValue.tag_Sequence, pBKDF2_params);


Should `keyLength` also be initialized in the `MacData(DerInputStream derin)` constructor?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2412170819