RFR for JDK-8048604 : Tests for strong crypto ciphers

[Valerie Peng]

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/20150915/2e15f602/attachment.htm>