Bug 208186

Summary: [OpenSSL] Implement WebCrypto APIs for AES-CTR and AES-KW
Product: WebKit Reporter: Tomoki Imai <tomoki.imai>
Component: PlatformAssignee: Tomoki Imai <tomoki.imai>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, don.olmstead, ews-watchlist, Hironori.Fujii, jiewen_tan, tomoki.imai, webkit-bug-importer, yoshiaki.jitsukawa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=210540
Bug Depends on:    
Bug Blocks: 206439    
Attachments:
Description Flags
patch
none
patch
none
patch to add AES-KW/AES-CTR none

Description Tomoki Imai 2020-02-25 01:28:24 PST
To follow up bug 207176, add WebCrypto APIs for remaining AES families AES-CTR and AES-KW using OpenSSL.
Comment 1 Tomoki Imai 2020-02-25 01:31:57 PST
Created attachment 391636 [details]
patch

Patch to implement WebCrypto API for AES-KW and AES-CTR using OpenSSL
Comment 2 Tomoki Imai 2020-02-25 02:08:53 PST
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.
Comment 3 Yoshiaki Jitsukawa 2020-03-01 22:22:30 PST
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)
Comment 4 Yoshiaki Jitsukawa 2020-03-01 23:52:15 PST
AES-KW part LGTM :)
Comment 5 Tomoki Imai 2020-03-08 23:56:12 PDT
Created attachment 393013 [details]
patch to add AES-KW/AES-CTR

Modify reviewed point
Comment 6 Tomoki Imai 2020-03-09 00:06:24 PDT
(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.
Comment 7 Yoshiaki Jitsukawa 2020-03-09 00:49:03 PDT
attachment 391641 [details] LGTM
Comment 8 Yoshiaki Jitsukawa 2020-03-09 01:13:26 PDT
(In reply to Yoshiaki Jitsukawa from comment #7)
> attachment 391641 [details] LGTM

sorry, it’s attachment 393013 [details]
Comment 9 WebKit Commit Bot 2020-03-10 23:22:26 PDT
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>
Comment 10 WebKit Commit Bot 2020-03-10 23:22:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2020-03-10 23:23:22 PDT
<rdar://problem/60309903>
Comment 12 Tomoki Imai 2020-03-10 23:26:07 PDT
Thanks for your review, r+, and cq+!