RFR for JDK-8048604 : Tests for strong crypto ciphers

Tue Sep 22 23:30:40 UTC 2015

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/
>>>> bug    : 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/20150922/29ee660e/attachment.htm>