RFR: 8348732: SunJCE and SunPKCS11 have different PBE key encodings
Francisco Ferrari Bihurriet
fferrari at openjdk.org
Tue Mar 18 23:38:08 UTC 2025
On Fri, 14 Mar 2025 21:58:47 GMT, Valerie Peng <valeriep at openjdk.org> wrote:
> As part of [https://bugs.openjdk.org/browse/JDK-8301553](JDK-8301553), SunPKCS11 provider added support for PBE SecretKeyFactories for `HmacPBESHAxxx` and `PBEWithHmacSHAxxxAndAES_yyy`. These impls produce keys whose encoding contains the PBKDF2 derived bytes. Given that SunJCE provider have supported `PBEWithHmacSHAxxxAndAES_yyy` SecretKeyFactories whose key encoding is the password bytes for long time. Such difference may be very confusing, e.g. using the same KeySpec and same-name SecretKeyFactory (from different providers), the resulting keys have same algorithm and format but different encodings.
>
> Given that the `P11Mac` and `P11PBECipher` classes already do key derivation internally, these PKCS11 SecretKeyFactories aren't a must-have and are proposed to be removed. I've also aligned the com.sun.crypto.provider.PBEKey class with com.sun.crypto.provider.PPBKDF2KeyImpl class to switch to "UTF-8" when converting the char[] to byte[]. This is to accomodate unicode passwords and given that "UTF-8" encoding is same for ASCII characters, this change should not affect backward compatibility.
Hi @valeriepeng, I found code assuming `com.sun.crypto.provider.PBEKey` contains only ASCII, please find my suggestions in the review comments. I also added a suggestion for the tests, in order to increase the coverage in that regard.
All the patches are verified to cleanly apply on top of this PR branch, when copied through the GitHub's copy button and applied with `xclip -sel clip | git apply`.
src/java.base/share/classes/com/sun/crypto/provider/PBEKey.java line 70:
> 68: passwd = new char[0];
> 69: }
> 70: this.key = PBKDF2KeyImpl.getPasswordBytes(passwd);
This change requires a counterpart in `sun.security.util.PBEUtil`, where the encoded `byte[]` from `com.sun.crypto.provider.PBEKey` is converted back to `char[]`:
diff --git a/src/java.base/share/classes/sun/security/util/PBEUtil.java b/src/java.base/share/classes/sun/security/util/PBEUtil.java
index 12e71418bf4..996bc0059f8 100644
--- a/src/java.base/share/classes/sun/security/util/PBEUtil.java
+++ b/src/java.base/share/classes/sun/security/util/PBEUtil.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2023, Red Hat, Inc.
+ * Copyright (c) 2023, 2025, Red Hat, Inc.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,9 @@
package sun.security.util;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
+import java.nio.charset.StandardCharsets;
import java.security.AlgorithmParameters;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
@@ -195,10 +198,7 @@ public PBEKeySpec getPBEKeySpec(int blkSize, int keyLength, int opmode,
}
initialize(blkSize, opmode, iCountInit, saltInit, ivSpecInit,
random);
- passwdChars = new char[passwdBytes.length];
- for (int i = 0; i < passwdChars.length; i++) {
- passwdChars[i] = (char) (passwdBytes[i] & 0x7f);
- }
+ passwdChars = decodePassword(passwdBytes);
return new PBEKeySpec(passwdChars, salt, iCount, keyLength);
} finally {
// password char[] was cloned in PBEKeySpec constructor,
@@ -247,6 +247,19 @@ private static int check(int iCount)
}
}
+ /*
+ * Inverse of com.sun.crypto.provider.PBKDF2KeyImpl.getPasswordBytes
+ */
+ private static char[] decodePassword(byte[] passwdBytes) {
+ CharBuffer cb =
+ StandardCharsets.UTF_8.decode(ByteBuffer.wrap(passwdBytes));
+ int len = cb.limit();
+ char[] passwd = new char[len];
+ cb.get(passwd);
+ cb.clear().put(new char[len]);
+ return passwd;
+ }
+
/*
* Obtain a PBEKeySpec for Mac services, after key and parameters
* validation. Used by HmacPKCS12PBECore (SunJCE) and P11Mac (SunPKCS11).
@@ -267,10 +280,7 @@ public static PBEKeySpec getPBAKeySpec(Key key,
(passwdBytes = key.getEncoded()) == null) {
throw new InvalidKeyException("Missing password");
}
- passwdChars = new char[passwdBytes.length];
- for (int i = 0; i < passwdChars.length; i++) {
- passwdChars[i] = (char) (passwdBytes[i] & 0x7f);
- }
+ passwdChars = decodePassword(passwdBytes);
Arrays.fill(passwdBytes, (byte)0x00);
} else {
throw new InvalidKeyException("SecretKey of PBE type required");
test/jdk/sun/security/pkcs11/Cipher/PBECipher.java line 1:
> 1: /*
I suggest changing this test's password to contain non-ASCII characters, so we have a better coverage in both _SunJCE_ (when checking the assertion data) and _SunPKCS11_ (when doing the actual test):
diff --git a/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java b/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java
index b12cb5c2687..93ef097933f 100644
--- a/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java
+++ b/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2023, Red Hat, Inc.
+ * Copyright (c) 2023, 2025, Red Hat, Inc.
*
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
@@ -47,7 +47,7 @@
*/
public final class PBECipher extends PKCS11Test {
- private static final char[] password = "123456".toCharArray();
+ private static final char[] password = "123456\uA4F7".toCharArray();
private static final byte[] salt = "abcdefgh".getBytes(
StandardCharsets.UTF_8);
private static final int iterations = 1000;
@@ -112,35 +112,35 @@ private static void checkAssertionValues(BigInteger expectedValue,
// Generated with SunJCE.
private static final AssertionData[] assertionData = new AssertionData[]{
cipherAssertionData("PBEWithHmacSHA1AndAES_128",
- "AES/CBC/PKCS5Padding", "ba1c9614d550912925d99e0bc8969032" +
- "7ac6258b72117dcf750c19ee6ca73dd4"),
+ "AES/CBC/PKCS5Padding", "9e097796e8d8224f2a7f5c950677d879" +
+ "c0c578340147c7ae357550e2f4d4c6ce"),
cipherAssertionData("PBEWithHmacSHA224AndAES_128",
- "AES/CBC/PKCS5Padding", "41960c43ca99cf2184511aaf2f0508a9" +
- "7da3762ee6c2b7e2027c8076811f2e52"),
+ "AES/CBC/PKCS5Padding", "7b915941d8e3a87c00e2fbd8ad67a578" +
+ "9a25648933b737706de4e4d48bdb61b6"),
cipherAssertionData("PBEWithHmacSHA256AndAES_128",
- "AES/CBC/PKCS5Padding", "6bb6a3dc3834e81e5ca6b5e70073ff46" +
- "903b188940a269ed26db2ffe622b8e16"),
+ "AES/CBC/PKCS5Padding", "c23912d15599908f47cc32c9da56b37f" +
+ "e41e958e9c3a6c6e4e631a2a9e6cd20f"),
cipherAssertionData("PBEWithHmacSHA384AndAES_128",
- "AES/CBC/PKCS5Padding", "22aabf7a6a059415dc4ca7d985f3de06" +
- "8f8300ca48d8de585d802670f4c1d9bd"),
+ "AES/CBC/PKCS5Padding", "f05c6b2dea545d59f2a6fde845170dd6" +
+ "7aebd6b1cc28904699d7dcff1a0a238c"),
cipherAssertionData("PBEWithHmacSHA512AndAES_128",
- "AES/CBC/PKCS5Padding", "b523e7c462a0b7fd74e492b3a6550464" +
- "ceebe81f08649ae163673afc242ad8a2"),
+ "AES/CBC/PKCS5Padding", "949c0c01a29375b9d421f6e2bf6ed0d7" +
+ "15a118e0980494797d3a3b799b67daf6"),
cipherAssertionData("PBEWithHmacSHA1AndAES_256",
- "AES/CBC/PKCS5Padding", "1e7c25e166afae069cec68ef9affca61" +
- "aea02ab1c3dc7471cb767ed7d6e37af0"),
+ "AES/CBC/PKCS5Padding", "7bd686b15bc09e5fb5aa1f881c92aa5a" +
+ "e72bdcd864c74e62395b9aaea7443bcd"),
cipherAssertionData("PBEWithHmacSHA224AndAES_256",
- "AES/CBC/PKCS5Padding", "6701f1cc75b6494ec4bd27158aa2c15d" +
- "7d10bc2f1fbb7d92d8277c7edfd1dd57"),
+ "AES/CBC/PKCS5Padding", "df58a1b26cca7e9e297da61ada03ddc4" +
+ "39d2a5699753433f19891de33f8741a2"),
cipherAssertionData("PBEWithHmacSHA256AndAES_256",
- "AES/CBC/PKCS5Padding", "f82eb2fc016505baeb23ecdf85163933" +
- "5e8d6d48b48631185641febb75898a1d"),
+ "AES/CBC/PKCS5Padding", "f6ae5a15ec2c18eaa25927858f1da990" +
+ "6df58a3b4830dbaaaa4c4317e53d717d"),
cipherAssertionData("PBEWithHmacSHA384AndAES_256",
- "AES/CBC/PKCS5Padding", "ee9528022e58cdd9be80cd88443e03b3" +
- "de13376cf97c53d946d5c5dfc88097be"),
+ "AES/CBC/PKCS5Padding", "5795625f51ec701594506944e5ed79f0" +
+ "c9d8e82319762f00f8ff06a8b6195ac4"),
cipherAssertionData("PBEWithHmacSHA512AndAES_256",
- "AES/CBC/PKCS5Padding", "18f472912ffaa31824e20a5486324e14" +
- "0225e20cb158762e8647b1216fe0ab7e"),
+ "AES/CBC/PKCS5Padding", "ddf55933f80f42f2a8d4e8726290766e" +
+ "024f225b76b594e8005c00227d553d05"),
};
private static final class NoRandom extends SecureRandom {
When we do this, it reveals that this test also needs adjusting the anonymous `PBEKey` encoding, to do UTF-8 encoding as `com.sun.crypto.provider.PBEKey`. Instead of duplicating the logic from `com.sun.crypto.provider.PBKDF2KeyImpl.getPasswordBytes()`, we can just use `com.sun.crypto.provider.PBEKey.getEncoded()`:
diff --git a/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java b/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java
index 147fd9a65f5..b12cb5c2687 100644
--- a/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java
+++ b/test/jdk/sun/security/pkcs11/Cipher/PBECipher.java
@@ -214,23 +214,16 @@ private static SecretKey getPasswordOnlyPBEKey()
}
private static SecretKey getAnonymousPBEKey(String algorithm,
- boolean isPbeCipherSvc) {
+ boolean isPbeCipherSvc) throws GeneralSecurityException {
+ byte[] encodedKey =
+ isPbeCipherSvc ? getPasswordOnlyPBEKey().getEncoded() : null;
return new PBEKey() {
public byte[] getSalt() { return salt.clone(); }
public int getIterationCount() { return iterations; }
public String getAlgorithm() { return algorithm; }
public String getFormat() { return "RAW"; }
public char[] getPassword() { return password.clone(); }
- public byte[] getEncoded() {
- byte[] encodedKey = null;
- if (isPbeCipherSvc) {
- encodedKey = new byte[password.length];
- for (int i = 0; i < password.length; i++) {
- encodedKey[i] = (byte) (password[i] & 0x7f);
- }
- }
- return encodedKey;
- }
+ public byte[] getEncoded() { return encodedKey; }
};
}
test/jdk/sun/security/pkcs11/Mac/PBAMac.java line 1:
> 1: /*
I suggest changing this test's password to contain non-ASCII characters, so we have a better coverage in both _SunJCE_ (when checking the assertion data) and _SunPKCS11_ (when doing the actual test):
diff --git a/test/jdk/sun/security/pkcs11/Mac/PBAMac.java b/test/jdk/sun/security/pkcs11/Mac/PBAMac.java
index b70a0a6d618..0baf85bb5de 100644
--- a/test/jdk/sun/security/pkcs11/Mac/PBAMac.java
+++ b/test/jdk/sun/security/pkcs11/Mac/PBAMac.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2023, Red Hat, Inc.
+ * Copyright (c) 2023, 2025, Red Hat, Inc.
*
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
@@ -44,7 +44,7 @@
*/
public final class PBAMac extends PKCS11Test {
- private static final char[] password = "123456".toCharArray();
+ private static final char[] password = "123456\uA4F7".toCharArray();
private static final byte[] salt = "abcdefgh".getBytes(
StandardCharsets.UTF_8);
private static final int iterations = 1000;
@@ -102,19 +102,19 @@ private static void checkAssertionValues(BigInteger expectedValue,
// Generated with SunJCE.
private static final AssertionData[] assertionData = new AssertionData[]{
macAssertionData("HmacPBESHA1", "HmacSHA1",
- "707606929395e4297adc63d520ac7d22f3f5fa66"),
+ "8611414ddb1875d9f576282199ab492a802b7d49"),
macAssertionData("HmacPBESHA224", "HmacSHA224",
- "4ffb5ad4974a7a9fca5a36ebe3e34dd443c07fb68c392f8b611657e6"),
+ "cebb12b48eb90c07336c695f771d1d0ef4ccf5b9524fc0ab6fb9813a"),
macAssertionData("HmacPBESHA256", "HmacSHA256",
- "9e8c102c212d2fd1334dc497acb4e002b04e84713b7eda5a63807af2" +
- "989d3e50"),
+ "d83a6a4e8b0e1ec939d05790f385dd774bd2b7c17cfa2dd004efc894" +
+ "e5d53f51"),
macAssertionData("HmacPBESHA384", "HmacSHA384",
- "77f31a785d4f2220251143a4ba80f5610d9d0aeaebb4a278b8a7535c" +
- "8cea8e8211809ba450458e351c5b66d691839c23"),
+ "ae6b69cf9edfd9cd8c3b51cdf2b0243502f35a3e6007f33b1ab73568" +
+ "2ea81ea562f4383bb9512ff70752367b7259b16f"),
macAssertionData("HmacPBESHA512", "HmacSHA512",
- "a53f942a844b234a69c1f92cba20ef272c4394a3cf4024dc16d9dbac" +
- "1969870b1c2b28b897149a1a3b9ad80a7ca8c547dfabf3ed5f144c6b" +
- "593900b62e120c45"),
+ "46f6d09b0e7e50a66fa559ea4c4e9737a9d9e258b94f0075230d0acb" +
+ "40f2c926f96a152c4f6b03b631efc7f99c84f052f1c78d79e07f2a9e" +
+ "4a96164f5b46e70b"),
};
public void main(Provider sunPKCS11) throws Exception {
-------------
Changes requested by fferrari (Author).
PR Review: https://git.openjdk.org/jdk/pull/24068#pullrequestreview-2696475636
PR Review Comment: https://git.openjdk.org/jdk/pull/24068#discussion_r2002127839
PR Review Comment: https://git.openjdk.org/jdk/pull/24068#discussion_r2002148875
PR Review Comment: https://git.openjdk.org/jdk/pull/24068#discussion_r2002152377
More information about the security-dev
mailing list