On Wed, 22 Oct 2025 22:02:50 GMT, Mark Powers <[email protected]> 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
