[webkit-reviews] review denied: [Bug 169761] [WebCrypto] Add support for AES-CTR : [Attachment 306148] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 169761: [WebCrypto] Add support for AES-CTR
https://bugs.webkit.org/show_bug.cgi?id=169761

Attachment 306148: Patch

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




--- Comment #9 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 306148
  --> https://bugs.webkit.org/attachment.cgi?id=306148
Patch

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

I think this looks good, but I'd like you to clean up a few things before we
land.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:48
> +static bool parametersAreValid(CryptoAlgorithmAesCtrParams& paramters)

paramters -> parameters (missing an 'e').

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:40
> +static size_t bigIntegerToSize(const Vector<uint8_t>& bigInteger)

I think you should call this bigIntegerToSizeT.

Also, do none of the "clampTo"

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:55
> +    // by the counterLength. It then increments the nonce which should stay
same for the whole operatoin. To remedy this issue,

operatoin -> operation.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:66
> +	   // Set Nonce to 1s

This comment makes it sound like you are setting the Nonce to some set of 1s: 
"111111111111". But it's not clear what you're doing here.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:107
> +    Vector<uint8_t> resetedCounter(counter.size());

reseted is a weird word. I suggest "remainingCounter" or something like that.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:109
> +    size_t nonceOffset = counter.size() - counterLength / 8 -
!!counterOffset;

!!counterOffset is overly clever. Is that to give you either 0 or 1?

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:135
> +    ASSERT(p <= tail.end());

ASSERT_WITH_SECURITY_IMPLICATION

>
LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/pbkdf
2.worker-expected.txt:15
> +PASS Derived key of type name: AES-CTR length: 128  using short password,
short salt, SHA-384, with 1 iterations 

Yay! More passes!