Skip to content

8359388: Stricter checking for cipher transformations #25808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+84 −52
Open

8359388: Stricter checking for cipher transformations #25808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
245bf36
8359388: Stricter checking for cipher transformations
valeriepeng Jun 13, 2025
8d8640c
Updated test per Mikhail's review comments.
valeriepeng Jun 16, 2025
0cb4198
remove the extra ";" as suggested
valeriepeng Jun 18, 2025
7d9e36b
Updated test per 2nd round of Mikhail's review comments.
valeriepeng Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 36 additions & 34 deletions src/java.base/share/classes/javax/crypto/Cipher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,19 +316,22 @@ private Cipher(CipherSpi firstSpi, Service firstService,

private static final String SHA512TRUNCATED = "SHA512/2";

// Parse the specified cipher transformation for algorithm and the
// optional mode and padding. If the transformation contains only
// algorithm, then only algorithm is returned. Otherwise, the
// transformation must contain all 3 and they must be non-empty.
private static String[] tokenizeTransformation(String transformation)
throws NoSuchAlgorithmException {
if (transformation == null) {
throw new NoSuchAlgorithmException("No transformation given");
}
/*
* array containing the components of a cipher transformation:
* Components of a cipher transformation:
*
* index 0: algorithm component (e.g., AES)
* index 1: feedback component (e.g., CFB)
* index 2: padding component (e.g., PKCS5Padding)
* 1) algorithm component (e.g., AES)
* 2) feedback component (e.g., CFB) - optional
* 3) padding component (e.g., PKCS5Padding) - optional
*/
String[] parts = { "", "", "" };

// check if the transformation contains algorithms with "/" in their
// name which can cause the parsing logic to go wrong
Expand All @@ -337,27 +340,35 @@ private static String[] tokenizeTransformation(String transformation)
int startIdx = (sha512Idx == -1 ? 0 :
sha512Idx + SHA512TRUNCATED.length());
int endIdx = transformation.indexOf('/', startIdx);
if (endIdx == -1) {
// algorithm
parts[0] = transformation.trim();

boolean algorithmOnly = (endIdx == -1);
String algo = (algorithmOnly ? transformation.trim() :
transformation.substring(0, endIdx).trim());
if (algo.isEmpty()) {
throw new NoSuchAlgorithmException("Invalid transformation: " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this exception is tested. It could be by adding test(" ", provider); in test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java on line 68

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can add it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is covered now

"algorithm not specified-"
+ transformation);
}
if (algorithmOnly) { // done
return new String[] { algo };
} else {
// algorithm/mode/padding
parts[0] = transformation.substring(0, endIdx).trim();
// continue parsing mode and padding
startIdx = endIdx+1;
endIdx = transformation.indexOf('/', startIdx);
if (endIdx == -1) {
throw new NoSuchAlgorithmException("Invalid transformation"
+ " format:" + transformation);
}
parts[1] = transformation.substring(startIdx, endIdx).trim();
parts[2] = transformation.substring(endIdx+1).trim();
}
if (parts[0].isEmpty()) {
throw new NoSuchAlgorithmException("Invalid transformation: " +
"algorithm not specified-"
String mode = transformation.substring(startIdx, endIdx).trim();
String padding = transformation.substring(endIdx+1).trim();
// ensure mode and padding are specified
if (mode.isEmpty() || padding.isEmpty()) {
throw new NoSuchAlgorithmException("Invalid transformation: " +
"missing mode and/or padding-"
+ transformation);
}
return new String[] { algo, mode, padding };
}
return parts;
}

// Provider attribute name for supported chaining mode
Expand Down Expand Up @@ -453,27 +464,18 @@ private static List<Transform> getTransforms(String transformation)
throws NoSuchAlgorithmException {
String[] parts = tokenizeTransformation(transformation);

String alg = parts[0];
String mode = (parts[1].length() == 0 ? null : parts[1]);
String pad = (parts[2].length() == 0 ? null : parts[2]);

if ((mode == null) && (pad == null)) {
if (parts.length == 1) {
// Algorithm only
Transform tr = new Transform(alg, "", null, null);
Transform tr = new Transform(parts[0], "", null, null);
return Collections.singletonList(tr);
} else {
// Algorithm w/ at least mode or padding or both
// Algorithm w/ both mode and padding
List<Transform> list = new ArrayList<>(4);
if ((mode != null) && (pad != null)) {
list.add(new Transform(alg, "/" + mode + "/" + pad, null, null));
}
if (mode != null) {
list.add(new Transform(alg, "/" + mode, null, pad));
}
if (pad != null) {
list.add(new Transform(alg, "//" + pad, mode, null));
}
list.add(new Transform(alg, "", mode, pad));
list.add(new Transform(parts[0], "/" + parts[1] + "/" + parts[2],
null, null));
list.add(new Transform(parts[0], "/" + parts[1], null, parts[2]));
list.add(new Transform(parts[0], "//" + parts[2], parts[1], null));
list.add(new Transform(parts[0], "", parts[1], parts[2]));
return list;
}
}
Expand Down
66 changes: 48 additions & 18 deletions test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,33 +24,63 @@

/*
* @test
* @bug 8358159
* @summary test that the Cipher.getInstance() handles
* transformations with empty mode and/or padding
* @run main TestEmptyModePadding
* @bug 8358159 8359388
* @summary test that the Cipher.getInstance() would reject improper
* transformations with empty mode and/or padding.
*/


import java.security.*;
import javax.crypto.*;
import java.security.NoSuchAlgorithmException;
import java.security.Provider;
import java.security.Security;
import javax.crypto.Cipher;

public class TestEmptyModePadding {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please change the imports to not use wildcard imports

import java.security.NoSuchAlgorithmException;
import java.security.Provider;
import java.security.Security;
import javax.crypto.Cipher;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do this.


public static void main(String[] args) throws Exception {
Provider provider = Security.getProvider(System.getProperty("test.provider.name", "SunJCE"));
Provider provider = Security.getProvider(
System.getProperty("test.provider.name", "SunJCE"));

System.out.println("Testing against " + provider.getName());

test("AES", provider);
test("AES/ECB/PKCS5Padding", provider);
test("AES//PKCS5Padding", provider); // Empty mode
test("AES/CBC/", provider); // Empty padding
test("AES/ /NoPadding", provider); // Mode is a space
test("AES/CBC/ ", provider); // Padding is a space
test("AES/ / ", provider); // Both mode and padding are spaces
test("AES//", provider); // Both mode and padding are missing
String[] testTransformations = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: Do you think it would be easier to read if each entry was a separate line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok either way, sounds like you prefer one line per entry. I can change them.

// transformations w/ only 1 component, i.e. algo
" ",
// transformations w/ only 2 components
"AES/",
"AES/ ",
"AES/CBC",
"PBEWithHmacSHA512/224AndAES_128/",
"PBEWithHmacSHA512/256AndAES_128/ ",
"PBEWithHmacSHA512/224AndAES_128/CBC",
// 3-component transformations w/ empty component(s)
"AES//",
"AES/ /",
"AES// ",
"AES/ / ",
"AES/CBC/", "AES/CBC/ ",
"AES//PKCS5Padding", "AES/ /NoPadding",
"PBEWithHmacSHA512/224AndAES_128//",
"PBEWithHmacSHA512/224AndAES_128/ /",
"PBEWithHmacSHA512/224AndAES_128// ",
"PBEWithHmacSHA512/224AndAES_128/ / ",
"PBEWithHmacSHA512/256AndAES_128/CBC/",
"PBEWithHmacSHA512/256AndAES_128/CBC/ ",
"PBEWithHmacSHA512/256AndAES_128//PKCS5Padding",
"PBEWithHmacSHA512/256AndAES_128/ /PKCS5Padding",
};

for (String t : testTransformations) {
test(t, provider);
}
}

private static void test(String transformation, Provider provider) throws Exception {
Cipher c = Cipher.getInstance(transformation, provider);
private static void test(String t, Provider p) throws Exception {
try {
Cipher c = Cipher.getInstance(t, p);
throw new RuntimeException("Should throw NSAE for \'" + t + "\'");
} catch (NoSuchAlgorithmException nsae) {
// transformation info is already in the NSAE message
System.out.println("Expected NSAE: " + nsae.getMessage());
}
}
}