[GTK] Adds implementation of subtle crypto AES-CBC algorithm
Created attachment 232189 [details] Patch
Adding dependency on bug 133316 and bug 133317.
Created attachment 238545 [details] Patch
Attachment 238545 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 238545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238545&action=review > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:39 > +static gnutls_cipher_hd_t createCipher(const CryptoAlgorithmAesCbcParams& parameters, const CryptoKeyAES& key, gnutls_cipher_algorithm_t *algorithm) Misplaced * :) Also perhaps a smart pointer could be used instead of a raw pointer here. > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:51 > + gnutlsKey.data = (unsigned char*) key.key().data(); We avoid C-style casts in WebCore, here I think a reinterpret_cast might work. > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:55 > + gnutlsIv.data = (unsigned char*) parameters.iv.data(); Ditto > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:68 > + gnutls_cipher_algorithm_t algo; algorithm > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:71 > + failureCallback(); No need to set ExceptionCode here? > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:90 > + failureCallback(); No need to set ExceptionCode here? > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:104 > + failureCallback(); No need to set ExceptionCode here? > Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:112 > +void CryptoAlgorithmAES_CBC::platformDecrypt(const CryptoAlgorithmAesCbcParams& parameters, const CryptoKeyAES& key, const CryptoOperationData& data, VectorCallback callback, VoidCallback failureCallback, ExceptionCode&) Same comments as in previous method
Created attachment 238601 [details] Patch
Attachment 238601 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gtk/CryptoAlgorithmAES_CBCGtk.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 238601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238601&action=review > Source/WebCore/crypto/CryptoExceptionCode.h:38 > + CryptoNotSupportedErr, > + CryptoInvalidAccessErr, > + CryptoTypeMismatchErr, > + CryptoExceededQuotaErr, > + CryptoSyntaxErr, These are in Source/WebCore/dom/DOMCoreException.idl already > Source/WebCore/crypto/CryptoExceptionCode.h:39 > + CryptoOperationErr I think you'd need to add the DataErr and OperationErr in the idl files of the subtlecrypto module instead.
Comment on attachment 238601 [details] Patch r- as per review in previous comment
You'll need a rebase of the patch, the files were renamed when included in the EFL build.
Created attachment 242797 [details] Patch
Attachment 242797 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Philn, I just uploaded a rebased version of the patch, but it still doesn't address your comments about generating the enums from subtle crypto IDL files. I'm still getting that to work.
Comment on attachment 242797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242797&action=review This patch needs a bit more work. It's a bit of a nitpick, but please use comments that align with WebKit style (full sentences with capital letter and a period). > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:41 > + ASSERT(sizeof(parameters.iv) == gnutls_cipher_get_iv_size(algorithm)); If this cannot be a static assertion, it should be a runtime failure IMO. Causing a runtime error on a debug build isn't useful when this is on some random system that isn't using a debug build. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:45 > + return 0; This should be nullptr, since the handle is a pointer. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:52 > + gnutlsIv.data = const_cast<unsigned char *>(reinterpret_cast<const unsigned char*>(parameters.iv.data())); Kinda spooky to cast away the constness. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:58 > + int ret = gnutls_cipher_init(&cipher, algorithm, &gnutlsKey, &gnutlsIv); > + if (ret != GNUTLS_E_SUCCESS) > + return 0; No need for the temporary value here. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:66 > + // in this context, AES_256_CBC is a valid algorithm also for 128 and 192 > + gnutls_cipher_algorithm_t algorithm = GNUTLS_CIPHER_AES_128_CBC; If AES_256_CBC is valid, why do you then use GNUTLS_CIPHER_AES_128_CBC. Perhaps either the code of the comment is incorrect? > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:70 > + ec = CryptoDataErr; The exception code isn't set for the Mac port. It's best to match behavior for now and fix the problem all at once in a separate bug, I think. Compatibility with other WebKit ports is more important for the moment, and we want this work to keep moving forward. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:79 > + uint8_t paddingSize = blockSize - (dataSize % blockSize); > + size_t paddedSize = (dataSize / blockSize) * blockSize; It might be clearer to calculate one in terms of the other. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:88 > + int ret; > + if (paddedSize > 0) { > + ret = gnutls_cipher_encrypt2(cipher, (void *) data.first, paddedSize, (void *) result.data(), paddedSize); > + if (ret != GNUTLS_E_SUCCESS) { 'ret' is unnecessary here. You should not use C-style casts in C++ code. If code accepts void pointers, perhaps the cast is unnecessary to begin with? > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:99 > + // Add PKCS7 padding <http://tools.ietf.org/html/rfc5652#section-6.3> > + // and encrypt last block > + memcpy(result.data() + paddedSize, data.first + paddedSize, blockSize - paddingSize); > + memset(result.data() + cipherTextSize - paddingSize, paddingSize, paddingSize); I wonder if it is possible to calculate the result directly into the result vector and avoid the copy. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:103 > + ret = gnutls_cipher_encrypt2(cipher, result.data() + paddedSize, blockSize, (void *) (result.data() + paddedSize), blockSize); > + if (ret != GNUTLS_E_SUCCESS) { You can avoid ret here as well. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:105 > + ec = CryptoOperationErr; Indentation is wrong here. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:126 > + Vector<uint8_t> result(data.second); The Mac port asks CommonCrypto for the size. Why don't we have to do that here? > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:128 > + int ret = gnutls_cipher_decrypt2(cipher, data.first, data.second, (void *) result.data(), result.size()); > + if (ret != GNUTLS_E_SUCCESS) { ret can be eliminated. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:136 > + size_t dataSize = result.size(); If you are going to store this in a variable, I'd do it before you use data.second and call it resultDataSize for clarity. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:138 > + uint8_t paddingSize = result.data() [dataSize - 1]; Extra space after data(). > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:147 > + if (result.data() [dataSize - i] != paddingSize) { Ditto. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:155 > + result.shrink(dataSize - paddingSize); Ah, perhaps this is where we determine the output size? Is it possible to calculate ahead of time and avoid the extra memory allocation?
Zan, what are your plans for this?
(In reply to Michael Catanzaro from comment #15) > Zan, what are your plans for this? libgcrypt-based implementation is currently stashed in the patch in bug #133122.
Forgot there was an existing bug for CBC support -- finished in bug #170550. *** This bug has been marked as a duplicate of bug 170550 ***