Bug 171420

Created attachment 308507 [details] WIP Works, but needs a simpler handling of wrap-around counters.

Attachment 308507 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:44: gcryptAES_CTR is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:201: CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:229: CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 4 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 308522 [details] Patch Uploading the initial patch, cleaned up, working, but with the block-by-block fallback for wrapping counters.

Attachment 308522 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:44: gcryptAES_CTR is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:204: CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:232: CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 308522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308522&action=review > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:42 > +using GCryptCipherOperation = gcry_error_t(gcry_cipher_hd_t, void*, size_t, const void*, size_t); This belongs in Utilities.h. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:69 > + // This is a helper functor that resets the cipher object, sets the provided counter data, > + // and executes the encrypt or decrypt operation, retrieving and returning the output data. > + auto callOperation = > + [&handle, &operation](const Vector<uint8_t>& counter, const uint8_t* inputData, const size_t inputSize) -> std::optional<Vector<uint8_t>> I don't see any reason to use a lambda here, when it could be a static function instead. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:101 > + { Nice use of extra scoping. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:103 > + PAL::GCrypt::Handle<gcry_mpi_t> blockSizeMaskMPI(gcry_mpi_set_ui(nullptr, blockSize - 1)); Why is this named "blockSizeMaskMPI" if it is being used as part of an addition operation rather than a bitmask operation? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:114 > + gcry_mpi_mul_2exp(counterLimitMPI, counterLimitMPI, counterLength); Same as in my review from a while back, this probably isn't worth worrying about at all as I doubt GCrypt will ever break this, but it will fail if GCrypt ever requires that the parameters be distinct. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:117 > + // the number of unique counter values we could procude for the specified counter Did you mean "produce"? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:168 > + // FIXME: This should be optimized further by first encrypting the amount of blocks for > + // which the counter won't yet wrap around, and then encrypting the rest of the blocks > + // starting from the counter set to 0. Is this not straightforward to implement? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:212 > + auto& aesParameters = downcast<CryptoAlgorithmAesCtrParams>(*parameters); > + auto& aesKey = downcast<CryptoKeyAES>(key.get()); > + > + auto output = gcryptAES_CTR(gcry_cipher_encrypt, aesKey.key(), aesParameters.counterVector(), aesParameters.length, plainText); Can you think of any way to share more of the implementation of this function with the other CryptoAlgorithm classes? It looks like these three lines are the only ones that ever differ.... > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:240 > + auto& aesParameters = downcast<CryptoAlgorithmAesCtrParams>(*parameters); > + auto& aesKey = downcast<CryptoKeyAES>(key.get()); > + > + auto output = gcryptAES_CTR(gcry_cipher_decrypt, aesKey.key(), aesParameters.counterVector(), aesParameters.length, cipherText); Ditto. A follow-up patch should work on reducing code duplication between all the d ifferent platformEncrypt/Decrypt/Sign/Verify functions, in addition to the static functions at the top of the file. I think the only code that needs to differ is this bit here that converts the CryptoAlgorithmParameters and CryptoKey into a call to the algorithm (gcryptAES_CTR here).

Comment on attachment 308522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308522&action=review Good job. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:99 > + // Calculate the block count: ((inputText.size() + blockSize - 1) / blockSize), remainder discarded. Potential overflow? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:125 > + if (counterLength == counter.size() * 8) Have you confirmed that GCrypt will not fail once the counter reaches its max? As far as I know, different libraries seem to treat this differently. Some will throw exception/fail, but some will reset the counter, and then start from 0. It is clever to have this early return though. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:212 >> + auto output = gcryptAES_CTR(gcry_cipher_encrypt, aesKey.key(), aesParameters.counterVector(), aesParameters.length, plainText); > > Can you think of any way to share more of the implementation of this function with the other CryptoAlgorithm classes? It looks like these three lines are the only ones that ever differ.... Good point. I was thinking about doing this in Apple ports as well. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:240 >> + auto output = gcryptAES_CTR(gcry_cipher_decrypt, aesKey.key(), aesParameters.counterVector(), aesParameters.length, cipherText); > > Ditto. A follow-up patch should work on reducing code duplication between all the d ifferent platformEncrypt/Decrypt/Sign/Verify functions, in addition to the static functions at the top of the file. I think the only code that needs to differ is this bit here that converts the CryptoAlgorithmParameters and CryptoKey into a call to the algorithm (gcryptAES_CTR here). Please CC me for the follow-up patch.

Comment on attachment 308522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308522&action=review >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:99 >> + // Calculate the block count: ((inputText.size() + blockSize - 1) / blockSize), remainder discarded. > > Potential overflow? I assume that it can't overflow since it's a multi-precision integer... right?

Comment on attachment 308522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308522&action=review BTW, please add test cases that are tied to your GCrypt implementation. For example, the one take, the leeway and so on. >>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:99 >>> + // Calculate the block count: ((inputText.size() + blockSize - 1) / blockSize), remainder discarded. >> >> Potential overflow? > > I assume that it can't overflow since it's a multi-precision integer... right? Sorry, in reality, it shouldn't.

Comment on attachment 308522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308522&action=review >>>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:99 >>>> + // Calculate the block count: ((inputText.size() + blockSize - 1) / blockSize), remainder discarded. >>> >>> Potential overflow? >> >> I assume that it can't overflow since it's a multi-precision integer... right? > > Sorry, in reality, it shouldn't. It shouldn't, and the MPI operations in the following scope are used with that in mind. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:103 >> + PAL::GCrypt::Handle<gcry_mpi_t> blockSizeMaskMPI(gcry_mpi_set_ui(nullptr, blockSize - 1)); > > Why is this named "blockSizeMaskMPI" if it is being used as part of an addition operation rather than a bitmask operation? The MPI is actually not necessary at all. Should be fine to set the roundedUpSize MPI to inputText.size(), and then just add `blockSize - 1` (i.e. 15) to that. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:125 >> + if (counterLength == counter.size() * 8) > > Have you confirmed that GCrypt will not fail once the counter reaches its max? As far as I know, different libraries seem to treat this differently. Some will throw exception/fail, but some will reset the counter, and then start from 0. > > It is clever to have this early return though. libgcrypt won't return an error once the counter reaches the maximum value -- it will just wrap it around silently. https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=blob;f=cipher/cipher-ctr.c;h=f9cb6b577b75a1a0727d2cbebf753e8afa7417ac;hb=refs/heads/master#l78 >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:168 >> + // starting from the counter set to 0. > > Is this not straightforward to implement? I'd approach it later. The problem is we require MPI-to-size_t decoding, which needs more attention. >>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:240 >>> + auto output = gcryptAES_CTR(gcry_cipher_decrypt, aesKey.key(), aesParameters.counterVector(), aesParameters.length, cipherText); >> >> Ditto. A follow-up patch should work on reducing code duplication between all the d ifferent platformEncrypt/Decrypt/Sign/Verify functions, in addition to the static functions at the top of the file. I think the only code that needs to differ is this bit here that converts the CryptoAlgorithmParameters and CryptoKey into a call to the algorithm (gcryptAES_CTR here). > > Please CC me for the follow-up patch. OK, I can look at that afterwards.

Created attachment 313072 [details] Patch for landing

Comment on attachment 313072 [details] Patch for landing Clearing flags on attachment: 313072 Committed r218484: <http://trac.webkit.org/changeset/218484>

All reviewed patches have been landed. Closing bug.