Code review comment for lp:~laurynas-biveinis/percona-server/merge-5.6.17

Hi Laurynas,

On Tue, Apr 22 2014 17:42:58 +0400, Laurynas Biveinis wrote:

>> Changes to lock_rec_other_trx_holds_expl() look okay-ish, though they
>> could benefit from the trx descriptors optimization by replacing
>> pointers to transactions with their IDs. I don’t insist, since it’s a
>> debug-only code.
>
> Debug-only code was the reason why I did not do this the first time
> around, but why not. Does the following look reasonable?
>

Hm, not quite. It still looks up pointers to transactions, and for that
it has iterate rw_trx_list by calling trx_rw_get_active_trx_by_id(). And
it also has to do so for each trx from rw_trx_snapshot, which makes it
even less efficient than the upstream implementation. It looks like
lock_rec_has_expl() does not really need a pointer to a transaction
either. It only uses it to match against lock->trx. So by changing it to
accept trx ID as the last argument and matching against lock->trx->id
instead, we can avoid the trx_rw_get_active_trx_by_id() call in
lock_rec_other_has_expl(), and potentially optimize some non-debug code
as well!

But again, I don’t insist. Just pointing out that the current implementation
is even less efficient than the upstream one.

>> But the OpenSSL changes are wrong. It looks like the only issue with
>> OpenSSL < 0.9.8l that breaks AES_ENCRYPT/AES_DECRYPT compatibility is
>> that those old versions incorrectly report the required IV length with
>> AES in ECB mode. IV is not required by the ECB cipher, so
>> EVP_CIPHER_iv_length() should return 0, whereas old versions
>> return 16.
>
> Yes.
>
>> Which confuses my_aes_needs_iv(), in particular, and makes the server
>> believe AES_*() functions require the 3rd argument with
>> block_encryption_mode=aes-128-ecb (i.e. an IV).
>
> Yes.
>
>> Which is also an upstream issue, i.e. a workaround for old OpenSSL
>> libs is required. And the workaround is simple: whenever
>> EVP_CIPHER_iv_length() is used to get the required IV length, also
>> check EVP_CIPHER_mode(). If it’s EVP_CIPH_ECB_MODE, disregard the
>> value returned by EVP_CIPHER_iv_length() and assume the
>> required IV length to be 0.
>
> OK, that's a better workaround than mine.
>

Will you report it upstream? AFAIK it is not documented anywhere that
OpenSSL < 0.9.8l is not supported, so it’s a genuine upstream issue.

>> The approach implemented in this MP is rather dangerous: we make the
>> server pretend that it accepts IV when required, when in fact it is
>> ignored for old OpenSSL versions. Security folks may get upset :)
>
> My idea was to make MySQL call OpenSSL in exactly same way (Foo vs
> FooEx aside) it was in 5.6.16-, including 5.5 GA etc. So that
> implementation would be as secure or insecure as, say, current 5.5.
>

I meant that by accepting IV for ciphers that require it, but ignoring
it the server would:

1. Generate cryptographically weak results with non-default
block_encryption_mode values, while users would assume them to be
cryptographically strong.

2. Generate results that would not be decryptable on servers with recent
OpenSSL versions that would actually take IV into account.