On Thu, 14 May 2026 04:44:05 GMT, Anthony Scarpino <[email protected]> wrote:
>> Please review the finalized PEM API at https://openjdk.org/jeps/8376991. The >> most significant changes from the second preview, JEP 524 >> (https://openjdk.org/jeps/524), include: >> >> - The `PEM` class is now an ordinary class rather than a record. It adds >> Binary-encoded content constructors and data is defensively copied. >> - The `DEREncodable` interface is renamed to `BinaryEncodable` to more >> accurately reflect the binary data stored in PEM text. >> - In `EncryptedPrivateKeyInfo`, the `encrypt` methods now accept >> `BinaryEncodable`, and the `getKey()` and `getKeyPair()` methods no longer >> include a `Provider` parameter. >> - A new `CryptoException` class indicates failures in cryptographic >> processing at runtime. >> >> thanks >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > comments src/java.base/share/classes/java/security/PEM.java line 167: > 165: this.type = type; > 166: final var c = content; > 167: CleanerFactory.cleaner().register(this, this::clear); Not a `Cleaner` expert, but I heard `this` cannot be referenced in the cleaner action. src/java.base/share/classes/java/security/PEM.java line 202: > 200: * {@link Base64#getMimeDecoder()}. > 201: * > 202: * @return the Base64-decoded content Suggestion: return a copy of... src/java.base/share/classes/java/security/PEMDecoder.java line 434: > 432: try { > 433: so = decode(pem); > 434: } finally { I realized we still need a `so == pem` check here. What if user calls this method with `BinaryEncodable.class` and `decode(pem)` returns a `PEM`? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 379: > 377: Objects.requireNonNull(password, "a password must be specified"); > 378: Objects.requireNonNull(algorithm, "an algorithm must be > specified"); > 379: char[] passwd = password.clone(); Why clone here? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 526: > 524: PBEKeySpec keySpec = new PBEKeySpec(password); > 525: try { > 526: return PKCS8Key.parseKey(Pem.decryptEncoding(this, keySpec), > null); The result of `Pem.decryptEncoding` should be cleared. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 579: > 577: BinaryEncodable d; > 578: try { > 579: d = Pem.toPKCS8Encodable(Pem.decryptEncoding(this, keySpec), > null); The result of `Pem.decryptEncoding` should be cleared. src/java.base/share/classes/sun/security/util/Pem.java line 320: > 318: } while (hyphen < 5); > 319: > 320: while ((c = is.read()) != eol && c != -1 && c != '\s' && c > != '\t') { This is different from reading the header. Space and tab are not skipped, instead, they end the loop. This means a block ending with `-----END SOMETHING------ xyz` is treated as valid. src/java.base/share/classes/sun/security/util/Pem.java line 377: > 375: > 376: /** > 377: * Return a PEM encoding with the given type and base64 data in a > String. This method is only used in a test. src/java.base/share/classes/sun/security/util/Pem.java line 395: > 393: .getBytes(StandardCharsets.ISO_8859_1); > 394: > 395: int crlfLen = (base64.length == 0 || Why is CRLF needed if `base64` is empty? src/java.base/share/classes/sun/security/util/Pem.java line 419: > 417: */ > 418: public static byte[] pemEncoded(PEM pem) { > 419: return pemEncoded(pem.type(), pem.content()); `pem.content()` here and on line 427 is leaked. Since these methods are only used in `PEM` and `PEMEncoder`, we can move it back into `PEM` as a package-private method without cloning the content. src/java.base/share/classes/sun/security/util/Pem.java line 481: > 479: try { > 480: p8KeySpec = new PKCS8EncodedKeySpec(encoded); > 481: } catch (NullPointerException e) { NPE should not happen. It's already used to create `p8key`. src/java.base/share/classes/sun/security/util/Pem.java line 507: > 505: // In case decode() could not read the public key, the > 506: // KeyFactory can try. Failure is ok as there may not > 507: // be a public key in the encoding. Will this ever happen? It seems `new PKCS8Key(encoded)` can always find the public key even if it's inside a SEC1v2 format. src/java.base/share/classes/sun/security/util/Pem.java line 526: > 524: > 525: private static boolean matchesAt(byte[] source, int offset, byte[] > match) { > 526: if (offset < 0) { Will this happen? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243324881 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243331896 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243339155 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243341221 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243343758 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243344820 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3243971475 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244153423 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244156391 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244187476 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244212330 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244236240 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244291357
