Bug 133344 - [GTK] Adds implementation of subtle crypto AES-CBC algorithm
Summary: [GTK] Adds implementation of subtle crypto AES-CBC algorithm
Status: RESOLVED DUPLICATE of bug 170550
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 133316 133317
Blocks: 133122
  Show dependency treegraph
 
Reported: 2014-05-28 03:52 PDT by Eduardo Lima Mitev
Modified: 2017-04-07 05:06 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.31 KB, patch)
2014-05-28 03:57 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (8.83 KB, patch)
2014-09-23 09:46 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (10.74 KB, patch)
2014-09-24 10:25 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (10.84 KB, patch)
2014-12-08 03:22 PST, Eduardo Lima Mitev
mrobinson: review-
elima: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eduardo Lima Mitev 2014-05-28 03:52:42 PDT
[GTK] Adds implementation of subtle crypto AES-CBC algorithm
Comment 1 Eduardo Lima Mitev 2014-05-28 03:57:56 PDT
Created attachment 232189 [details]
Patch
Comment 2 Eduardo Lima Mitev 2014-05-28 03:59:30 PDT
Adding dependency on bug 133316 and bug 133317.
Comment 3 Eduardo Lima Mitev 2014-09-23 09:46:02 PDT
Created attachment 238545 [details]
Patch
Comment 4 WebKit Commit Bot 2014-09-23 09:47:17 PDT
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.
Comment 5 Philippe Normand 2014-09-24 00:49:14 PDT
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
Comment 6 Eduardo Lima Mitev 2014-09-24 10:25:17 PDT
Created attachment 238601 [details]
Patch
Comment 7 WebKit Commit Bot 2014-09-24 10:27:49 PDT
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.
Comment 8 Philippe Normand 2014-09-24 23:56:27 PDT
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.
Comment 9 Philippe Normand 2014-09-30 07:11:55 PDT
Comment on attachment 238601 [details]
Patch

r- as per review in previous comment
Comment 10 Philippe Normand 2014-12-05 06:59:49 PST
You'll need a rebase of the patch, the files were renamed when included in the EFL build.
Comment 11 Eduardo Lima Mitev 2014-12-08 03:22:26 PST
Created attachment 242797 [details]
Patch
Comment 12 WebKit Commit Bot 2014-12-08 03:24:20 PST
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.
Comment 13 Eduardo Lima Mitev 2014-12-08 03:25:54 PST
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.
Comment 14 Martin Robinson 2015-04-23 16:16:16 PDT
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?
Comment 15 Michael Catanzaro 2017-04-01 18:07:11 PDT
Zan, what are your plans for this?
Comment 16 Zan Dobersek 2017-04-02 23:33:49 PDT
(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.
Comment 17 Zan Dobersek 2017-04-07 05:06:15 PDT
Forgot there was an existing bug for CBC support -- finished in bug #170550.

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