RFR: 8359388: Stricter checking for cipher transformations [v2]
Mikhail Yankelevich
myankelevich at openjdk.org
Tue Jun 17 10:34:29 UTC 2025
On Mon, 16 Jun 2025 22:20:10 GMT, Valerie Peng <valeriep at openjdk.org> wrote:
>> Based on the javadoc of `javax.crypto.Cipher` class, the cipher transformation should be either "algorithm/mode/padding" or
>> "algorithm". When parsing the transformation, space(s) is trimmed off and empty strings are considered as "unspecified". This PR adds checks to ensure that transformations with empty "mode" and/or "padding" value in the "algorithm/mode/padding" form leads to `NoSuchAlgorithmException`. This reverts some changes made in [https://bugs.openjdk.org/browse/JDK-8358159](https://bugs.openjdk.org/browse/JDK-8358159) which allows empty mode and/or padding in the transformations.
>>
>>
>> Thanks in advance for the review~
>
> Valerie Peng has updated the pull request incrementally with one additional commit since the last revision:
>
> Updated test per Mikhail's review comments.
Thank you for your changes!
Just a few super minor questions
test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java line 30:
> 28: * @summary test that the Cipher.getInstance() would reject improper
> 29: * transformations with empty mode and/or padding.
> 30: * @run main TestEmptyModePadding
minor: Is `@run` needed here? It's fine to leave it here, if you prefer it this way though.
test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java line 37:
> 35: import javax.crypto.*;
> 36:
> 37: public class TestEmptyModePadding {
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;
test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java line 45:
> 43: System.out.println("Testing against " + provider.getName());
> 44:
> 45: String[] testTransformations = {
minor: Do you think it would be easier to read if each entry was a separate line?
test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java line 69:
> 67: for (String t : testTransformations) {
> 68: test(t, provider);
> 69: };
nit, `;` after for loop
Suggestion:
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/25808#pullrequestreview-2935056506
PR Review Comment: https://git.openjdk.org/jdk/pull/25808#discussion_r2151902251
PR Review Comment: https://git.openjdk.org/jdk/pull/25808#discussion_r2151902337
PR Review Comment: https://git.openjdk.org/jdk/pull/25808#discussion_r2151906412
PR Review Comment: https://git.openjdk.org/jdk/pull/25808#discussion_r2151894079
More information about the security-dev
mailing list