[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 #10 from Tomoki Imai <tomoki.imai at sony.com> ---
Comment on attachment 389622
  --> https://bugs.webkit.org/attachment.cgi?id=389622
patch

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

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CFBOpenSSL.cpp:33
>> +#include <openssl/aes.h>
> 
> Probably we shouldn't include low level header "aes.h".

Yes, removed.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:33
>> +#include <openssl/aes.h>
> 
> Probably we shouldn't include low level header "aes.h".

Removed.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:75
>> +    size_t blocks = inputText.size() / EVP_MAX_IV_LENGTH + 1;
> 
> It's unclear to me why EVP_MAX_IV_LENGTH should be used. Shouldn't this be replaced with the block size from EVP_CIPHER_block_size()?
> 
> Also It seems that blocks is the number of padded cipher blocks thus it will differ depends on the operation mode.

Yes, we should use EVP_CIPHER_block_size here. (not addressed yet)

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:78
>> +    if (counterLength < EVP_MAX_KEY_LENGTH && blocks > (int)(1 << counterLength))
> 
> counterLength may be more than 64 so the bit shift may result in an overflow. EVP_MAX_KEY_LENGTH is 64 bytes so it doesn't make sense to compare counterLength (in bits) with EVP_MAX_KEY_LENGTH (in bytes).

Thanks for pointing this out, you're right, using EVP_MAX_KEY_LENGTH is incorrect. (not addressed yet)

What we wanted to here is checking counterLength is large enough to decrypt/encrypt data of arbitrary size fit in the address space, or the number of blocks is smaller than value range of counter.
To test the former, Mac implementation compares counterLength and __WORDSIZE, which is expanded to 32 or 64.

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp?rev=255668#L58

Unfortunately, on Windows __WORDSIZE is not available.
Is there any common way to get __WORDSIZE on every ports?

We also noticed that platform-independent CryptoAlgorithmAES_GCM uses __WORDSIZE.
It is used in #if directive, and expanded as 0 as it's not defined.
As a result it currently uses 32 bit path, even when 64bit Windows. 
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_GCM.cpp?rev=255668#L42

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:83
>> +    size_t counts = bigIntegerToSizeT(counter);
> 
> counts may be more than 2^64.

bigIntegerToSizeT should return a value up to 2^64-1 on 64bit machine, and up to 2^32-1 on 32bit machine. (not addressed yet)
>From my understanding, there is no case where we touch 64 ~ 127-th (0-origin) bit from right on 64bit machine, because the largest block number we may have is bound by 2^64.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp?rev=255668#L37

Also here we want to use __WORDSIZE equivalent for Windows.

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:157
>> +    auto output = crypt(AES_ENCRYPT, key.key(), parameters.counterVector(), parameters.length, plainText);
> 
> According to the document, EVP_CipherInit_ex() takes 1 for encryption and 0 for decryption for the last parameter so AES_ENCRYPT/AES_DECRYPT shouldn't appear hear. Instead, how about passing crypt() to a boolean parameter and give 1 or 0 to EVP_CipherInit_ex().

Right, we shouldn't use AES_ENCRYPT/AES_DECRYPT here.
We now passes 1 or 0 to crypt, instead of boolean as you suggested.
It is because the other ports also passes the platform-dependent constants here.
- https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp?rev=255668#L200
- https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp?rev=255668#L144

-- 
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/8924d963/attachment.htm>