Bug 208186

Created attachment 391636 [details] patch Patch to implement WebCrypto API for AES-KW and AES-CTR using OpenSSL

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

AES-KW part LGTM :)

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

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

attachment 391641 [details] LGTM

(In reply to Yoshiaki Jitsukawa from comment #7) > attachment 391641 [details] LGTM sorry, it’s attachment 393013 [details]

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>

All reviewed patches have been landed. Closing bug.

<rdar://problem/60309903>

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