Skip to content

Commit ec7c6be

Browse files
author
Valerie Peng
committed
8359388: Stricter checking for cipher transformations
Reviewed-by: mullan
1 parent 197fde5 commit ec7c6be

File tree

2 files changed

+85
-55
lines changed

2 files changed

+85
-55
lines changed

‎src/java.base/share/classes/javax/crypto/Cipher.java

Lines changed: 37 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -316,19 +316,22 @@ private Cipher(CipherSpi firstSpi, Service firstService,
316316

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

319+
// Parse the specified cipher transformation for algorithm and the
320+
// optional mode and padding. If the transformation contains only
321+
// algorithm, then only algorithm is returned. Otherwise, the
322+
// transformation must contain all 3 and they must be non-empty.
319323
private static String[] tokenizeTransformation(String transformation)
320324
throws NoSuchAlgorithmException {
321325
if (transformation == null) {
322326
throw new NoSuchAlgorithmException("No transformation given");
323327
}
324328
/*
325-
* array containing the components of a cipher transformation:
329+
* Components of a cipher transformation:
326330
*
327-
* index 0: algorithm component (e.g., AES)
328-
* index 1: feedback component (e.g., CFB)
329-
* index 2: padding component (e.g., PKCS5Padding)
331+
* 1) algorithm component (e.g., AES)
332+
* 2) feedback component (e.g., CFB) - optional
333+
* 3) padding component (e.g., PKCS5Padding) - optional
330334
*/
331-
String[] parts = { "", "", "" };
332335

333336
// check if the transformation contains algorithms with "/" in their
334337
// name which can cause the parsing logic to go wrong
@@ -337,27 +340,35 @@ private static String[] tokenizeTransformation(String transformation)
337340
int startIdx = (sha512Idx == -1 ? 0 :
338341
sha512Idx + SHA512TRUNCATED.length());
339342
int endIdx = transformation.indexOf('/', startIdx);
340-
if (endIdx == -1) {
341-
// algorithm
342-
parts[0] = transformation.trim();
343+
344+
boolean algorithmOnly = (endIdx == -1);
345+
String algo = (algorithmOnly ? transformation.trim() :
346+
transformation.substring(0, endIdx).trim());
347+
if (algo.isEmpty()) {
348+
throw new NoSuchAlgorithmException("Invalid transformation: " +
349+
"algorithm not specified-"
350+
+ transformation);
351+
}
352+
if (algorithmOnly) { // done
353+
return new String[] { algo };
343354
} else {
344-
// algorithm/mode/padding
345-
parts[0] = transformation.substring(0, endIdx).trim();
355+
// continue parsing mode and padding
346356
startIdx = endIdx+1;
347357
endIdx = transformation.indexOf('/', startIdx);
348358
if (endIdx == -1) {
349359
throw new NoSuchAlgorithmException("Invalid transformation"
350360
+ " format:" + transformation);
351361
}
352-
parts[1] = transformation.substring(startIdx, endIdx).trim();
353-
parts[2] = transformation.substring(endIdx+1).trim();
354-
}
355-
if (parts[0].isEmpty()) {
356-
throw new NoSuchAlgorithmException("Invalid transformation: " +
357-
"algorithm not specified-"
362+
String mode = transformation.substring(startIdx, endIdx).trim();
363+
String padding = transformation.substring(endIdx+1).trim();
364+
// ensure mode and padding are specified
365+
if (mode.isEmpty() || padding.isEmpty()) {
366+
throw new NoSuchAlgorithmException("Invalid transformation: " +
367+
"missing mode and/or padding-"
358368
+ transformation);
369+
}
370+
return new String[] { algo, mode, padding };
359371
}
360-
return parts;
361372
}
362373

363374
// Provider attribute name for supported chaining mode
@@ -453,28 +464,17 @@ private static List<Transform> getTransforms(String transformation)
453464
throws NoSuchAlgorithmException {
454465
String[] parts = tokenizeTransformation(transformation);
455466

456-
String alg = parts[0];
457-
String mode = (parts[1].length() == 0 ? null : parts[1]);
458-
String pad = (parts[2].length() == 0 ? null : parts[2]);
459-
460-
if ((mode == null) && (pad == null)) {
467+
if (parts.length == 1) {
461468
// Algorithm only
462-
Transform tr = new Transform(alg, "", null, null);
463-
return Collections.singletonList(tr);
469+
return List.of(new Transform(parts[0], "", null, null));
464470
} else {
465-
// Algorithm w/ at least mode or padding or both
466-
List<Transform> list = new ArrayList<>(4);
467-
if ((mode != null) && (pad != null)) {
468-
list.add(new Transform(alg, "/" + mode + "/" + pad, null, null));
469-
}
470-
if (mode != null) {
471-
list.add(new Transform(alg, "/" + mode, null, pad));
472-
}
473-
if (pad != null) {
474-
list.add(new Transform(alg, "//" + pad, mode, null));
475-
}
476-
list.add(new Transform(alg, "", mode, pad));
477-
return list;
471+
// Algorithm w/ both mode and padding
472+
return List.of(
473+
new Transform(parts[0], "/" + parts[1] + "/" + parts[2],
474+
null, null),
475+
new Transform(parts[0], "/" + parts[1], null, parts[2]),
476+
new Transform(parts[0], "//" + parts[2], parts[1], null),
477+
new Transform(parts[0], "", parts[1], parts[2]));
478478
}
479479
}
480480

‎test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java

Lines changed: 48 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,33 +24,63 @@
2424

2525
/*
2626
* @test
27-
* @bug 8358159
28-
* @summary test that the Cipher.getInstance() handles
29-
* transformations with empty mode and/or padding
30-
* @run main TestEmptyModePadding
27+
* @bug 8359388
28+
* @summary test that the Cipher.getInstance() would reject improper
29+
* transformations with empty mode and/or padding.
3130
*/
3231

33-
34-
import java.security.*;
35-
import javax.crypto.*;
32+
import java.security.NoSuchAlgorithmException;
33+
import java.security.Provider;
34+
import java.security.Security;
35+
import javax.crypto.Cipher;
3636

3737
public class TestEmptyModePadding {
3838

3939
public static void main(String[] args) throws Exception {
40-
Provider provider = Security.getProvider(System.getProperty("test.provider.name", "SunJCE"));
40+
Provider provider = Security.getProvider(
41+
System.getProperty("test.provider.name", "SunJCE"));
42+
43+
System.out.println("Testing against " + provider.getName());
4144

42-
test("AES", provider);
43-
test("AES/ECB/PKCS5Padding", provider);
44-
test("AES//PKCS5Padding", provider); // Empty mode
45-
test("AES/CBC/", provider); // Empty padding
46-
test("AES/ /NoPadding", provider); // Mode is a space
47-
test("AES/CBC/ ", provider); // Padding is a space
48-
test("AES/ / ", provider); // Both mode and padding are spaces
49-
test("AES//", provider); // Both mode and padding are missing
45+
String[] testTransformations = {
46+
// transformations w/ only 1 component, i.e. algo
47+
" ",
48+
// transformations w/ only 2 components
49+
"AES/",
50+
"AES/ ",
51+
"AES/CBC",
52+
"PBEWithHmacSHA512/224AndAES_128/",
53+
"PBEWithHmacSHA512/256AndAES_128/ ",
54+
"PBEWithHmacSHA512/224AndAES_128/CBC",
55+
// 3-component transformations w/ empty component(s)
56+
"AES//",
57+
"AES/ /",
58+
"AES// ",
59+
"AES/ / ",
60+
"AES/CBC/", "AES/CBC/ ",
61+
"AES//PKCS5Padding", "AES/ /NoPadding",
62+
"PBEWithHmacSHA512/224AndAES_128//",
63+
"PBEWithHmacSHA512/224AndAES_128/ /",
64+
"PBEWithHmacSHA512/224AndAES_128// ",
65+
"PBEWithHmacSHA512/224AndAES_128/ / ",
66+
"PBEWithHmacSHA512/256AndAES_128/CBC/",
67+
"PBEWithHmacSHA512/256AndAES_128/CBC/ ",
68+
"PBEWithHmacSHA512/256AndAES_128//PKCS5Padding",
69+
"PBEWithHmacSHA512/256AndAES_128/ /PKCS5Padding",
70+
};
5071

72+
for (String t : testTransformations) {
73+
test(t, provider);
74+
}
5175
}
5276

53-
private static void test(String transformation, Provider provider) throws Exception {
54-
Cipher c = Cipher.getInstance(transformation, provider);
77+
private static void test(String t, Provider p) throws Exception {
78+
try {
79+
Cipher c = Cipher.getInstance(t, p);
80+
throw new RuntimeException("Should throw NSAE for \'" + t + "\'");
81+
} catch (NoSuchAlgorithmException nsae) {
82+
// transformation info is already in the NSAE message
83+
System.out.println("Expected NSAE: " + nsae.getMessage());
84+
}
5585
}
5686
}

0 commit comments

Comments
 (0)