Some more general comments - related to the restructuring.
In AESKeyWrap at 152-155 - that check probably should be moved to W().
KWP should do the formatting prior to passing the data to W(). Also at
185-187 - move that to W_INV().
AESKeyWrap at 158 - shouldn't you be returning the cipher text length?
That's what the comment in FeedbackCipher says. W() should probably be
returning the final length.
The length of the final ciphertext data should be 8 bytes longer than
the plaintext. decryptFinal() seems to do the right thing by decreasing
the length returned. But again - shouldn't that be the purview of W_INV()?
The call in KeyWrapCipher.engineGetOutputSize() should probably also be
passed into KWUtil rather than being done in KeyWrapCipher. And the
more I look at this, the more I think that all of the engineUpdate
operations should throw UnsupportedOperationException - it would
certainly simplify the logic. That would make the call return either
inputLength + 8 or inputLength - 8 depending on mode.
KWUtil.W() - should probably check that in.length >= inLen + 8 and throw
a ShortBufferException if not.
Would it be useful to add a comment in KeyWrapCipher that warns
maintainers that AESKeyWrap(Padded).encryptFinal() and decryptFinal()
uses the input buffer as the output buffer? That's a reasonable approach
for decryption, but I'm wondering if you might want to support in-place
encryption as there's no spec prohibition requiring data to be held
until the encryption is complete.
Mike
On 5/24/2021 7:01 PM, Valerie Peng wrote:
On Fri, 21 May 2021 20:44:57 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
Valerie Peng has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains seven commits:
- Merge master into JDK-8248268
- Minor update to address review comments.
- Changed AESParameters to allow 4-byte, 8-byte IVs and removed
KWParameters and KWPParameters.
- Refactor code to reduce code duplication
Address review comments
Add more test vectors
- Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
AES/KWP/NoPadding
- Restored Iv algorithm parameters impl.
- 8248268: Support KWP in addition to KW
Updated existing AESWrap support with AES/KW/NoPadding cipher
transformation. Added support for AES/KWP/NoPadding and
AES/KW/PKCS5Padding support to SunJCE provider.
src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 50:
48:
49: public AESParameters() {
50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 8);
A cipher object may not take different IV sizes at the same time. I was just
wondering how it could be used in practice. Maybe something like:
AlgorithmParameters algParams = AlgorithmParameters.getInstance("AES");
algParams.init(ivParameterSpec);
The IV parameter is given with the init() method. Then, it may be not
necessary to construct the BlockCipherParamsCore object will all potential IV
sizes. See the comments in BlockCipherParamsCore.
Cipher objects normally takes just one iv size. BlockCipherParamsCore is used
by various impls of AlgorithmParametersSpi class which may be used with
different block cipher algorithms. The iv parameter is given with the
AlgorithmParametersSpi.init() method and invalid iv will lead to exceptions.
Since there are iv size checks built in BlockCipherParamsCore already, it seems
safer to relax the check a bit for AES (4 and 8 for KWP and KW). The other
choice is to just remove the size check from BlockCipherParamsCore for AES
algorithm, but then we'd not be able to reject invalid iv lengths as before.
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
line 52:
50: private byte[] iv = null;
51:
52: private int[] moreSizes = null;
The moreSizes is not used other than the init() method field. It may be not
easy to check the specific size if we cache all supported sized in the object.
For example, if the required IV size if 8 bytes, it may be a problem about how
to make sure the iv size is 8 bytes exactly for a specific algorithm.
Maybe, we could just have a ivSize filed. The default value is block_size,
which coupe be set with the init(ivParameterSpec) method.
....
private int ivSize;
...
BlockCipherParamsCore(int blkSize) {
block_size = blkSize;
ivSize = blkSize;
}
...
void init(AlgorithmParameterSpec paramSpec) {
ivSize = ...; // reset IV size.
}
// use ivSize while encoding and decoding.
The problem with this is that this assumes that the specified paramSpec
argument of the init() method is always valid.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2404
