[Webkit-unassigned] [Bug 133344] [GTK] Adds implementation of subtle crypto AES-CBC algorithm

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=133344

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #242797|review?                     |review-
              Flags|                            |

--- Comment #14 from Martin Robinson <mrobinson at webkit.org> ---
Comment on attachment 242797
  --> https://bugs.webkit.org/attachment.cgi?id=242797
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150423/e84b4695/attachment-0001.html>