[webkit-reviews] review denied: [Bug 207238] [WebCrypto][CommonCrypto] Incorrect AES-CTR with counterLength longer than 64 : [Attachment 390229] Patch

[bugzilla-daemon at webkit.org]

Jiewen Tan <jiewen_tan at apple.com> has denied Yoshiaki Jitsukawa
<yoshiaki.jitsukawa at sony.com>'s request for review:
Bug 207238: [WebCrypto][CommonCrypto] Incorrect AES-CTR with counterLength
longer than 64
https://bugs.webkit.org/show_bug.cgi?id=207238

Attachment 390229: Patch

https://bugs.webkit.org/attachment.cgi?id=390229&action=review




--- Comment #11 from Jiewen Tan <jiewen_tan at apple.com> ---
Comment on attachment 390229
  --> https://bugs.webkit.org/attachment.cgi?id=390229
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390229&action=review

Overall, the patch looks good to me. Please address some of my comments.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:1
> +/*

Why don't you have a .cpp file?

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:35
> +template <size_t BlockSize = 16> // Counter block size in bytes.

Any reasons that the BlockSize is not fixed to 16 bytes?

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:36
> +class CryptoAlgorithmCounterBlockHelper {

Could it be more specific about AES-CTR? I guess no other algorithms need this.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:76
> +	   for (size_t i = 0; i < sizeof(size_t); i++) {

It is unfortunate that there is no helpers in bitset to get value out.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:101
> +	   for (int j = 0; j < 8; j++) {

It looks like that you reverse the endian of the byte? You can use
    for (size_t j = 7; j >= 0; j--)
to keep the endian same.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:102
> +	       bits.set(i * 8 + j, byte & 0x01); // Test the LSB and set it to
the bit.

This is why I dislike bitset. It is very tedious to manipulate the value and it
is not efficient to do that as well. I guess there are no better options if you
have dealt with counter size larger then size of size_t. The efficiency is
probably no issues here comparing to the bundled decryption/encryption
operations.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:110
> +	   for (int j = 0; j < 8; j++) {

Ditto.