Bug 170550 – [GCrypt] Implement AES_CBC support

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GCrypt] Implement AES_CBC support

[Zan Dobersek]

Created attachment 306392 [details]
Patch

[Build Bot]

Attachment 306392 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:153:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:181:  CryptoAlgorithmAES_CBC::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 306392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306392&action=review

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:69
> +        size_t paddedSize = roundUpToMultipleOf(gcry_cipher_get_algo_blklen(*algorithm), size + 1);

What's the +1 for?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:71
> +        size_t padding = paddedSize - size;

I think paddingValue would be a clearer name for this variable.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:72
> +        ASSERT(padding > 0);

I'm confused by this assert. Does padding really have to be greater than zero? What happens if the message length is evenly divisible by block size? And, although unlikely, what if the size of the message is one less than the size of the block? You're sure this assert can never be triggered by web content?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:75
> +        for (size_t i = size; i < paddedSize; ++i)
> +            plainText[i] = padding;

You prefer writing out this loop to just using memcpy()? I wonder if memcpy() might be more efficient.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:157
> +        [parameters = WTFMove(parameters), key = WTFMove(key), plainText = WTFMove(plainText), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback), &context]() mutable {

I should maybe have asked this before, but why does it have to be mutable?

[Michael Catanzaro]

Comment on attachment 306392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306392&action=review

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:145
> +        for (size_t i = size - padding; i < size; ++i) {
> +            if (output[i] != padding)
> +                return std::nullopt;
> +        }

And this could be a memcmp().

[Jiewen Tan]

Comment on attachment 306392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306392&action=review

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:157
>> +        [parameters = WTFMove(parameters), key = WTFMove(key), plainText = WTFMove(plainText), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback), &context]() mutable {
> 
> I should maybe have asked this before, but why does it have to be mutable?

Callbacks are moved later on. Therefore, it has to be mutable.

[Zan Dobersek]

Comment on attachment 306392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306392&action=review

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:69
>> +        size_t paddedSize = roundUpToMultipleOf(gcry_cipher_get_algo_blklen(*algorithm), size + 1);
> 
> What's the +1 for?

It rounds up the size value to the next multiple of the cipher's block length. I'll add a comment.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:71
>> +        size_t padding = paddedSize - size;
> 
> I think paddingValue would be a clearer name for this variable.

OK.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:72
>> +        ASSERT(padding > 0);
> 
> I'm confused by this assert. Does padding really have to be greater than zero? What happens if the message length is evenly divisible by block size? And, although unlikely, what if the size of the message is one less than the size of the block? You're sure this assert can never be triggered by web content?

If the message length is an exact multiple of the block size, it gets rounded to the next multiple in the roundUpToMultipleOf() call above because of the `size + 1` input.
If the message length is one less than the size of the block, then it gets rounded up to the size of the block, with the padding consisting of the single 0x01 byte.

Overall this assert doesn't do that much, assuming that the Vector<> object isn't somehow holding close to 2 ** 64 bytes of data or that gcry_cipher_get_algo_blklen() isn't returning an incorrect value.

The other important thing I missed here is that the padding value has to be small enough to fit into a byte.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:75
>> +            plainText[i] = padding;
> 
> You prefer writing out this loop to just using memcpy()? I wonder if memcpy() might be more efficient.

memset() would probably be better here.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:145
>> +        }
> 
> And this could be a memcmp().

For memcmp() you'd have to construct a separate uint8_t array of `padding` length that's filled with `padding` values.

This better matches std::count():

  if (std::count(output.end() - padding, output.end(), padding) != padding)
      return std::nullopt;

[Zan Dobersek]

Created attachment 306474 [details]
Patch for landing

[Build Bot]

Attachment 306474 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:156:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:184:  CryptoAlgorithmAES_CBC::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 306474 [details]
Patch for landing

Clearing flags on attachment: 306474

Committed r215087: <http://trac.webkit.org/changeset/215087>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #6)
> The other important thing I missed here is that the padding value has to be
> small enough to fit into a byte.

Yeah, I thought about complaining about your use of size_t for the padding value,  which seemed pretty silly, but decided not to since you got to it by adding two size_t values.

[Zan Dobersek]

*** Bug 133344 has been marked as a duplicate of this bug. ***