8359388: Stricter checking for cipher transformations #25808
Conversation
|
👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@valeriepeng The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| System.out.println("Testing " + transformation); | ||
| try { | ||
| Cipher c = Cipher.getInstance(transformation, provider); | ||
| throw new RuntimeException("Expected NSAE not thrown"); |
There was a problem hiding this comment.
Minor: Do you think it would be a good idea to print out transformation and provider data for debug? In System.err that is. Otherwise afaik the exception is thrown and no trace in system error, just in System.out.println, making debugging quite challenging
Just something like this
| throw new RuntimeException("Expected NSAE not thrown"); | |
| System.err.println("Error while testing " + transformation); | |
| throw new RuntimeException("Expected NSAE not thrown"); |
or just add it to the RuntimeException message
There was a problem hiding this comment.
Ok, I will update the test to include the transformation info in the exception message. As for provider, it's somewhat related, but given that it stays the same for the test, I printed it out just once in the beginning of the test.
| String algo = (algorithmOnly ? transformation.trim() : | ||
| transformation.substring(0, endIdx).trim()); | ||
| if (algo.isEmpty()) { | ||
| throw new NoSuchAlgorithmException("Invalid transformation: " + |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sure, I can add it.
There was a problem hiding this comment.
Thank you! This is covered now
| * transformations with empty mode and/or padding | ||
| * @bug 8358159 8359388 | ||
| * @summary test that the Cipher.getInstance() would reject improper | ||
| * transformations with empty mode and/or padding. | ||
| * @run main TestEmptyModePadding |
There was a problem hiding this comment.
minor: Is @run needed here? It's fine to leave it here, if you prefer it this way though.
There was a problem hiding this comment.
I am ok either way, just didn't bother to remove it from the original test. :)
| @@ -37,20 +37,45 @@ | |||
| public class TestEmptyModePadding { | |||
There was a problem hiding this comment.
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;There was a problem hiding this comment.
Sure, I can do this.
|
|
||
| String[] testTransformations = { |
There was a problem hiding this comment.
minor: Do you think it would be easier to read if each entry was a separate line?
There was a problem hiding this comment.
I am ok either way, sounds like you prefer one line per entry. I can change them.
Co-authored-by: Mikhail Yankelevich <[email protected]>
Based on the javadoc of
javax.crypto.Cipherclass, 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 which allows empty mode and/or padding in the transformations.Thanks in advance for the review~
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25808/head:pull/25808$ git checkout pull/25808Update a local copy of the PR:
$ git checkout pull/25808$ git pull https://git.openjdk.org/jdk.git pull/25808/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25808View PR using the GUI difftool:
$ git pr show -t 25808Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25808.diff
Using Webrev
Link to Webrev Comment