RFR for JDK-8048604 : Tests for strong crypto ciphers

Tristan Yan tristan.yan at oracle.com
Wed Sep 23 16:50:55 UTC 2015


Thanks Valerie, I fixed with new version, please review it again.
http://cr.openjdk.java.net/~tyan/8048604/webrev.03/ <http://cr.openjdk.java.net/~tyan/8048604/webrev.03/>
Tristan

> On Sep 22, 2015, at 4:30 PM, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> Hi, Tristan,
> 
> The updated webrev addressed most of my previous comments except the few that I noted below:
> 
> <Overall>
> - Can u please make sure that all the Cipher objects are retrieved using "SunJCE" provider? At least the AESPBEWrapper.java, CICOSkipTest.java (there may be more as I didn't check all) didn't     specify a provider when calling Cipher.getInstance().
> 
> <CICO_PBE_SKIP_Test.java>
> - both the proceedSkipTestUsingXXX() methods should check to ensure that "SAVE" number of bytes are read.
> 
> <CICOSkipTest.java>
> - line 217 is redundant
> 
> Thanks,
> Valerie
> 
> On 9/21/2015 10:14 AM, Tristan Yan wrote:
>> 
>> Thank you Valerie. Please review the updated version of it
>> http://cr.openjdk.java.net/~tyan/8048604/webrev.02/ <http://cr.openjdk.java.net/%7Etyan/8048604/webrev.02/>
>> 
>> Cheers
>> Tristan
>> 
>>> On Sep 16, 2015, at 3:24 PM, Valerie Peng <valerie.peng at oracle.com <mailto:valerie.peng at oracle.com>> wrote:
>>> 
>>> 
>>> Can u please make sure that all the Cipher objects are retrieved using "SunJCE" provider?
>>> I noticed some inconsistencies here and there.
>>> 
>>> <TextPKCS5PaddingTest.java>
>>> - line 94, 'provider' object should be used here.
>>> - need @library tag to find the TestUtilities class, otherwise, the compilation fails.
>>> 
>>> <PbeAlgorithm.java>
>>> - variable "model" should be named "mode".
>>> - nit: rename the class to PBEAlgorithm for consistency.
>>> 
>>> <AbstractPBEWrapper.java>
>>> -  line 98 has a typo, I think u meant "or" instead of "of"
>>> 
>>> <PBKDF2Wrapper.java>
>>> - typo on line51: TANSFORMATION should be TRANSFORMATION
>>> - line 94, why not just init with mode directly as in other Wrapper classes?
>>> 
>>> <CICO_PBE_Test.java>
>>> - line 63, variable name normally starts with lower case. Can u fix it for better readability? Same goes for other PBE tests.
>>> 
>>> <CICO_PBE_SKIP_Test.java>
>>> - the test class description is not very readable and contains a few typos. Can u please double check?
>>> - typo on lin 132 - decript
>>> - both the proceedSkipTestUsingXXX() methods should check to ensure that "SAVE" number of bytes are read.
>>> 
>>> <CICO_PBE_RW_Test.java>
>>> - line 79 doesn't look right at all. The comparison should be made against the output written to output stream instead of itself.
>>>  if (!TestUtilities.equalsBlock(plainText, plainText, TEXT_SIZE)) {
>>> That's all.
>>> Valerie
>>> 
>>> On 9/15/2015 8:15 PM, Valerie Peng wrote:
>>>> 
>>>> 
>>>> Most of the tests are DES, DESede, and Blowfish and they are written in a way which won't work with AES due to the larger block size as the block size 8 is dispersed through out the tests instead of as a constant.
>>>> At some point, I think we need to enhance these tests to cover AES instead of the legacy algorithms such as DES, DESede, etc. Can u define a constant for the block size and replace 8 with this constant?
>>>> 
>>>> Some nit - If tests are already placed under CICO directory, their names do not need to contain CICO.
>>>> Also, maybe u can just place the tests under com/sun/crypto/provider/CICO instead of com/sun/crypto/provider/Cipher/CICO.
>>>> 
>>>> <TestUtilities.java>
>>>> - if my reading of the code is right, the equalsBlockSave(byte[] b1, byte[]b2, int bLen, int save) method compares b1 and b2 by chopping up b1 into blocks of 'bLen' and b2 into blocks of 'save', and then compare the first 'save' bytes of each block to make sure they are equal. line 59 looks incorrect -  the number of blocks should be computed using b1.length instead of b2.length. The term "save" seems confusing too. Maybe "partial" would be more suitable? Or maybe changing "bLen" to "b1Len", and "save" to "b2Len".
>>>> The description on line 50 could use some more words to explain what this method does without reading through the code.
>>>> 
>>>> <CICOChainingTest.java>
>>>> - after line 85, check that there are no further data available after reading "recoveredText".
>>>> 
>>>> <CICODESFuncTest.java>
>>>> - line 63, "is not exist" should be "does not exist" or "not found".
>>>> - the comments on line 67-68 and line 77 seems contradictory to each other. Essentially, NoPadding is tested for all modes vs PKCS5Padding is tested for some modes.
>>>> - the check from line106-110 should be moved up to right after the Cipher.getInstance() calls on line 97 and 98.
>>>> 
>>>> <ReadModel.java>
>>>> - line 74, instead of byte[] plainText, I think it's clearer to just have "int inputLen". The content of the input array is not used in any of the enum values, but rather just the length for output buffer allocation.
>>>> - the variable "ci1" should really be named "ciIn" so that it's clear that this argument is the cipher associated with the CipherInputStream.  
>>>> 
>>>> <CICOSkipTest.java>
>>>> - line 124, "blockLen" should really be "numOfBlocks".
>>>> - Will LengthLimitException ever be thrown for this test? Given that you are only using default key length, I doubt the check on line 163 will be true.
>>>> - line 178-180 seems redundant? I think they can just be removed.
>>>> - line 184, why is the key length check being done here again inside the same method? This one for sure is useless.
>>>> - Instead of 2 constructors with comments indicating what ciphers they are for, it's better to just use static factory method. The implementation isn't that different, they both return a pair of ciphers. U can handle the different parameter type and secret key generation using a switch construct based on the specified algorithm 'algo'.
>>>> - line 217 and 234, 45 should be 'save'
>>>> 
>>>> <TextPKCS5PaddingTest.java>
>>>> - the testPaddingScheme() method doesn't seem too useful as it is testing the PKCS5Padding inside the test class itself. How would this detect any regression in SunJCE provider? I understand that the test may have problem accessing the actual PKCS5Padding class inside the SunJCE provider, but still, copy-n-paste the internal class out into test class is meaningless. This test method and the cut-n-pasted classes should be removed.
>>>> 
>>>> I will send u comments on PBE ones in a separate email.
>>>> Thanks,
>>>> Valerie
>>>> 
>>>> On 8/12/2015 4:06 PM, Tristan Yan wrote:
>>>>> 
>>>>> Please be free review these new tests for strong crypto ciphers.
>>>>> 
>>>>> webrev : http://cr.openjdk.java.net/~tyan/8048604/webrev.01/ <http://cr.openjdk.java.net/%7Etyan/8048604/webrev.01/>
>>>>> bug    : https://bugs.openjdk.java.net/browse/JDK-8048604 <https://bugs.openjdk.java.net/browse/JDK-8048604>
>>>>> 
>>>>> Thank you very much
>>>>> Tristan
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150923/0579fbd8/attachment.htm>


More information about the security-dev mailing list