On Thu, 23 Oct 2025 14:20:47 GMT, Weijun Wang <[email protected]> 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
