[webkit-reviews] review granted: [Bug 170271] [GCrypt] Implement AES_GCM support : [Attachment 305949] 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 170271: [GCrypt] Implement AES_GCM support
https://bugs.webkit.org/show_bug.cgi?id=170271

Attachment 305949: Patch

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




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

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:39
> +#include <pal/crypto/gcrypt/Handle.h>
> +#include <pal/crypto/gcrypt/Utilities.h>

...

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:46
> +    auto algorithm = PAL::GCrypt::aesAlgorithmForKeySize(key.size() * 8);

Ugh, there's no good way to avoid having to write PAL::GCrypt everywhere, is
there? Because a simple "using PAL::GCrypt" is not going to work, right? This
is unfortunate.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:51
> +    gcry_error_t error = gcry_cipher_open(&handle, *algorithm,
GCRY_CIPHER_MODE_GCM, 0);

We should probably pass GCRY_CIPHER_SECURE instead of 0 to ensure the key
doesn't get paged to disk...?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:98
> +	   output.appendVector(tag);

It feels confusing/fragile, but I guess it's hard to change the cross-platform
interface for this? Or is this how the data is expected to be received by the
JS API?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:101
> +    return WTFMove(output);

Remove WTFMove.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:158
> +	   size_t offset = cipherText.size() - tagLength;

I'd declare this up above gcry_cipher_decrypt() so that you can use it on the
line following that too.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:163
> +    return WTFMove(output);

No WTFMove.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:177
> +		       [callback = WTFMove(callback), exceptionCallback =
WTFMove(exceptionCallback)](ScriptExecutionContext& context) {

Don't capture callback.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:185
> +		   [output = WTFMove(*output), callback = WTFMove(callback),
exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext&
context) mutable {

Don't capture exceptionCallback.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:203
> +		       [callback = WTFMove(callback), exceptionCallback =
WTFMove(exceptionCallback)](ScriptExecutionContext& context) {

Ditto.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:211
> +		   [output = WTFMove(*output), callback = WTFMove(callback),
exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext&
context) mutable {

Ditto.