[webkit-reviews] review denied: [Bug 169761] [WebCrypto] Add support for AES-CTR : [Attachment 306148] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 5 17:30:34 PDT 2017
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!
More information about the webkit-reviews
mailing list