[Webkit-unassigned] [Bug 207176] [OpenSSL] Implement WebCrypto APIs for AES family except AES-KW

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=207176

--- Comment #9 from Tomoki Imai <tomoki.imai at sony.com> ---
Comment on attachment 389617
  --> https://bugs.webkit.org/attachment.cgi?id=389617
patch

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

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:59
>> +        return WTF::nullopt;
> 
> In this case we can return early before creating a cipher context.

Thanks, we moved the aesAlgorithm and make an early return if we cannot obtain algorithm.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:67
>> +        return WTF::nullopt;
> 
> Shouldn't we clean up "ctx" before leaving the function? We may want to introduce a wrapper class like CairoUniquePtr or something like that.

We definitely should clean up. We introduced OpenSSLCryptoUniquePtr to make sure that we call EVP_CIPHER_CTX_free every branch.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CFBOpenSSL.cpp:53
>> +    Vector<uint8_t> cipher(plainText.size());
> 
> "cipher" should be renamed to "cipherText" for better consistency?

Thanks, we renamed all "cipher" to "cipherText", and "plain" to "plainText".

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_GCMOpenSSL.cpp:50
>> +    if (iv.size() != EVP_MAX_IV_LENGTH)
> 
> Is this intended to be "if (iv.size() > EVP_MAX_IV_LENGTH)" ?

Thanks, good point.
Actually I believe we don't need this check because AES-GCM accepts the initialization vectors of arbitrary length, and EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, ivlen, NULL) doesn't have ivlen limitation for GCM mode.
- https://www.w3.org/TR/WebCryptoAPI/#aes-gcm-params
- https://www.openssl.org/docs/manmaster/man3/EVP_CIPHER_block_size.html

We may want to have LayoutTests to make sure that it works with the arbitrary length IV.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_GCMOpenSSL.cpp:111
>> +    if (iv.size() != EVP_MAX_IV_LENGTH)
> 
> Is this intended to be "if (iv.size() > EVP_MAX_IV_LENGTH)" ?

As described in cryptEncrypt, we believe we don't need this check.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_GCMOpenSSL.cpp:114
>> +    if (cipherText.size() <= tagLength)
> 
> This is already checked in CryptoAlgorithmAES_GCM::decrypt().

Thanks, removed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200204/cb1c8b08/attachment.htm>