<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mrobinson@webkit.org" title="Martin Robinson <mrobinson@webkit.org>"> <span class="fn">Martin Robinson</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Adds implementation of subtle crypto AES-CBC algorithm"
href="https://bugs.webkit.org/show_bug.cgi?id=133344">bug 133344</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #242797 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Adds implementation of subtle crypto AES-CBC algorithm"
href="https://bugs.webkit.org/show_bug.cgi?id=133344#c14">Comment # 14</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Adds implementation of subtle crypto AES-CBC algorithm"
href="https://bugs.webkit.org/show_bug.cgi?id=133344">bug 133344</a>
from <span class="vcard"><a class="email" href="mailto:mrobinson@webkit.org" title="Martin Robinson <mrobinson@webkit.org>"> <span class="fn">Martin Robinson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=242797&action=diff" name="attach_242797" title="Patch">attachment 242797</a> <a href="attachment.cgi?id=242797&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=242797&action=review">https://bugs.webkit.org/attachment.cgi?id=242797&action=review</a>
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).
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:41
> + ASSERT(sizeof(parameters.iv) == gnutls_cipher_get_iv_size(algorithm));</span >
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.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:45
> + return 0;</span >
This should be nullptr, since the handle is a pointer.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:52
> + gnutlsIv.data = const_cast<unsigned char *>(reinterpret_cast<const unsigned char*>(parameters.iv.data()));</span >
Kinda spooky to cast away the constness.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:58
> + int ret = gnutls_cipher_init(&cipher, algorithm, &gnutlsKey, &gnutlsIv);
> + if (ret != GNUTLS_E_SUCCESS)
> + return 0;</span >
No need for the temporary value here.
<span class="quote">> 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;</span >
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?
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:70
> + ec = CryptoDataErr;</span >
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.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:79
> + uint8_t paddingSize = blockSize - (dataSize % blockSize);
> + size_t paddedSize = (dataSize / blockSize) * blockSize;</span >
It might be clearer to calculate one in terms of the other.
<span class="quote">> 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) {</span >
'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?
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:99
> + // Add PKCS7 padding <<a href="http://tools.ietf.org/html/rfc5652#section-6.3">http://tools.ietf.org/html/rfc5652#section-6.3</a>>
> + // and encrypt last block
> + memcpy(result.data() + paddedSize, data.first + paddedSize, blockSize - paddingSize);
> + memset(result.data() + cipherTextSize - paddingSize, paddingSize, paddingSize);</span >
I wonder if it is possible to calculate the result directly into the result vector and avoid the copy.
<span class="quote">> 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) {</span >
You can avoid ret here as well.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:105
> + ec = CryptoOperationErr;</span >
Indentation is wrong here.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:126
> + Vector<uint8_t> result(data.second);</span >
The Mac port asks CommonCrypto for the size. Why don't we have to do that here?
<span class="quote">> 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) {</span >
ret can be eliminated.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:136
> + size_t dataSize = result.size();</span >
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.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:138
> + uint8_t paddingSize = result.data() [dataSize - 1];</span >
Extra space after data().
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:147
> + if (result.data() [dataSize - i] != paddingSize) {</span >
Ditto.
<span class="quote">> Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:155
> + result.shrink(dataSize - paddingSize);</span >
Ah, perhaps this is where we determine the output size? Is it possible to calculate ahead of time and avoid the extra memory allocation?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>