Bug 170271

Created attachment 305840 [details] Patch

Attachment 305840 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:164: CryptoAlgorithmAES_GCM::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:190: CryptoAlgorithmAES_GCM::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:86: Missing space before { [whitespace/braces] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 305845 [details] Patch

Attachment 305845 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:166: CryptoAlgorithmAES_GCM::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:192: CryptoAlgorithmAES_GCM::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Depends on the Utilities.h header that's being added in bug #170269.

Created attachment 305949 [details] Patch

Attachment 305949 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:166: CryptoAlgorithmAES_GCM::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:192: CryptoAlgorithmAES_GCM::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 305949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305949&action=review > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:39 > +#include <pal/crypto/gcrypt/Handle.h> > +#include <pal/crypto/gcrypt/Utilities.h> ... > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:46 > + auto algorithm = PAL::GCrypt::aesAlgorithmForKeySize(key.size() * 8); Ugh, there's no good way to avoid having to write PAL::GCrypt everywhere, is there? Because a simple "using PAL::GCrypt" is not going to work, right? This is unfortunate. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:51 > + gcry_error_t error = gcry_cipher_open(&handle, *algorithm, GCRY_CIPHER_MODE_GCM, 0); We should probably pass GCRY_CIPHER_SECURE instead of 0 to ensure the key doesn't get paged to disk...? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:98 > + output.appendVector(tag); It feels confusing/fragile, but I guess it's hard to change the cross-platform interface for this? Or is this how the data is expected to be received by the JS API? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:101 > + return WTFMove(output); Remove WTFMove. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:158 > + size_t offset = cipherText.size() - tagLength; I'd declare this up above gcry_cipher_decrypt() so that you can use it on the line following that too. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:163 > + return WTFMove(output); No WTFMove. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:177 > + [callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) { Don't capture callback. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:185 > + [output = WTFMove(*output), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) mutable { Don't capture exceptionCallback. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:203 > + [callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) { Ditto. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:211 > + [output = WTFMove(*output), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) mutable { Ditto.

Comment on attachment 305949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305949&action=review >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:46 >> + auto algorithm = PAL::GCrypt::aesAlgorithmForKeySize(key.size() * 8); > > Ugh, there's no good way to avoid having to write PAL::GCrypt everywhere, is there? Because a simple "using PAL::GCrypt" is not going to work, right? This is unfortunate. I think it would work, but I would revisit this later, after the majority of changes lands. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:51 >> + gcry_error_t error = gcry_cipher_open(&handle, *algorithm, GCRY_CIPHER_MODE_GCM, 0); > > We should probably pass GCRY_CIPHER_SECURE instead of 0 to ensure the key doesn't get paged to disk...? OK. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:98 >> + output.appendVector(tag); > > It feels confusing/fragile, but I guess it's hard to change the cross-platform interface for this? Or is this how the data is expected to be received by the JS API? Confusing/fragile how? Change it how? As it stands, the callback object in the custom JS bindings works with a Vector<uint8_t> object whose contents are then copied into an ArrayBuffer. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_GCMGCrypt.cpp:158 >> + size_t offset = cipherText.size() - tagLength; > > I'd declare this up above gcry_cipher_decrypt() so that you can use it on the line following that too. OK.

Created attachment 306065 [details] Patch for landing

Comment on attachment 306065 [details] Patch for landing Clearing flags on attachment: 306065 Committed r214822: <http://trac.webkit.org/changeset/214822>

All reviewed patches have been landed. Closing bug.

I think this broke the GTK EWS build? https://webkit-queues.webkit.org/results/3466261

(In reply to Tim Horton from comment #13) > I think this broke the GTK EWS build? > https://webkit-queues.webkit.org/results/3466261 Committed r214837: <http://trac.webkit.org/changeset/214837>