Bug 208186 – [OpenSSL] Implement WebCrypto APIs for AES-CTR and AES-KW

• Component: Platform
• Status: RESOLVED FIXED

[Tomoki Imai]

To follow up bug 207176, add WebCrypto APIs for remaining AES families AES-CTR and AES-KW using OpenSSL.

[Tomoki Imai]

Created attachment 391636 [details]
patch

Patch to implement WebCrypto API for AES-KW and AES-CTR using OpenSSL

[Tomoki Imai]

Created attachment 391641 [details]
patch

I noticed the previous patch attachment 391636 [details] outputs the warning.
Source\WebCore\crypto\openssl\CryptoAlgorithmAES_CTROpenSSL.cpp(73,76): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)

For 64bit machine, counterLength is 63 in the worst case, so I think 1 << (size_t)63 causes overflow. 
I fixed this warning by casting 1 to size_t.

[Yoshiaki Jitsukawa]

Comment on attachment 391641 [details]
patch

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

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:66
> +    // Disable padding

Do we need to explicitly disable padding in CTR mode?

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:70
> +    size_t blocks = inputText.size() / blockSize + 1;

Is there any particular reason why you don't use roundUpToMultipleOf() like other AES variants?
e.g. const size_t blocks= roundUpToMultipleOf(blockSize, plainSize) / blockSize

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:73
> +    if (counterLength < sizeof(size_t) * 8 && blocks > ((size_t) 1 << counterLength))

Could you remove the space between "(size_t)" and "1"?

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:86
> +    Vector<uint8_t> outputText(headSize);

outputText should be reserved and used both in first and second parts to avoid reallocation.

> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:102
> +    {

Could you early skip the second part by doing condition check like:
 if (capacity < block)

[Yoshiaki Jitsukawa]

AES-KW part LGTM :)

[Tomoki Imai]

Created attachment 393013 [details]
patch to add AES-KW/AES-CTR

Modify reviewed point

[Tomoki Imai]

(In reply to Yoshiaki Jitsukawa from comment #3)
> Comment on attachment 391641 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391641&action=review
> 
> > Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:66
> > +    // Disable padding
> 
> Do we need to explicitly disable padding in CTR mode?

It looks like OpenSSL doesn't need explicit disabling padding for CTR mode, but we want to keep it, because the OpenSSL document says "By default encryption operations are padded using standard block padding and the padding is checked and removed when decrypting.".
https://www.openssl.org/docs/man1.1.1/man3/EVP_CIPHER_CTX_set_padding.html

We also noticed that it also says "This function should be called after the context is set up for encryption or decryption with EVP_EncryptInit_ex(), EVP_DecryptInit_ex() or EVP_CipherInit_ex()". Our landed patch in bug 207176 calls EVP_CIPHER_CTX_set_padding before EVP_EncryptInit_ex. We'd like to add one more follow-up patch for this.

> 
> > Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:70
> > +    size_t blocks = inputText.size() / blockSize + 1;
> 
> Is there any particular reason why you don't use roundUpToMultipleOf() like
> other AES variants?
> e.g. const size_t blocks= roundUpToMultipleOf(blockSize, plainSize) /
> blockSize

No, so we now use roundUpToMultipleOf.
 
> > Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:73
> > +    if (counterLength < sizeof(size_t) * 8 && blocks > ((size_t) 1 << counterLength))
> 
> Could you remove the space between "(size_t)" and "1"?

Sure, removed.

> 
> > Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:86
> > +    Vector<uint8_t> outputText(headSize);
> 
> outputText should be reserved and used both in first and second parts to
> avoid reallocation.

Good idea! We applied this in attachment 393013 [details].

> 
> > Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CTROpenSSL.cpp:102
> > +    {
> 
> Could you early skip the second part by doing condition check like:
>  if (capacity < block)

Yes, we added "if (capacity < block)" guard for the second part.

[Yoshiaki Jitsukawa]

attachment 391641 [details] LGTM

[Yoshiaki Jitsukawa]

(In reply to Yoshiaki Jitsukawa from comment #7)
> attachment 391641 [details] LGTM

sorry, it’s attachment 393013 [details]

[WebKit Commit Bot]

Comment on attachment 393013 [details]
patch to add AES-KW/AES-CTR

Clearing flags on attachment: 393013

Committed r258254: <https://trac.webkit.org/changeset/258254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60309903>

[Tomoki Imai]

Thanks for your review, r+, and cq+!