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

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Eduardo Lima Mitev]

[GTK] Adds implementation of subtle crypto AES-CBC algorithm

[Eduardo Lima Mitev]

Created attachment 232189 [details]
Patch

[Eduardo Lima Mitev]

Adding dependency on bug 133316 and bug 133317.

[Eduardo Lima Mitev]

Created attachment 238545 [details]
Patch

[WebKit Commit Bot]

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.

[Philippe Normand]

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

[Eduardo Lima Mitev]

Created attachment 238601 [details]
Patch

[WebKit Commit Bot]

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.

[Philippe Normand]

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.

[Philippe Normand]

Comment on attachment 238601 [details]
Patch

r- as per review in previous comment

[Philippe Normand]

You'll need a rebase of the patch, the files were renamed when included in the EFL build.

[Eduardo Lima Mitev]

Created attachment 242797 [details]
Patch

[WebKit Commit Bot]

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.

[Eduardo Lima Mitev]

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.

[Martin Robinson]

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?

[Michael Catanzaro]

Zan, what are your plans for this?

[Zan Dobersek]

(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.

[Zan Dobersek]

Forgot there was an existing bug for CBC support -- finished in bug #170550.

*** This bug has been marked as a duplicate of bug 170550 ***