[webkit-reviews] review granted: [Bug 171420] [GCrypt] AES_CTR support : [Attachment 308522] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 171420: [GCrypt] AES_CTR support
https://bugs.webkit.org/show_bug.cgi?id=171420

Attachment 308522: Patch

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




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 308522
  --> https://bugs.webkit.org/attachment.cgi?id=308522
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:42
> +using GCryptCipherOperation = gcry_error_t(gcry_cipher_hd_t, void*, size_t,
const void*, size_t);

This belongs in Utilities.h.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:69
> +    // This is a helper functor that resets the cipher object, sets the
provided counter data,
> +    // and executes the encrypt or decrypt operation, retrieving and
returning the output data.
> +    auto callOperation =
> +	   [&handle, &operation](const Vector<uint8_t>& counter, const uint8_t*
inputData, const size_t inputSize) -> std::optional<Vector<uint8_t>>

I don't see any reason to use a lambda here, when it could be a static function
instead.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:101
> +    {

Nice use of extra scoping.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:103
> +	   PAL::GCrypt::Handle<gcry_mpi_t>
blockSizeMaskMPI(gcry_mpi_set_ui(nullptr, blockSize - 1));

Why is this named "blockSizeMaskMPI" if it is being used as part of an addition
operation rather than a bitmask operation?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:114
> +    gcry_mpi_mul_2exp(counterLimitMPI, counterLimitMPI, counterLength);

Same as in my review from a while back, this probably isn't worth worrying
about at all as I doubt GCrypt will ever break this, but it will fail if GCrypt
ever requires that the parameters be distinct.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:117
> +    // the number of unique counter values we could procude for the
specified counter

Did you mean "produce"?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:168
> +    // FIXME: This should be optimized further by first encrypting the
amount of blocks for
> +    // which the counter won't yet wrap around, and then encrypting the rest
of the blocks
> +    // starting from the counter set to 0.

Is this not straightforward to implement?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:212
> +	       auto& aesParameters =
downcast<CryptoAlgorithmAesCtrParams>(*parameters);
> +	       auto& aesKey = downcast<CryptoKeyAES>(key.get());
> +
> +	       auto output = gcryptAES_CTR(gcry_cipher_encrypt, aesKey.key(),
aesParameters.counterVector(), aesParameters.length, plainText);

Can you think of any way to share more of the implementation of this function
with the other CryptoAlgorithm classes? It looks like these three lines are the
only ones that ever differ....

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:240
> +	       auto& aesParameters =
downcast<CryptoAlgorithmAesCtrParams>(*parameters);
> +	       auto& aesKey = downcast<CryptoKeyAES>(key.get());
> +
> +	       auto output = gcryptAES_CTR(gcry_cipher_decrypt, aesKey.key(),
aesParameters.counterVector(), aesParameters.length, cipherText);

Ditto. A follow-up patch should work on reducing code duplication between all
the d ifferent platformEncrypt/Decrypt/Sign/Verify functions, in addition to
the static functions at the top of the file. I think the only code that needs
to differ is this bit here that converts the CryptoAlgorithmParameters and
CryptoKey into a call to the algorithm (gcryptAES_CTR here).