Bug 207176 – [OpenSSL] Implement WebCrypto APIs for AES-CBC, AES-CFB and AES-GCM

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Tomoki Imai]

As the first step of bug 206439, add AES-CBC, AES-CFB, AES-CTR, and AES-GCM support to WebCrypto with OpenSSL backend.

[Tomoki Imai]

Created attachment 389617 [details]
patch

Patch to add AES-CBC, AES-CFB, AES-CTR, AES-GCM support

[Tomoki Imai]

It would be nice to land bug 207174 before this bug, because bug 207174 adds more LayoutTests which checks AES-CBC implementations we're going to add in this bug.

[Yoshiaki Jitsukawa]

Comment on attachment 389617 [details]
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.

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:64
> +

Ensure that the iv size is equal to the block size.

> 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.

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:100
> +

Please check the IV size.

It'd be nice to check if cipherSize is a multiple of the block size.

[Yoshiaki Jitsukawa]

Comment on attachment 389617 [details]
patch

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

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:64
>> +
> 
> Ensure that the iv size is equal to the block size.

Never mind. IV size is checked in CryptoAlgorithmAES_CBC::decrypt().

>> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:100
>> +
> 
> Please check the IV size.
> 
> It'd be nice to check if cipherSize is a multiple of the block size.

Never mind. IV size is checked in CryptoAlgorithmAES_CBC::encrypt().

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CFBOpenSSL.cpp:53
> +    Vector<uint8_t> cipher(plainText.size());

"cipher" should be renamed to "cipherText" for better consistency?

> 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)" ?

> 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)" ?

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_GCMOpenSSL.cpp:114
> +    if (cipherText.size() <= tagLength)

This is already checked in CryptoAlgorithmAES_GCM::decrypt().

[Tomoki Imai]

Created attachment 389622 [details]
patch

Just addressed style-checker errors from attachment 389617 [details].

[Tomoki Imai]

(In reply to Yoshiaki Jitsukawa from comment #4)
> Comment on attachment 389617 [details]
> patch
> 

Thanks for your review! 
We're now addressing your comments.

[Yoshiaki Jitsukawa]

Comment on attachment 389622 [details]
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".

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:33
> +#include <openssl/aes.h>

Probably we shouldn't include low level header "aes.h".

> 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.

> 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).

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:83
> +    size_t counts = bigIntegerToSizeT(counter);

counts may be more than 2^64.

> 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().

[Tomoki Imai]

Created attachment 389647 [details]
patch

Fixed the issues pointed in comment 3, comment 4, and comment 7.
Some of issues in CryptoAlgorithmAES_CTROpenSSL is not addressed yet.
I'll describe about the remaining issues later.

[Tomoki Imai]

Comment on attachment 389617 [details]
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.

[Tomoki Imai]

Comment on attachment 389622 [details]
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

[Tomoki Imai]

Comment on attachment 389647 [details]
patch

I'm making r? as we need more reviews on the patch, but not cq? because we still have non-addressed review comments.

[Tomoki Imai]

We're going to drop AES-CTR support for now because we cannot be confident of our CTR mode implementation.
After landing the AES-CBC, AES-CFB, and AES-GCM, we will make another patch to support AES-CTR.

[Tomoki Imai]

Created attachment 389788 [details]
patch

- Drop AES-CTR support.
- Replace "Finalise" with "Finalize"
- Fix some capitalization in comments.

By this patch, we addressed all the reviewed points in comment 3 - comment 7.

[Jiewen Tan]

Comment on attachment 389788 [details]
patch

Sorry that I'm not familiar with OpenSSL. Overall, the implementation seems solid. I will let someone from Sony to put the trigger to r+ it.

[Yoshiaki Jitsukawa]

Comment on attachment 389788 [details]
patch

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

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_GCMOpenSSL.cpp:61
> +    Vector<uint8_t> cipherText(plainText.size());

We can reserve this cipherText buffer including the tag area like: Vector<uint8_t> cipherText(plainText.size() + tagLength)
so that we can avoid potential memory reallocation afterwards.

[Tomoki Imai]

Created attachment 390054 [details]
patch

Reserve tag memory area along with cipherText in CryptoAlgorithmAES_GCMOpenSSL(suggested in comment 15)

[WebKit Commit Bot]

Comment on attachment 390054 [details]
patch

Clearing flags on attachment: 390054

Committed r256014: <https://trac.webkit.org/changeset/256014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59254413>